fix: Require mfa code for password change if its enabled (#10341)

packages/cli/src/Mfa/mfa.service.ts

@@ -60,7 +60,9 @@ export class MfaService {
 		if (mfaToken) {
 			const decryptedSecret = this.cipher.decrypt(user.mfaSecret!);
 			return this.totp.verifySecret({ secret: decryptedSecret, token: mfaToken });
-		} else if (mfaRecoveryCode) {
+		}
+
+		if (mfaRecoveryCode) {
 			const validCodes = user.mfaRecoveryCodes.map((code) => this.cipher.decrypt(code));
 			const index = validCodes.indexOf(mfaRecoveryCode);
 			if (index === -1) return false;
@@ -70,6 +72,7 @@ export class MfaService {
 			await this.authUserRepository.save(user);
 			return true;
 		}
+
 		return false;
 	}
 

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

@@ -15,6 +15,9 @@ import { UserRepository } from '@/databases/repositories/user.repository';
 import { EventService } from '@/events/event.service';
 import { badPasswords } from '@test/testData';
 import { mockInstance } from '@test/mocking';
+import { AuthUserRepository } from '@/databases/repositories/authUser.repository';
+import { MfaService } from '@/Mfa/mfa.service';
+import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
 
 const browserId = 'test-browser-id';
 
@@ -23,6 +26,8 @@ describe('MeController', () => {
 	const eventService = mockInstance(EventService);
 	const userService = mockInstance(UserService);
 	const userRepository = mockInstance(UserRepository);
+	const mockMfaService = mockInstance(MfaService);
+	mockInstance(AuthUserRepository);
 	mockInstance(License).isWithinUsersLimit.mockReturnValue(true);
 	const controller = Container.get(MeController);
 
@@ -179,7 +184,7 @@ describe('MeController', () => {
 
 		it('should update the password in the DB, and issue a new cookie', async () => {
 			const req = mock<MeRequest.Password>({
-				user: mock({ password: passwordHash }),
+				user: mock({ password: passwordHash, mfaEnabled: false }),
 				body: { currentPassword: 'old_password', newPassword: 'NewPassword123' },
 				browserId,
 			});
@@ -212,6 +217,52 @@ describe('MeController', () => {
 				fieldsChanged: ['password'],
 			});
 		});
+
+		describe('mfa enabled', () => {
+			it('should throw BadRequestError if mfa code is missing', async () => {
+				const req = mock<MeRequest.Password>({
+					user: mock({ password: passwordHash, mfaEnabled: true }),
+					body: { currentPassword: 'old_password', newPassword: 'NewPassword123' },
+				});
+
+				await expect(controller.updatePassword(req, mock())).rejects.toThrowError(
+					new BadRequestError('Two-factor code is required to change password.'),
+				);
+			});
+
+			it('should throw ForbiddenError if invalid mfa code is given', async () => {
+				const req = mock<MeRequest.Password>({
+					user: mock({ password: passwordHash, mfaEnabled: true }),
+					body: { currentPassword: 'old_password', newPassword: 'NewPassword123', mfaCode: '123' },
+				});
+				mockMfaService.validateMfa.mockResolvedValue(false);
+
+				await expect(controller.updatePassword(req, mock())).rejects.toThrowError(
+					new ForbiddenError('Invalid two-factor code.'),
+				);
+			});
+
+			it('should succeed when mfa code is correct', async () => {
+				const req = mock<MeRequest.Password>({
+					user: mock({ password: passwordHash, mfaEnabled: true }),
+					body: {
+						currentPassword: 'old_password',
+						newPassword: 'NewPassword123',
+						mfaCode: 'valid',
+					},
+					browserId,
+				});
+				const res = mock<Response>();
+				userRepository.save.calledWith(req.user).mockResolvedValue(req.user);
+				jest.spyOn(jwt, 'sign').mockImplementation(() => 'new-signed-token');
+				mockMfaService.validateMfa.mockResolvedValue(true);
+
+				const result = await controller.updatePassword(req, res);
+
+				expect(result).toEqual({ success: true });
+				expect(req.user.password).not.toBe(passwordHash);
+			});
+		});
 	});
 
 	describe('storeSurveyAnswers', () => {

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

@@ -23,6 +23,8 @@ import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { UserRepository } from '@/databases/repositories/user.repository';
 import { isApiEnabled } from '@/PublicApi';
 import { EventService } from '@/events/event.service';
+import { MfaService } from '@/Mfa/mfa.service';
+import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
 
 export const API_KEY_PREFIX = 'n8n_api_';
 
@@ -44,6 +46,7 @@ export class MeController {
 		private readonly passwordUtility: PasswordUtility,
 		private readonly userRepository: UserRepository,
 		private readonly eventService: EventService,
+		private readonly mfaService: MfaService,
 	) {}
 
 	/**
@@ -115,12 +118,12 @@ export class MeController {
 	/**
 	 * Update the logged-in user's password.
 	 */
-	@Patch('/password')
+	@Patch('/password', { rateLimit: true })
 	async updatePassword(req: MeRequest.Password, res: Response) {
 		const { user } = req;
-		const { currentPassword, newPassword } = req.body;
+		const { currentPassword, newPassword, mfaCode } = req.body;
 
-		// If SAML is enabled, we don't allow the user to change their email address
+		// If SAML is enabled, we don't allow the user to change their password
 		if (isSamlLicensedAndEnabled()) {
 			this.logger.debug('Attempted to change password for user, while SAML is enabled', {
 				userId: user.id,
@@ -145,6 +148,17 @@ export class MeController {
 
 		const validPassword = this.passwordUtility.validate(newPassword);
 
+		if (user.mfaEnabled) {
+			if (typeof mfaCode !== 'string') {
+				throw new BadRequestError('Two-factor code is required to change password.');
+			}
+
+			const isMfaTokenValid = await this.mfaService.validateMfa(user.id, mfaCode, undefined);
+			if (!isMfaTokenValid) {
+				throw new ForbiddenError('Invalid two-factor code.');
+			}
+		}
+
 		user.password = await this.passwordUtility.hash(validPassword);
 
 		const updatedUser = await this.userRepository.save(user, { transaction: false });

packages/cli/src/requests.ts

@@ -224,7 +224,7 @@ export declare namespace MeRequest {
 	export type Password = AuthenticatedRequest<
 		{},
 		{},
-		{ currentPassword: string; newPassword: string; token?: string }
+		{ currentPassword: string; newPassword: string; mfaCode?: string }
 	>;
 	export type SurveyAnswers = AuthenticatedRequest<{}, {}, Record<string, string> | {}>;
 }

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

@@ -116,9 +116,15 @@ export async function updateOtherUserSettings(
 	return await makeRestApiRequest(context, 'PATCH', `/users/${userId}/settings`, settings);
 }
 
+export type UpdateUserPasswordParams = {
+	newPassword: string;
+	currentPassword: string;
+	mfaCode?: string;
+};
+
 export async function updateCurrentUserPassword(
 	context: IRestApiContext,
-	params: { newPassword: string; currentPassword: string },
+	params: UpdateUserPasswordParams,
 ): Promise<void> {
 	return await makeRestApiRequest(context, 'PATCH', '/me/password', params);
 }

packages/editor-ui/src/components/ChangePasswordModal.vue

@@ -35,7 +35,7 @@ import { CHANGE_PASSWORD_MODAL_KEY } from '../constants';
 import Modal from '@/components/Modal.vue';
 import { useUsersStore } from '@/stores/users.store';
 import { createEventBus } from 'n8n-design-system/utils';
-import type { IFormInputs } from '@/Interface';
+import type { IFormInputs, IFormInput } from '@/Interface';
 import { useI18n } from '@/composables/useI18n';
 
 const config = ref<IFormInputs | null>(null);
@@ -68,10 +68,18 @@ const onInput = (e: { name: string; value: string }) => {
 	}
 };
 
-const onSubmit = async (values: { currentPassword: string; password: string }) => {
+const onSubmit = async (values: {
+	currentPassword: string;
+	password: string;
+	mfaCode?: string;
+}) => {
 	try {
 		loading.value = true;
-		await usersStore.updateCurrentUserPassword(values);
+		await usersStore.updateCurrentUserPassword({
+			currentPassword: values.currentPassword,
+			newPassword: values.password,
+			mfaCode: values.mfaCode,
+		});
 
 		showMessage({
 			type: 'success',
@@ -92,8 +100,8 @@ const onSubmitClick = () => {
 };
 
 onMounted(() => {
-	const form: IFormInputs = [
-		{
+	const inputs: Record<string, IFormInput> = {
+		currentPassword: {
 			name: 'currentPassword',
 			properties: {
 				label: i18n.baseText('auth.changePassword.currentPassword'),
@@ -104,7 +112,16 @@ onMounted(() => {
 				focusInitially: true,
 			},
 		},
-		{
+		mfaCode: {
+			name: 'mfaCode',
+			properties: {
+				label: i18n.baseText('auth.changePassword.mfaCode'),
+				type: 'text',
+				required: true,
+				capitalize: true,
+			},
+		},
+		newPassword: {
 			name: 'password',
 			properties: {
 				label: i18n.baseText('auth.newPassword'),
@@ -116,7 +133,7 @@ onMounted(() => {
 				capitalize: true,
 			},
 		},
-		{
+		newPasswordAgain: {
 			name: 'password2',
 			properties: {
 				label: i18n.baseText('auth.changePassword.reenterNewPassword'),
@@ -132,7 +149,13 @@ onMounted(() => {
 				capitalize: true,
 			},
 		},
-	];
+	};
+
+	const { currentUser } = usersStore;
+
+	const form: IFormInputs = currentUser?.mfaEnabled
+		? [inputs.currentPassword, inputs.mfaCode, inputs.newPassword, inputs.newPasswordAgain]
+		: [inputs.currentPassword, inputs.newPassword, inputs.newPasswordAgain];
 
 	config.value = form;
 });

packages/editor-ui/src/components/__tests__/ChangePasswordModal.test.ts

@@ -0,0 +1,20 @@
+import { createTestingPinia } from '@pinia/testing';
+import ChangePasswordModal from '@/components/ChangePasswordModal.vue';
+import type { createPinia } from 'pinia';
+import { createComponentRenderer } from '@/__tests__/render';
+
+const renderComponent = createComponentRenderer(ChangePasswordModal);
+
+describe('ChangePasswordModal', () => {
+	let pinia: ReturnType<typeof createPinia>;
+
+	beforeEach(() => {
+		pinia = createTestingPinia({});
+	});
+
+	it('should render correctly', () => {
+		const wrapper = renderComponent({ pinia });
+
+		expect(wrapper.html()).toMatchSnapshot();
+	});
+});

packages/editor-ui/src/components/__tests__/__snapshots__/ChangePasswordModal.test.ts.snap

@@ -0,0 +1,6 @@
+// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
+
+exports[`ChangePasswordModal > should render correctly 1`] = `
+"<!--teleport start-->
+<!--teleport end-->"
+`;

packages/editor-ui/src/plugins/i18n/locales/en.json

@@ -98,6 +98,7 @@
 	"activationModal.yourWorkflowWillNowRegularlyCheck": "Your workflow will now regularly check {serviceName} for events and trigger executions for them.",
 	"auth.changePassword": "Change password",
 	"auth.changePassword.currentPassword": "Current password",
+	"auth.changePassword.mfaCode": "Two-factor code",
 	"auth.changePassword.error": "Problem changing the password",
 	"auth.changePassword.missingTokenError": "Missing token",
 	"auth.changePassword.missingUserIdError": "Missing user ID",

packages/editor-ui/src/stores/users.store.ts

@@ -258,17 +258,8 @@ export const useUsersStore = defineStore(STORES.USERS, () => {
 		addUsers([usersById.value[userId]]);
 	};
 
-	const updateCurrentUserPassword = async ({
-		password,
-		currentPassword,
-	}: {
-		password: string;
-		currentPassword: string;
-	}) => {
-		await usersApi.updateCurrentUserPassword(rootStore.restApiContext, {
-			newPassword: password,
-			currentPassword,
-		});
+	const updateCurrentUserPassword = async (params: usersApi.UpdateUserPasswordParams) => {
+		await usersApi.updateCurrentUserPassword(rootStore.restApiContext, params);
 	};
 
 	const deleteUser = async (params: { id: string; transferId?: string }) => {