From 39c8e50ad0513649f5a8cef911b7d6cdd61c2372 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Thu, 15 Aug 2024 10:36:04 +0300 Subject: [PATCH] fix: Require mfa code to change email (#10354) --- .../__tests__/me.controller.test.ts | 96 +++++++++++++++++-- packages/cli/src/controllers/me.controller.ts | 39 +++++--- packages/cli/src/requests.ts | 5 + packages/editor-ui/src/api/users.ts | 15 +-- packages/editor-ui/src/event-bus/mfa.ts | 2 + packages/editor-ui/src/stores/users.store.ts | 7 +- .../src/views/SettingsPersonalView.vue | 54 +++++++++-- .../__tests__/SettingsPersonalView.test.ts | 4 + 8 files changed, 181 insertions(+), 41 deletions(-) diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/packages/cli/src/controllers/__tests__/me.controller.test.ts index 3ff4c5bbe0860..71a6d693842cf 100644 --- a/packages/cli/src/controllers/__tests__/me.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/me.controller.test.ts @@ -53,9 +53,14 @@ describe('MeController', () => { password: 'password', authIdentities: [], role: 'global:owner', + mfaEnabled: false, }); - const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; - const req = mock({ user, body: reqBody, browserId }); + const req = mock({ user, browserId }); + req.body = { + email: 'valid@email.com', + firstName: 'John', + lastName: 'Potato', + }; const res = mock(); userRepository.findOneByOrFail.mockResolvedValue(user); userRepository.findOneOrFail.mockResolvedValue(user); @@ -67,7 +72,7 @@ describe('MeController', () => { expect(externalHooks.run).toHaveBeenCalledWith('user.profile.beforeUpdate', [ user.id, user.email, - reqBody, + req.body, ]); expect(userService.update).toHaveBeenCalled(); @@ -98,25 +103,25 @@ describe('MeController', () => { password: 'password', authIdentities: [], role: 'global:member', + mfaEnabled: false, }); - const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; const req = mock({ user, browserId }); - req.body = reqBody; + req.body = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; const res = mock(); userRepository.findOneOrFail.mockResolvedValue(user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); // Add invalid data to the request payload - Object.assign(reqBody, { id: '0', role: 'global:owner' }); + Object.assign(req.body, { id: '0', role: 'global:owner' }); await controller.updateCurrentUser(req, res); expect(userService.update).toHaveBeenCalled(); const updatePayload = userService.update.mock.calls[0][1]; - expect(updatePayload.email).toBe(reqBody.email); - expect(updatePayload.firstName).toBe(reqBody.firstName); - expect(updatePayload.lastName).toBe(reqBody.lastName); + expect(updatePayload.email).toBe(req.body.email); + expect(updatePayload.firstName).toBe(req.body.firstName); + expect(updatePayload.lastName).toBe(req.body.lastName); expect(updatePayload.id).toBeUndefined(); expect(updatePayload.role).toBeUndefined(); }); @@ -127,10 +132,11 @@ describe('MeController', () => { password: 'password', authIdentities: [], role: 'global:owner', + mfaEnabled: false, }); const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; const req = mock({ user, body: reqBody }); - // userService.findOneOrFail.mockResolvedValue(user); + req.body = reqBody; // We don't want the body to be a mock object externalHooks.run.mockImplementationOnce(async (hookName) => { if (hookName === 'user.profile.beforeUpdate') { @@ -142,6 +148,76 @@ describe('MeController', () => { new BadRequestError('Invalid email address'), ); }); + + describe('when mfa is enabled', () => { + it('should throw BadRequestError if mfa code is missing', async () => { + const user = mock({ + id: '123', + email: 'valid@email.com', + password: 'password', + authIdentities: [], + role: 'global:owner', + mfaEnabled: true, + }); + const req = mock({ user, browserId }); + req.body = { email: 'new@email.com', firstName: 'John', lastName: 'Potato' }; + + await expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError( + new BadRequestError('Two-factor code is required to change email'), + ); + }); + + it('should throw InvalidMfaCodeError if mfa code is invalid', async () => { + const user = mock({ + id: '123', + email: 'valid@email.com', + password: 'password', + authIdentities: [], + role: 'global:owner', + mfaEnabled: true, + }); + const req = mock({ user, browserId }); + req.body = { + email: 'new@email.com', + firstName: 'John', + lastName: 'Potato', + mfaCode: 'invalid', + }; + mockMfaService.validateMfa.mockResolvedValue(false); + + await expect(controller.updateCurrentUser(req, mock())).rejects.toThrow( + InvalidMfaCodeError, + ); + }); + + it("should update the user's email if mfa code is valid", async () => { + const user = mock({ + id: '123', + email: 'valid@email.com', + password: 'password', + authIdentities: [], + role: 'global:owner', + mfaEnabled: true, + }); + const req = mock({ user, browserId }); + req.body = { + email: 'new@email.com', + firstName: 'John', + lastName: 'Potato', + mfaCode: '123456', + }; + const res = mock(); + userRepository.findOneByOrFail.mockResolvedValue(user); + userRepository.findOneOrFail.mockResolvedValue(user); + jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); + userService.toPublic.mockResolvedValue({} as unknown as PublicUser); + mockMfaService.validateMfa.mockResolvedValue(true); + + const result = await controller.updateCurrentUser(req, res); + + expect(result).toEqual({}); + }); + }); }); describe('updatePassword', () => { diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 179076d25d224..19429228edead 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -54,7 +54,8 @@ export class MeController { */ @Patch('/') async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise { - const { id: userId, email: currentEmail } = req.user; + const { id: userId, email: currentEmail, mfaEnabled } = req.user; + const payload = plainToInstance(UserUpdatePayload, req.body, { excludeExtraneousValues: true }); const { email } = payload; @@ -76,17 +77,28 @@ export class MeController { await validateEntity(payload); + const isEmailBeingChanged = email !== currentEmail; + // If SAML is enabled, we don't allow the user to change their email address - if (isSamlLicensedAndEnabled()) { - if (email !== currentEmail) { - this.logger.debug( - 'Request to update user failed because SAML user may not change their email', - { - userId, - payload, - }, - ); - throw new BadRequestError('SAML user may not change their email'); + if (isSamlLicensedAndEnabled() && isEmailBeingChanged) { + this.logger.debug( + 'Request to update user failed because SAML user may not change their email', + { + userId, + payload, + }, + ); + throw new BadRequestError('SAML user may not change their email'); + } + + if (mfaEnabled && isEmailBeingChanged) { + if (!payload.mfaCode) { + throw new BadRequestError('Two-factor code is required to change email'); + } + + const isMfaTokenValid = await this.mfaService.validateMfa(userId, payload.mfaCode, undefined); + if (!isMfaTokenValid) { + throw new InvalidMfaCodeError(); } } @@ -102,8 +114,9 @@ export class MeController { this.authService.issueCookie(res, user, req.browserId); - const fieldsChanged = (Object.keys(payload) as Array).filter( - (key) => payload[key] !== preUpdateUser[key], + const changeableFields = ['email', 'firstName', 'lastName'] as const; + const fieldsChanged = changeableFields.filter( + (key) => key in payload && payload[key] !== preUpdateUser[key], ); this.eventService.emit('user-updated', { user, fieldsChanged }); diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 436c0b1b18d94..581ad920ea82d 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -43,6 +43,11 @@ export class UserUpdatePayload implements Pick { return await makeRestApiRequest(context, 'PATCH', '/me', params); } diff --git a/packages/editor-ui/src/event-bus/mfa.ts b/packages/editor-ui/src/event-bus/mfa.ts index a50b6512b7630..290ac46ca67e6 100644 --- a/packages/editor-ui/src/event-bus/mfa.ts +++ b/packages/editor-ui/src/event-bus/mfa.ts @@ -7,8 +7,10 @@ export interface MfaModalClosedEventPayload { } export interface MfaModalEvents { + /** Command to request closing of the modal */ close: MfaModalClosedEventPayload | undefined; + /** Event that the modal has been closed */ closed: MfaModalClosedEventPayload | undefined; } diff --git a/packages/editor-ui/src/stores/users.store.ts b/packages/editor-ui/src/stores/users.store.ts index a07c40193dc5d..ef153bb4b7e5e 100644 --- a/packages/editor-ui/src/stores/users.store.ts +++ b/packages/editor-ui/src/stores/users.store.ts @@ -224,12 +224,7 @@ export const useUsersStore = defineStore(STORES.USERS, () => { await usersApi.changePassword(rootStore.restApiContext, params); }; - const updateUser = async (params: { - id: string; - firstName: string; - lastName: string; - email: string; - }) => { + const updateUser = async (params: usersApi.UpdateCurrentUserParams) => { const user = await usersApi.updateCurrentUser(rootStore.restApiContext, params); addUsers([user]); }; diff --git a/packages/editor-ui/src/views/SettingsPersonalView.vue b/packages/editor-ui/src/views/SettingsPersonalView.vue index 08cc4ed12896a..60b40fb6863ff 100644 --- a/packages/editor-ui/src/views/SettingsPersonalView.vue +++ b/packages/editor-ui/src/views/SettingsPersonalView.vue @@ -16,6 +16,16 @@ import { createFormEventBus } from 'n8n-design-system/utils'; import type { MfaModalEvents } from '@/event-bus/mfa'; import { promptMfaCodeBus } from '@/event-bus/mfa'; +type UserBasicDetailsForm = { + firstName: string; + lastName: string; + email: string; +}; + +type UserBasicDetailsWithMfa = UserBasicDetailsForm & { + mfaCode?: string; +}; + const i18n = useI18n(); const { showToast, showError } = useToast(); @@ -114,12 +124,17 @@ onMounted(() => { function onInput() { hasAnyBasicInfoChanges.value = true; } + function onReadyToSubmit(ready: boolean) { readyToSubmit.value = ready; } -async function onSubmit(form: { firstName: string; lastName: string; email: string }) { + +/** Saves users basic info and personalization settings */ +async function saveUserSettings(params: UserBasicDetailsWithMfa) { try { - await Promise.all([updateUserBasicInfo(form), updatePersonalisationSettings()]); + // The MFA code might be invalid so we update the user's basic info first + await updateUserBasicInfo(params); + await updatePersonalisationSettings(); showToast({ title: i18n.baseText('settings.personal.personalSettingsUpdated'), @@ -130,19 +145,43 @@ async function onSubmit(form: { firstName: string; lastName: string; email: stri showError(e, i18n.baseText('settings.personal.personalSettingsUpdatedError')); } } -async function updateUserBasicInfo(form: { firstName: string; lastName: string; email: string }) { + +async function onSubmit(form: UserBasicDetailsForm) { + if (!usersStore.currentUser?.mfaEnabled) { + await saveUserSettings(form); + return; + } + + uiStore.openModal(PROMPT_MFA_CODE_MODAL_KEY); + + promptMfaCodeBus.once('closed', async (payload: MfaModalEvents['closed']) => { + if (!payload) { + // User closed the modal without submitting the form + return; + } + + await saveUserSettings({ + ...form, + mfaCode: payload.mfaCode, + }); + }); +} + +async function updateUserBasicInfo(userBasicInfo: UserBasicDetailsWithMfa) { if (!hasAnyBasicInfoChanges.value || !usersStore.currentUserId) { return; } await usersStore.updateUser({ id: usersStore.currentUserId, - firstName: form.firstName, - lastName: form.lastName, - email: form.email, + firstName: userBasicInfo.firstName, + lastName: userBasicInfo.lastName, + email: userBasicInfo.email, + mfaCode: userBasicInfo.mfaCode, }); hasAnyBasicInfoChanges.value = false; } + async function updatePersonalisationSettings() { if (!hasAnyPersonalisationChanges.value) { return; @@ -150,12 +189,15 @@ async function updatePersonalisationSettings() { uiStore.setTheme(currentSelectedTheme.value); } + function onSaveClick() { formBus.emit('submit'); } + function openPasswordModal() { uiStore.openModal(CHANGE_PASSWORD_MODAL_KEY); } + function onMfaEnableClick() { uiStore.openModal(MFA_SETUP_MODAL_KEY); } diff --git a/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts b/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts index e62812e9066f5..9c8a330f6ee98 100644 --- a/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts +++ b/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts @@ -92,6 +92,7 @@ describe('SettingsPersonalView', () => { }); it('should commit the theme change after clicking save', async () => { + vi.spyOn(usersStore, 'updateUser').mockReturnValue(Promise.resolve()); const { getByPlaceholderText, findByText, getByTestId } = renderComponent({ pinia }); await waitAllPromises(); @@ -102,6 +103,9 @@ describe('SettingsPersonalView', () => { await waitAllPromises(); getByTestId('save-settings-button').click(); + + await waitAllPromises(); + expect(uiStore.theme).toBe('dark'); }); });