diff --git a/packages/cli/package.json b/packages/cli/package.json index 6c7f275e800d9..df23a55365583 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -129,6 +129,7 @@ "callsites": "^3.1.0", "change-case": "^4.1.1", "class-validator": "^0.14.0", + "class-transformer": "^0.5.1", "client-oauth2": "^4.2.5", "compression": "^1.7.4", "connect-history-api-fallback": "^1.6.0", diff --git a/packages/cli/src/GenericHelpers.ts b/packages/cli/src/GenericHelpers.ts index 7d8011abdec59..c33e4dff88cc5 100644 --- a/packages/cli/src/GenericHelpers.ts +++ b/packages/cli/src/GenericHelpers.ts @@ -22,6 +22,7 @@ import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; import type { TagEntity } from '@db/entities/TagEntity'; import type { User } from '@db/entities/User'; +import type { UserUpdatePayload } from '@/requests'; /** * Returns the base URL n8n is reachable from @@ -99,7 +100,7 @@ export async function generateUniqueName( } export async function validateEntity( - entity: WorkflowEntity | CredentialsEntity | TagEntity | User, + entity: WorkflowEntity | CredentialsEntity | TagEntity | User | UserUpdatePayload, ): Promise { const errors = await validate(entity); diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 463716b5f73e4..9c5fb6cfcfd4b 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -1,4 +1,5 @@ import validator from 'validator'; +import { plainToInstance } from 'class-transformer'; import { Delete, Get, Patch, Post, RestController } from '@/decorators'; import { compareHash, @@ -13,7 +14,7 @@ import { issueCookie } from '@/auth/jwt'; import { Response } from 'express'; import type { Repository } from 'typeorm'; import type { ILogger } from 'n8n-workflow'; -import { AuthenticatedRequest, MeRequest } from '@/requests'; +import { AuthenticatedRequest, MeRequest, UserUpdatePayload } from '@/requests'; import type { PublicUser, IDatabaseCollections, @@ -21,6 +22,7 @@ import type { IInternalHooksClass, } from '@/Interfaces'; import { randomBytes } from 'crypto'; +import pick from 'lodash.pick'; @RestController('/me') export class MeController { @@ -53,38 +55,44 @@ export class MeController { * Update the logged-in user's settings, except password. */ @Patch('/') - async updateCurrentUser(req: MeRequest.Settings, res: Response): Promise { - const { email } = req.body; + async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise { + const { id: userId, email: currentEmail } = req.user; + + const payload = plainToInstance( + UserUpdatePayload, + pick(req.body, 'email', 'firstName', 'lastName'), + ); + + const { email } = payload; if (!email) { this.logger.debug('Request to update user email failed because of missing email in payload', { - userId: req.user.id, - payload: req.body, + userId, + payload, }); throw new BadRequestError('Email is mandatory'); } if (!validator.isEmail(email)) { this.logger.debug('Request to update user email failed because of invalid email in payload', { - userId: req.user.id, + userId, invalidEmail: email, }); throw new BadRequestError('Invalid email address'); } - const { email: currentEmail } = req.user; - const newUser = new User(); - - Object.assign(newUser, req.user, req.body); + await validateEntity(payload); - await validateEntity(newUser); - - const user = await this.userRepository.save(newUser); + await this.userRepository.update(userId, payload); + const user = await this.userRepository.findOneOrFail({ + where: { id: userId }, + relations: { globalRole: true }, + }); - this.logger.info('User updated successfully', { userId: user.id }); + this.logger.info('User updated successfully', { userId }); await issueCookie(res, user); - const updatedKeys = Object.keys(req.body); + const updatedKeys = Object.keys(payload); void this.internalHooks.onUserUpdate({ user, fields_changed: updatedKeys, diff --git a/packages/cli/src/databases/entities/User.ts b/packages/cli/src/databases/entities/User.ts index 6cba438f7942a..d62bf8482fc26 100644 --- a/packages/cli/src/databases/entities/User.ts +++ b/packages/cli/src/databases/entities/User.ts @@ -111,6 +111,9 @@ export class User extends AbstractEntity implements IUser { @AfterLoad() @AfterUpdate() computeIsPending(): void { - this.isPending = this.password === null; + this.isPending = + this.globalRole?.name === 'owner' && this.globalRole.scope === 'global' + ? false + : this.password === null; } } diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 0a1be41429b3b..f8c5af17b4a84 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -10,11 +10,28 @@ import type { IWorkflowSettings, } from 'n8n-workflow'; +import { IsEmail, IsString, Length } from 'class-validator'; +import { NoXss } from '@db/utils/customValidators'; import type { PublicUser, IExecutionDeleteFilter, IWorkflowDb } from '@/Interfaces'; import type { Role } from '@db/entities/Role'; import type { User } from '@db/entities/User'; import type * as UserManagementMailer from '@/UserManagement/email/UserManagementMailer'; +export class UserUpdatePayload implements Pick { + @IsEmail() + email: string; + + @NoXss() + @IsString({ message: 'First name must be of type string.' }) + @Length(1, 32, { message: 'First name must be $constraint1 to $constraint2 characters long.' }) + firstName: string; + + @NoXss() + @IsString({ message: 'Last name must be of type string.' }) + @Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' }) + lastName: string; +} + export type AuthlessRequest< RouteParams = {}, ResponseBody = {}, @@ -144,11 +161,7 @@ export declare namespace ExecutionRequest { // ---------------------------------- export declare namespace MeRequest { - export type Settings = AuthenticatedRequest< - {}, - {}, - Pick - >; + export type UserUpdate = AuthenticatedRequest<{}, {}, UserUpdatePayload>; export type Password = AuthenticatedRequest< {}, {}, diff --git a/packages/cli/test/unit/controllers/me.controller.test.ts b/packages/cli/test/unit/controllers/me.controller.test.ts index f926c4fc4fe87..2c560bca69d92 100644 --- a/packages/cli/test/unit/controllers/me.controller.test.ts +++ b/packages/cli/test/unit/controllers/me.controller.test.ts @@ -25,40 +25,74 @@ describe('MeController', () => { describe('updateCurrentUser', () => { it('should throw BadRequestError if email is missing in the payload', async () => { - const req = mock({}); + const req = mock({}); expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError( new BadRequestError('Email is mandatory'), ); }); it('should throw BadRequestError if email is invalid', async () => { - const req = mock({ body: { email: 'invalid-email' } }); + const req = mock({ body: { email: 'invalid-email' } }); expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError( new BadRequestError('Invalid email address'), ); }); it('should update the user in the DB, and issue a new cookie', async () => { - const req = mock({ - user: mock({ id: '123', password: 'password', authIdentities: [] }), - body: { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }, + const user = mock({ + id: '123', + password: 'password', + authIdentities: [], + globalRoleId: '1', }); + const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; + const req = mock({ user, body: reqBody }); const res = mock(); - userRepository.save.calledWith(anyObject()).mockResolvedValue(req.user); + userRepository.findOneByOrFail.mockResolvedValue(user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); await controller.updateCurrentUser(req, res); + expect(userRepository.update).toHaveBeenCalled(); + const cookieOptions = captor(); expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, 'signed-token', cookieOptions); expect(cookieOptions.value.httpOnly).toBe(true); expect(cookieOptions.value.sameSite).toBe('lax'); expect(externalHooks.run).toHaveBeenCalledWith('user.profile.update', [ - req.user.email, + user.email, anyObject(), ]); }); + + it('should not allow updating any other fields on a user besides email and name', async () => { + const user = mock({ + id: '123', + password: 'password', + authIdentities: [], + globalRoleId: '1', + }); + const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; + const req = mock({ user, body: reqBody }); + const res = mock(); + userRepository.findOneByOrFail.mockResolvedValue(user); + jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); + + // Add invalid data to the request payload + Object.assign(reqBody, { id: '0', globalRoleId: '42' }); + + await controller.updateCurrentUser(req, res); + + expect(userRepository.update).toHaveBeenCalled(); + + const updatedUser = userRepository.update.mock.calls[0][1]; + expect(updatedUser.email).toBe(reqBody.email); + expect(updatedUser.firstName).toBe(reqBody.firstName); + expect(updatedUser.lastName).toBe(reqBody.lastName); + expect(updatedUser.id).not.toBe('0'); + expect(updatedUser.globalRoleId).not.toBe('42'); + }); }); describe('updatePassword', () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 055fb2d89c4d0..9e45e143d5363 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -163,6 +163,7 @@ importers: callsites: ^3.1.0 change-case: ^4.1.1 chokidar: 3.5.2 + class-transformer: ^0.5.1 class-validator: ^0.14.0 client-oauth2: ^4.2.5 compression: ^1.7.4 @@ -259,6 +260,7 @@ importers: bull: 4.10.2 callsites: 3.1.0 change-case: 4.1.2 + class-transformer: 0.5.1 class-validator: 0.14.0 client-oauth2: 4.3.3 compression: 1.7.4 @@ -4138,7 +4140,7 @@ packages: dependencies: '@storybook/client-logger': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/manager-api': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/preview-api': 7.0.0-beta.46 @@ -4346,7 +4348,7 @@ packages: '@storybook/client-logger': 7.0.0-beta.46 '@storybook/components': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/docs-tools': 7.0.0-beta.46 '@storybook/global': 5.0.0 '@storybook/manager-api': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe @@ -4566,7 +4568,7 @@ packages: '@babel/core': 7.20.12 '@babel/preset-env': 7.20.2_@babel+core@7.20.12 '@babel/types': 7.20.7 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/csf-tools': 7.0.0-beta.46 '@storybook/node-logger': 7.0.0-beta.46 '@storybook/types': 7.0.0-beta.46 @@ -4606,7 +4608,7 @@ packages: react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0 dependencies: '@storybook/client-logger': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/theming': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/types': 7.0.0-beta.46 @@ -4677,7 +4679,7 @@ packages: '@storybook/builder-manager': 7.0.0-beta.46 '@storybook/core-common': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/csf-tools': 7.0.0-beta.46 '@storybook/docs-mdx': 0.0.1-next.6 '@storybook/global': 5.0.0 @@ -4747,7 +4749,7 @@ packages: resolution: {integrity: sha512-H7zXfL1wf/1jWi5MaFISt/taxE41fgpV/uLfi5CHcHLX9ZgeQs2B/2utpUgwvBsxiL+E/jKAt5cLeuZCIvglMg==} dependencies: '@babel/types': 7.20.7 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/types': 7.0.0-beta.46 fs-extra: 11.1.0 recast: 0.23.1 @@ -4762,8 +4764,8 @@ packages: lodash: 4.17.21 dev: true - /@storybook/csf/0.0.2-next.9: - resolution: {integrity: sha512-ECOLMK425s+z8oA0aVAhBhhquuwTsZrM4oha/5De44JG8uYGXhqVrv/l27oxZEkwytuiQu+9f65HxYli+DY+3w==} + /@storybook/csf/0.0.2-next.10: + resolution: {integrity: sha512-m2PFgBP/xRIF85VrDhvesn9ktaD2pN3VUjvMqkAL/cINp/3qXsCyI81uw7N5VEOkQAbWrY2FcydnvEPDEdE8fA==} dependencies: type-fest: 2.19.0 dev: true @@ -4799,7 +4801,7 @@ packages: '@storybook/channels': 7.0.0-beta.46 '@storybook/client-logger': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/router': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/theming': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe @@ -4889,7 +4891,7 @@ packages: '@storybook/channels': 7.0.0-beta.46 '@storybook/client-logger': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/types': 7.0.0-beta.46 '@types/qs': 6.9.7 @@ -8090,6 +8092,10 @@ packages: resolution: {integrity: sha512-kgMuFyE78OC6Dyu3Dy7vcx4uy97EIbVxJB/B0eJ3bUNAkwdNcxYzgKltnyADiYwsR7SEqkkUPsEUT//OVS6XMA==} dev: false + /class-transformer/0.5.1: + resolution: {integrity: sha512-SQa1Ws6hUbfC98vKGxZH3KFY0Y1lm5Zm0SY8XX9zbK7FJCyVEac3ATW0RIpwzW+oOfmHE5PMPufDG9hCfoEOMw==} + dev: false + /class-utils/0.3.6: resolution: {integrity: sha512-qOhPa/Fj7s6TY8H8esGu5QNpMMQxz79h+urzrNYN6mn+9BnxlDGf5QZ+XeCDsxSjPqsSR56XOZOJmpeurnLMeg==} engines: {node: '>=0.10.0'}