fix: Require mfa code to change email (#10354)

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<MeRequest.UserUpdate>({ user, browserId });
-			const req = mock<MeRequest.UserUpdate>({ user, body: reqBody, browserId });
+			req.body = {
+				email: 'valid@email.com',
+				firstName: 'John',
+				lastName: 'Potato',
+			};
 			const res = mock<Response>();
 			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<MeRequest.UserUpdate>({ user, browserId });
-			req.body = reqBody;
+			req.body = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' };
 			const res = mock<Response>();
 			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.email).toBe(req.body.email);
-			expect(updatePayload.firstName).toBe(reqBody.firstName);
+			expect(updatePayload.firstName).toBe(req.body.firstName);
-			expect(updatePayload.lastName).toBe(reqBody.lastName);
+			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<MeRequest.UserUpdate>({ 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<User>({
+					id: '123',
+					email: 'valid@email.com',
+					password: 'password',
+					authIdentities: [],
+					role: 'global:owner',
+					mfaEnabled: true,
+				});
+				const req = mock<MeRequest.UserUpdate>({ 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<User>({
+					id: '123',
+					email: 'valid@email.com',
+					password: 'password',
+					authIdentities: [],
+					role: 'global:owner',
+					mfaEnabled: true,
+				});
+				const req = mock<MeRequest.UserUpdate>({ 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<User>({
+					id: '123',
+					email: 'valid@email.com',
+					password: 'password',
+					authIdentities: [],
+					role: 'global:owner',
+					mfaEnabled: true,
+				});
+				const req = mock<MeRequest.UserUpdate>({ user, browserId });
+				req.body = {
+					email: 'new@email.com',
+					firstName: 'John',
+					lastName: 'Potato',
+					mfaCode: '123456',
+				};
+				const res = mock<Response>();
+				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', () => {

packages/cli/src/controllers/me.controller.ts

@@ -54,7 +54,8 @@ export class MeController {
 	 */
 	@Patch('/')
 	async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise<PublicUser> {
-		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 (isSamlLicensedAndEnabled() && isEmailBeingChanged) {
-			if (email !== currentEmail) {
+			this.logger.debug(
-				this.logger.debug(
+				'Request to update user failed because SAML user may not change their email',
-					'Request to update user failed because SAML user may not change their email',
+				{
-					{
+					userId,
-						userId,
+					payload,
-						payload,
+				},
-					},
+			);
-				);
+			throw new BadRequestError('SAML user may not change their email');
-				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<keyof UserUpdatePayload>).filter(
+		const changeableFields = ['email', 'firstName', 'lastName'] as const;
-			(key) => payload[key] !== preUpdateUser[key],
+		const fieldsChanged = changeableFields.filter(
+			(key) => key in payload && payload[key] !== preUpdateUser[key],
 		);
 
 		this.eventService.emit('user-updated', { user, fieldsChanged });

packages/cli/src/requests.ts

@@ -43,6 +43,11 @@ export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'la
 	@IsString({ message: 'Last name must be of type string.' })
 	@Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' })
 	lastName: string;
+
+	@IsOptional()
+	@Expose()
+	@IsString({ message: 'Two factor code must be a string.' })
+	mfaCode?: string;
 }
 
 export class UserSettingsUpdatePayload {

packages/editor-ui/src/api/users.ts

@@ -89,14 +89,17 @@ export async function changePassword(
 	await makeRestApiRequest(context, 'POST', '/change-password', params);
 }
 
+export type UpdateCurrentUserParams = {
+	id?: string;
+	firstName?: string;
+	lastName?: string;
+	email: string;
+	mfaCode?: string;
+};
+
 export async function updateCurrentUser(
 	context: IRestApiContext,
-	params: {
+	params: UpdateCurrentUserParams,
-		id?: string;
-		firstName?: string;
-		lastName?: string;
-		email: string;
-	},
 ): Promise<IUserResponse> {
 	return await makeRestApiRequest(context, 'PATCH', '/me', params);
 }

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;
 }
 

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: {
+	const updateUser = async (params: usersApi.UpdateCurrentUserParams) => {
-		id: string;
-		firstName: string;
-		lastName: string;
-		email: string;
-	}) => {
 		const user = await usersApi.updateCurrentUser(rootStore.restApiContext, params);
 		addUsers([user]);
 	};

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,
+		firstName: userBasicInfo.firstName,
-		lastName: form.lastName,
+		lastName: userBasicInfo.lastName,
-		email: form.email,
+		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);
 }

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');
 		});
 	});