diff --git a/cypress/e2e/18-user-management.cy.ts b/cypress/e2e/18-user-management.cy.ts index 710a453b41..5fd763207a 100644 --- a/cypress/e2e/18-user-management.cy.ts +++ b/cypress/e2e/18-user-management.cy.ts @@ -236,7 +236,10 @@ describe('User Management', { disableAutoLogin: true }, () => { INSTANCE_OWNER.email, updatedPersonalData.newPassword, ); - personalSettingsPage.actions.updateEmail(updatedPersonalData.newEmail); + personalSettingsPage.actions.updateEmail( + updatedPersonalData.newEmail, + updatedPersonalData.newPassword, + ); successToast().should('contain', 'Personal details updated'); personalSettingsPage.actions.loginWithNewData( updatedPersonalData.newEmail, diff --git a/cypress/pages/settings-personal.ts b/cypress/pages/settings-personal.ts index 49c7de1283..681a89b461 100644 --- a/cypress/pages/settings-personal.ts +++ b/cypress/pages/settings-personal.ts @@ -25,6 +25,7 @@ export class PersonalSettingsPage extends BasePage { firstNameInput: () => cy.getByTestId('firstName').find('input').first(), lastNameInput: () => cy.getByTestId('lastName').find('input').first(), emailInputContainer: () => cy.getByTestId('email'), + currentPasswordConfirmationInput: () => cy.getByTestId('currentPassword').find('input'), emailInput: () => cy.getByTestId('email').find('input').first(), changePasswordLink: () => cy.getByTestId('change-password-link').first(), saveSettingsButton: () => cy.getByTestId('save-settings-button'), @@ -66,8 +67,14 @@ export class PersonalSettingsPage extends BasePage { .find('div[class^="_errorInput"]') .should('exist'); }, - updateEmail: (newEmail: string) => { + /** + * @param currentPassword only required if MFA is disabled + */ + updateEmail: (newEmail: string, currentPassword?: string) => { this.getters.emailInput().type('{selectall}').type(newEmail).type('{enter}'); + if (currentPassword) { + this.getters.currentPasswordConfirmationInput().type(currentPassword).type('{enter}'); + } }, tryToSetInvalidEmail: (newEmail: string) => { this.actions.updateEmail(newEmail); diff --git a/packages/@n8n/api-types/src/dto/user/user-update-request.dto.ts b/packages/@n8n/api-types/src/dto/user/user-update-request.dto.ts index ad70071d81..f7fe30f649 100644 --- a/packages/@n8n/api-types/src/dto/user/user-update-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/user/user-update-request.dto.ts @@ -28,4 +28,8 @@ export class UserUpdateRequestDto extends Z.class({ firstName: nameSchema().optional(), lastName: nameSchema().optional(), mfaCode: z.string().optional(), + /** + * The current password is required when changing the email address and MFA is disabled. + */ + currentPassword: z.string().optional(), }) {} diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/packages/cli/src/controllers/__tests__/me.controller.test.ts index 95c56af9e7..b16576a83a 100644 --- a/packages/cli/src/controllers/__tests__/me.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/me.controller.test.ts @@ -87,6 +87,7 @@ describe('MeController', () => { const user = mock({ 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({ + id: '123', + email: 'michel-old@email.com', + password: passwordHash, + mfaEnabled: false, + }); + const req = mock({ 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({ + id: '123', + email: 'michel-old@email.com', + password: passwordHash, + mfaEnabled: false, + }); + const req = mock({ 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({ + email: 'michel-old@email.com', + password: passwordHash, + mfaEnabled: false, + }); + const req = mock({ 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({ + email: 'michel-old@email.com', + password: passwordHash, + mfaEnabled: false, + }); + const req = mock({ user, browserId }); + const res = mock(); + 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({ + email: 'michel@email.com', + password: passwordHash, + mfaEnabled: false, + }); + const req = mock({ user, browserId }); + const res = mock(); + 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', () => { diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 568204a1c1..a30d700a30 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/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. */ diff --git a/packages/cli/test/integration/me.api.test.ts b/packages/cli/test/integration/me.api.test.ts index 7b9ebc8a77..469ddb80d5 100644 --- a/packages/cli/test/integration/me.api.test.ts +++ b/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(), diff --git a/packages/cli/test/integration/saml/saml.api.test.ts b/packages/cli/test/integration/saml/saml.api.test.ts index dc5e3e74d3..cb40b1ae3f 100644 --- a/packages/cli/test/integration/saml/saml.api.test.ts +++ b/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); }); diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index a1caae7c79..4dbce6c2b0 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -156,6 +156,9 @@ "auth.changePassword.passwordsMustMatchError": "Passwords must match", "auth.changePassword.reenterNewPassword": "Re-enter new password", "auth.changePassword.tokenValidationError": "Invalid password-reset token", + "auth.confirmPassword": "Confirm password", + "auth.confirmPassword.currentPassword": "Current password", + "auth.confirmPassword.confirmPasswordToChangeEmail": "Please confirm your password in order to change your email address.", "auth.defaultPasswordRequirements": "8+ characters, at least 1 number and 1 capital letter", "auth.validation.missingParameters": "Missing token or user id", "auth.email": "Email", diff --git a/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/ConfirmPasswordModal.test.ts b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/ConfirmPasswordModal.test.ts new file mode 100644 index 0000000000..1b8ce3b8da --- /dev/null +++ b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/ConfirmPasswordModal.test.ts @@ -0,0 +1,87 @@ +import { createTestingPinia } from '@pinia/testing'; +import ConfirmPasswordModal from '@/components/ConfirmPasswordModal/ConfirmPasswordModal.vue'; +import type { createPinia } from 'pinia'; +import { createComponentRenderer } from '@/__tests__/render'; +import userEvent from '@testing-library/user-event'; +import { waitFor } from '@testing-library/vue'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { CONFIRM_PASSWORD_MODAL_KEY } from '@/constants'; +import { confirmPasswordEventBus } from './confirm-password.event-bus'; +import { STORES } from '@n8n/stores'; + +const renderModal = createComponentRenderer(ConfirmPasswordModal); + +const ModalStub = { + template: ` +
+ + + + +
+ `, +}; + +const initialState = { + [STORES.UI]: { + modalsById: { + [CONFIRM_PASSWORD_MODAL_KEY]: { + open: true, + }, + }, + modalStack: [CONFIRM_PASSWORD_MODAL_KEY], + }, +}; + +const global = { + stubs: { + Modal: ModalStub, + }, +}; + +describe('ConfirmPasswordModal', () => { + let pinia: ReturnType; + + beforeEach(() => { + pinia = createTestingPinia({ initialState }); + }); + + it('should render correctly', () => { + const wrapper = renderModal({ pinia }); + + expect(wrapper.html()).toMatchSnapshot(); + }); + + it('should emit password entered by the user when submitting form', async () => { + const eventBusSpy = vi.spyOn(confirmPasswordEventBus, 'emit'); + + const { getByTestId } = renderModal({ + global, + pinia, + }); + + // Wait for the onMounted hook to complete and form inputs to render + const input = await waitFor(() => getByTestId('currentPassword').querySelector('input')!); + + await userEvent.clear(input); + await userEvent.type(input, 'testpassword123'); + + await userEvent.click(getByTestId('confirm-password-button')); + + expect(eventBusSpy).toHaveBeenCalledWith('close', { + currentPassword: 'testpassword123', + }); + }); + + it('should not submit form when password is empty', async () => { + const { getByTestId } = renderModal({ + global, + pinia, + }); + const eventBusSpy = vi.spyOn(confirmPasswordEventBus, 'emit'); + + await userEvent.click(getByTestId('confirm-password-button')); + + expect(eventBusSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/ConfirmPasswordModal.vue b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/ConfirmPasswordModal.vue new file mode 100644 index 0000000000..dd8393b09c --- /dev/null +++ b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/ConfirmPasswordModal.vue @@ -0,0 +1,91 @@ + + + + diff --git a/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/__snapshots__/ConfirmPasswordModal.test.ts.snap b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/__snapshots__/ConfirmPasswordModal.test.ts.snap new file mode 100644 index 0000000000..cbbfb17b72 --- /dev/null +++ b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/__snapshots__/ConfirmPasswordModal.test.ts.snap @@ -0,0 +1,6 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`ConfirmPasswordModal > should render correctly 1`] = ` +" +" +`; diff --git a/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/confirm-password.event-bus.ts b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/confirm-password.event-bus.ts new file mode 100644 index 0000000000..5866028ee3 --- /dev/null +++ b/packages/frontend/editor-ui/src/components/ConfirmPasswordModal/confirm-password.event-bus.ts @@ -0,0 +1,11 @@ +import { createEventBus } from '@n8n/utils/event-bus'; + +export interface ConfirmPasswordClosedEventPayload { + currentPassword: string; +} + +export interface ConfirmPasswordModalEvents { + close: ConfirmPasswordClosedEventPayload | undefined; +} + +export const confirmPasswordEventBus = createEventBus(); diff --git a/packages/frontend/editor-ui/src/components/Modals.vue b/packages/frontend/editor-ui/src/components/Modals.vue index c7eea0d061..1ed89c84e5 100644 --- a/packages/frontend/editor-ui/src/components/Modals.vue +++ b/packages/frontend/editor-ui/src/components/Modals.vue @@ -43,6 +43,7 @@ import { WORKFLOW_SETTINGS_MODAL_KEY, WORKFLOW_SHARE_MODAL_KEY, EXPERIMENT_TEMPLATE_RECO_V2_KEY, + CONFIRM_PASSWORD_MODAL_KEY, } from '@/constants'; import AboutModal from '@/components/AboutModal.vue'; @@ -50,6 +51,7 @@ import ActivationModal from '@/components/ActivationModal.vue'; import ApiKeyCreateOrEditModal from '@/components/ApiKeyCreateOrEditModal.vue'; import NewAssistantSessionModal from '@/components/AskAssistant/Chat/NewAssistantSessionModal.vue'; import ChangePasswordModal from '@/components/ChangePasswordModal.vue'; +import ConfirmPasswordModal from '@/components/ConfirmPasswordModal/ConfirmPasswordModal.vue'; import ChatEmbedModal from '@/components/ChatEmbedModal.vue'; import CommunityPackageInstallModal from '@/components/CommunityPackageInstallModal.vue'; import CommunityPackageManageConfirmModal from '@/components/CommunityPackageManageConfirmModal.vue'; @@ -168,6 +170,10 @@ import NodeRecommendationModal from '@/experiments/templateRecoV2/components/Nod + + + +