fix(core): User update endpoint should only allow updating email, firstName, and lastName (#5526)

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",

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<void> {
 	const errors = await validate(entity);
 

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,
@@ -7,13 +8,13 @@ import {
 	validatePassword,
 } from '@/UserManagement/UserManagementHelper';
 import { BadRequestError } from '@/ResponseHelper';
-import { User } from '@db/entities/User';
+import type { User } from '@db/entities/User';
 import { validateEntity } from '@/GenericHelpers';
 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,
@@ -53,38 +54,40 @@ export class MeController {
 	 * Update the logged-in user's settings, except password.
 	 */
 	@Patch('/')
-	async updateCurrentUser(req: MeRequest.Settings, res: Response): Promise<PublicUser> {
+	async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise<PublicUser> {
-		const { email } = req.body;
+		const { id: userId, email: currentEmail } = req.user;
+		const payload = plainToInstance(UserUpdatePayload, req.body);
+
+		const { email } = payload;
 		if (!email) {
 			this.logger.debug('Request to update user email failed because of missing email in payload', {
-				userId: req.user.id,
+				userId,
-				payload: req.body,
+				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;
+		await validateEntity(payload);
-		const newUser = new User();
 
-		Object.assign(newUser, req.user, req.body);
+		await this.userRepository.update(userId, payload);
+		const user = await this.userRepository.findOneOrFail({
+			where: { id: userId },
+			relations: { globalRole: true },
+		});
 
-		await validateEntity(newUser);
+		this.logger.info('User updated successfully', { userId });
-
-		const user = await this.userRepository.save(newUser);
-
-		this.logger.info('User updated successfully', { userId: user.id });
 
 		await issueCookie(res, user);
 
-		const updatedKeys = Object.keys(req.body);
+		const updatedKeys = Object.keys(payload);
 		void this.internalHooks.onUserUpdate({
 			user,
 			fields_changed: updatedKeys,

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;
 	}
 }

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<User, 'email' | 'firstName' | 'lastName'> {
+	@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<
+	export type UserUpdate = AuthenticatedRequest<{}, {}, UserUpdatePayload>;
-		{},
-		{},
-		Pick<PublicUser, 'email' | 'firstName' | 'lastName'>
-	>;
 	export type Password = AuthenticatedRequest<
 		{},
 		{},

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<MeRequest.Settings>({});
+			const req = mock<MeRequest.UserUpdate>({});
 			expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError(
 				new BadRequestError('Email is mandatory'),
 			);
 		});
 
 		it('should throw BadRequestError if email is invalid', async () => {
-			const req = mock<MeRequest.Settings>({ body: { email: 'invalid-email' } });
+			const req = mock<MeRequest.UserUpdate>({ 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<MeRequest.Settings>({
+			const user = mock<User>({
-				user: mock({ id: '123', password: 'password', authIdentities: [] }),
+				id: '123',
-				body: { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' },
+				password: 'password',
+				authIdentities: [],
+				globalRoleId: '1',
 			});
+			const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' };
+			const req = mock<MeRequest.UserUpdate>({ user, body: reqBody });
 			const res = mock<Response>();
-			userRepository.save.calledWith(anyObject()).mockResolvedValue(req.user);
+			userRepository.findOneOrFail.mockResolvedValue(user);
 			jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token');
 
 			await controller.updateCurrentUser(req, res);
 
+			expect(userRepository.update).toHaveBeenCalled();
+
 			const cookieOptions = captor<CookieOptions>();
 			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<User>({
+				id: '123',
+				password: 'password',
+				authIdentities: [],
+				globalRoleId: '1',
+			});
+			const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' };
+			const req = mock<MeRequest.UserUpdate>({ user, body: reqBody });
+			const res = mock<Response>();
+			userRepository.findOneOrFail.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', () => {

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:
+  /@storybook/csf/0.0.2-next.10:
-    resolution: {integrity: sha512-ECOLMK425s+z8oA0aVAhBhhquuwTsZrM4oha/5De44JG8uYGXhqVrv/l27oxZEkwytuiQu+9f65HxYli+DY+3w==}
+    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'}