feat(core): Prompt user to confirm password when changing email and mfa is disabled (#19408)

Co-authored-by: Marc Littlemore <MarcL@users.noreply.github.com>

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

@@ -87,6 +87,7 @@ describe('MeController', () => {
 			const user = mock<User>({
 				id: '123',
 				password: 'password',
+				email: 'current@email.com',
 				authIdentities: [],
 				role: GLOBAL_OWNER_ROLE,
 				mfaEnabled: false,
@@ -103,7 +104,7 @@ describe('MeController', () => {
 				controller.updateCurrentUser(
 					req,
 					mock(),
-					mock({ email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }),
+					mock({ email: user.email, firstName: 'John', lastName: 'Potato' }),
 				),
 			).rejects.toThrowError(new BadRequestError('Invalid email address'));
 		});
@@ -191,6 +192,133 @@ describe('MeController', () => {
 				expect(result).toEqual({});
 			});
 		});
+
+		describe('when mfa is disabled and email is being changed', () => {
+			const oldPasswordPlain = 'old_password';
+			const passwordHash = '$2a$10$ffitcKrHT.Ls.m9FfWrMrOod76aaI0ogKbc3S96Q320impWpCbgj6'; // Hashed 'old_password'
+
+			it('should throw BadRequestError if currentPassword is missing', async () => {
+				const user = mock<User>({
+					id: '123',
+					email: 'michel-old@email.com',
+					password: passwordHash,
+					mfaEnabled: false,
+				});
+				const req = mock<AuthenticatedRequest>({ user, browserId });
+
+				await expect(
+					controller.updateCurrentUser(
+						req,
+						mock(),
+						new UserUpdateRequestDto({
+							email: 'michel-new@email.com',
+							firstName: 'Michel',
+							lastName: 'n8n',
+						}),
+					),
+				).rejects.toThrowError(new BadRequestError('Current password is required to change email'));
+			});
+
+			it('should throw BadRequestError if currentPassword is not a string', async () => {
+				const user = mock<User>({
+					id: '123',
+					email: 'michel-old@email.com',
+					password: passwordHash,
+					mfaEnabled: false,
+				});
+				const req = mock<AuthenticatedRequest>({ user, browserId });
+
+				await expect(
+					controller.updateCurrentUser(req, mock(), {
+						email: 'michel-new@email.com',
+						firstName: 'Michel',
+						lastName: 'n8n',
+						currentPassword: 123 as any,
+					} as any),
+				).rejects.toThrowError(new BadRequestError('Current password is required to change email'));
+			});
+
+			it('should throw BadRequestError if currentPassword is incorrect', async () => {
+				const user = mock<User>({
+					email: 'michel-old@email.com',
+					password: passwordHash,
+					mfaEnabled: false,
+				});
+				const req = mock<AuthenticatedRequest>({ user, browserId });
+
+				await expect(
+					controller.updateCurrentUser(
+						req,
+						mock(),
+						mock({
+							email: 'michel-new@email.com',
+							firstName: 'Michel',
+							lastName: 'n8n',
+							currentPassword: 'wrong-password',
+						}),
+					),
+				).rejects.toThrowError(
+					new BadRequestError(
+						'Unable to update profile. Please check your credentials and try again.',
+					),
+				);
+			});
+
+			it('should update the user email if currentPassword is correct', async () => {
+				const user = mock<User>({
+					email: 'michel-old@email.com',
+					password: passwordHash,
+					mfaEnabled: false,
+				});
+				const req = mock<AuthenticatedRequest>({ user, browserId });
+				const res = mock<Response>();
+				userRepository.findOneByOrFail.mockResolvedValue(user);
+				userRepository.findOneOrFail.mockResolvedValue(user);
+				jest.spyOn(jwt, 'sign').mockImplementation(() => 'new-signed-token');
+				userService.toPublic.mockResolvedValue({} as unknown as PublicUser);
+
+				const result = await controller.updateCurrentUser(
+					req,
+					res,
+					mock({
+						email: 'michel-new@email.com',
+						firstName: 'Michel',
+						lastName: 'n8n',
+						currentPassword: oldPasswordPlain,
+					}),
+				);
+
+				expect(userService.update).toHaveBeenCalled();
+				expect(result).toEqual({});
+			});
+
+			it('should not require currentPassword when email is not being changed', async () => {
+				const user = mock<User>({
+					email: 'michel@email.com',
+					password: passwordHash,
+					mfaEnabled: false,
+				});
+				const req = mock<AuthenticatedRequest>({ user, browserId });
+				const res = mock<Response>();
+				userRepository.findOneByOrFail.mockResolvedValue(user);
+				userRepository.findOneOrFail.mockResolvedValue(user);
+				jest.spyOn(jwt, 'sign').mockImplementation(() => 'new-signed-token');
+				userService.toPublic.mockResolvedValue({} as unknown as PublicUser);
+
+				const result = await controller.updateCurrentUser(
+					req,
+					res,
+					new UserUpdateRequestDto({
+						email: 'michel@email.com',
+						firstName: 'Michel',
+						lastName: 'n8n',
+					}),
+				);
+
+				expect(userService.update).toHaveBeenCalled();
+				expect(result).toEqual({});
+			});
+		});
 	});
 
 	describe('updatePassword', () => {

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

@@ -11,6 +11,8 @@ import { Body, Patch, Post, RestController } from '@n8n/decorators';
 import { plainToInstance } from 'class-transformer';
 import { Response } from 'express';
 
+import { PersonalizationSurveyAnswersV4 } from './survey-answers.dto';
+
 import { AuthService } from '@/auth/auth.service';
 import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error';
@@ -28,7 +30,6 @@ import {
 	isOidcCurrentAuthenticationMethod,
 } from '@/sso.ee/sso-helpers';
 
-import { PersonalizationSurveyAnswersV4 } from './survey-answers.dto';
 @RestController('/me')
 export class MeController {
 	constructor(
@@ -54,11 +55,11 @@ export class MeController {
 		const {
 			id: userId,
 			email: currentEmail,
-			mfaEnabled,
 			firstName: currentFirstName,
 			lastName: currentLastName,
 		} = req.user;
 
+		const { currentPassword, ...payloadWithoutPassword } = payload;
 		const { email, firstName, lastName } = payload;
 		const isEmailBeingChanged = email !== currentEmail;
 		const isFirstNameChanged = firstName !== currentFirstName;
@@ -72,7 +73,7 @@ export class MeController {
 				`Request to update user failed because ${getCurrentAuthenticationMethod()} user may not change their profile information`,
 				{
 					userId,
-					payload,
+					payload: payloadWithoutPassword,
 				},
 			);
 			throw new BadRequestError(
@@ -80,33 +81,16 @@ export class MeController {
 			);
 		}
 
-		// If SAML is enabled, we don't allow the user to change their email address
-		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');
-		}
+		await this.validateChangingUserEmail(req.user, payload);
 
-		if (mfaEnabled && isEmailBeingChanged) {
-			if (!payload.mfaCode) {
-				throw new BadRequestError('Two-factor code is required to change email');
-			}
-
-			const isMfaCodeValid = await this.mfaService.validateMfa(userId, payload.mfaCode, undefined);
-			if (!isMfaCodeValid) {
-				throw new InvalidMfaCodeError();
-			}
-		}
-
-		await this.externalHooks.run('user.profile.beforeUpdate', [userId, currentEmail, payload]);
+		await this.externalHooks.run('user.profile.beforeUpdate', [
+			userId,
+			currentEmail,
+			payloadWithoutPassword,
+		]);
 
 		const preUpdateUser = await this.userRepository.findOneByOrFail({ id: userId });
-		await this.userService.update(userId, payload);
+		await this.userService.update(userId, payloadWithoutPassword);
 		const user = await this.userRepository.findOneOrFail({
 			where: { id: userId },
 			relations: ['role'],
@@ -130,6 +114,60 @@ export class MeController {
 		return publicUser;
 	}
 
+	private async validateChangingUserEmail(currentUser: User, payload: UserUpdateRequestDto) {
+		if (!payload.email || payload.email === currentUser.email) {
+			// email is not being changed
+			return;
+		}
+		const { currentPassword: providedCurrentPassword, ...payloadWithoutPassword } = payload;
+		const { id: userId, mfaEnabled } = currentUser;
+
+		// If SAML is enabled, we don't allow the user to change their email address
+		if (isSamlLicensedAndEnabled()) {
+			this.logger.debug(
+				'Request to update user failed because SAML user may not change their email',
+				{
+					userId: currentUser.id,
+					payload: payloadWithoutPassword,
+				},
+			);
+			throw new BadRequestError('SAML user may not change their email');
+		}
+
+		if (mfaEnabled) {
+			if (!payload.mfaCode) {
+				throw new BadRequestError('Two-factor code is required to change email');
+			}
+
+			const isMfaCodeValid = await this.mfaService.validateMfa(userId, payload.mfaCode, undefined);
+			if (!isMfaCodeValid) {
+				throw new InvalidMfaCodeError();
+			}
+		} else {
+			if (currentUser.password === null) {
+				this.logger.debug('User with no password changed their email', {
+					userId: currentUser.id,
+					payload: payloadWithoutPassword,
+				});
+				return;
+			}
+
+			if (!providedCurrentPassword || typeof providedCurrentPassword !== 'string') {
+				throw new BadRequestError('Current password is required to change email');
+			}
+
+			const isProvidedPasswordCorrect = await this.passwordUtility.compare(
+				providedCurrentPassword,
+				currentUser.password,
+			);
+			if (!isProvidedPasswordCorrect) {
+				throw new BadRequestError(
+					'Unable to update profile. Please check your credentials and try again.',
+				);
+			}
+		}
+	}
+
 	/**
 	 * Update the logged-in user's password.
 	 */

packages/cli/test/integration/me.api.test.ts

@@ -27,6 +27,9 @@ beforeEach(async () => {
 	});
 });
 
+const ownerPassword = randomValidPassword();
+const memberPassword = randomValidPassword();
+
 describe('Owner shell', () => {
 	let ownerShell: User;
 	let authOwnerShellAgent: SuperAgentTest;
@@ -132,7 +135,6 @@ describe('Owner shell', () => {
 });
 
 describe('Member', () => {
-	const memberPassword = randomValidPassword();
 	let member: User;
 	let authMemberAgent: SuperAgentTest;
 
@@ -146,7 +148,7 @@ describe('Member', () => {
 	});
 
 	test('PATCH /me should succeed with valid inputs', async () => {
-		for (const validPayload of VALID_PATCH_ME_PAYLOADS) {
+		for (const validPayload of getValidPatchMePayloads('member')) {
 			const response = await authMemberAgent.patch('/me').send(validPayload).expect(200);
 
 			const { id, email, firstName, lastName, personalizationAnswers, role, password, isPending } =
@@ -175,7 +177,7 @@ describe('Member', () => {
 	});
 
 	test('PATCH /me should fail with invalid inputs', async () => {
-		for (const invalidPayload of INVALID_PATCH_ME_PAYLOADS) {
+		for (const invalidPayload of getInvalidPatchMePayloads('member')) {
 			const response = await authMemberAgent.patch('/me').send(invalidPayload);
 			expect(response.statusCode).toBe(400);
 
@@ -192,6 +194,39 @@ describe('Member', () => {
 		}
 	});
 
+	test('PATCH /me should fail when changing email without currentPassword', async () => {
+		const payloadWithoutPassword = {
+			email: randomEmail(),
+			firstName: randomName(),
+			lastName: randomName(),
+		};
+
+		const response = await authMemberAgent.patch('/me').send(payloadWithoutPassword);
+		expect(response.statusCode).toBe(400);
+		expect(response.body.message).toContain('Current password is required to change email');
+
+		const storedMember = await Container.get(UserRepository).findOneByOrFail({});
+		expect(storedMember.email).toBe(member.email);
+	});
+
+	test('PATCH /me should fail when changing email with wrong currentPassword', async () => {
+		const payloadWithWrongPassword = {
+			email: randomEmail(),
+			firstName: randomName(),
+			lastName: randomName(),
+			currentPassword: 'WrongPassword123',
+		};
+
+		const response = await authMemberAgent.patch('/me').send(payloadWithWrongPassword);
+		expect(response.statusCode).toBe(400);
+		expect(response.body.message).toContain(
+			'Unable to update profile. Please check your credentials and try again.',
+		);
+
+		const storedMember = await Container.get(UserRepository).findOneByOrFail({});
+		expect(storedMember.email).toBe(member.email);
+	});
+
 	test('PATCH /me/password should succeed with valid inputs', async () => {
 		const validPayload = {
 			currentPassword: memberPassword,
@@ -243,10 +278,13 @@ describe('Member', () => {
 
 describe('Owner', () => {
 	test('PATCH /me should succeed with valid inputs', async () => {
-		const owner = await createUser({ role: GLOBAL_OWNER_ROLE });
+		const owner = await createUser({
+			role: GLOBAL_OWNER_ROLE,
+			password: ownerPassword,
+		});
 		const authOwnerAgent = testServer.authAgentFor(owner);
 
-		for (const validPayload of VALID_PATCH_ME_PAYLOADS) {
+		for (const validPayload of getValidPatchMePayloads('owner')) {
 			const response = await authOwnerAgent.patch('/me').send(validPayload);
 
 			expect(response.statusCode).toBe(200);
@@ -314,6 +352,24 @@ const EMPTY_SURVEY: IPersonalizationSurveyAnswersV4 = {
 	personalization_survey_n8n_version: '1.0.0',
 };
 
+function getValidPatchMePayloads(userType: 'owner' | 'member') {
+	return VALID_PATCH_ME_PAYLOADS.map((payload) => {
+		if (userType === 'owner') {
+			return { ...payload, currentPassword: ownerPassword };
+		}
+		return { ...payload, currentPassword: memberPassword };
+	});
+}
+
+function getInvalidPatchMePayloads(userType: 'owner' | 'member') {
+	return INVALID_PATCH_ME_PAYLOADS.map((payload) => {
+		if (userType === 'owner') {
+			return { ...payload, currentPassword: ownerPassword };
+		}
+		return { ...payload, currentPassword: memberPassword };
+	});
+}
+
 const VALID_PATCH_ME_PAYLOADS = [
 	{
 		email: randomEmail(),

packages/cli/test/integration/saml/saml.api.test.ts

@@ -50,10 +50,9 @@ describe('Instance owner', () => {
 			await authOwnerAgent
 				.patch('/me')
 				.send({
-					email: randomEmail(),
+					email: owner.email,
 					firstName: randomName(),
 					lastName: randomName(),
-					password: randomValidPassword(),
 				})
 				.expect(200);
 		});