diff --git a/packages/cli/src/Mfa/mfa.service.ts b/packages/cli/src/Mfa/mfa.service.ts index 91270cf4ab..43b1d3438b 100644 --- a/packages/cli/src/Mfa/mfa.service.ts +++ b/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; } diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/packages/cli/src/controllers/__tests__/me.controller.test.ts index bfde0bcc0e..df22c377bc 100644 --- a/packages/cli/src/controllers/__tests__/me.controller.test.ts +++ b/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({ - 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({ + 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({ + 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({ + user: mock({ password: passwordHash, mfaEnabled: true }), + body: { + currentPassword: 'old_password', + newPassword: 'NewPassword123', + mfaCode: 'valid', + }, + browserId, + }); + const res = mock(); + 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', () => { diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index bbdac4bf02..4089cd3523 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/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 }); diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 5f5daa589e..5301351c77 100644 --- a/packages/cli/src/requests.ts +++ b/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 | {}>; } diff --git a/packages/editor-ui/src/api/users.ts b/packages/editor-ui/src/api/users.ts index 0085baadc1..bb730c671c 100644 --- a/packages/editor-ui/src/api/users.ts +++ b/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 { return await makeRestApiRequest(context, 'PATCH', '/me/password', params); } diff --git a/packages/editor-ui/src/components/ChangePasswordModal.vue b/packages/editor-ui/src/components/ChangePasswordModal.vue index 493a77e165..24de3b5dfb 100644 --- a/packages/editor-ui/src/components/ChangePasswordModal.vue +++ b/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(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 = { + 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; }); diff --git a/packages/editor-ui/src/components/__tests__/ChangePasswordModal.test.ts b/packages/editor-ui/src/components/__tests__/ChangePasswordModal.test.ts new file mode 100644 index 0000000000..7248774c81 --- /dev/null +++ b/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; + + beforeEach(() => { + pinia = createTestingPinia({}); + }); + + it('should render correctly', () => { + const wrapper = renderComponent({ pinia }); + + expect(wrapper.html()).toMatchSnapshot(); + }); +}); diff --git a/packages/editor-ui/src/components/__tests__/__snapshots__/ChangePasswordModal.test.ts.snap b/packages/editor-ui/src/components/__tests__/__snapshots__/ChangePasswordModal.test.ts.snap new file mode 100644 index 0000000000..db656032b2 --- /dev/null +++ b/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`] = ` +" +" +`; diff --git a/packages/editor-ui/src/plugins/i18n/locales/en.json b/packages/editor-ui/src/plugins/i18n/locales/en.json index 772f4c2d58..48081770ad 100644 --- a/packages/editor-ui/src/plugins/i18n/locales/en.json +++ b/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", diff --git a/packages/editor-ui/src/stores/users.store.ts b/packages/editor-ui/src/stores/users.store.ts index 0c6963819c..d0145b2cb0 100644 --- a/packages/editor-ui/src/stores/users.store.ts +++ b/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 }) => {