fix(core): Better input validation for the changeRole endpoint (#8189)

also refactored the code to 1. stop passing around `scope === 'global'`, since this code can be used only for changing globalRole. 2. leak less details when input validation fails. ## Review / Merge checklist - [x] PR title and summary are descriptive - [x] Tests included
n8n-io · Jan 3, 2024 · cfe9525 · cfe9525
1 parent 11cda41
commit cfe9525
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 158 deletions.
diff --git a/packages/cli/src/GenericHelpers.ts b/packages/cli/src/GenericHelpers.ts
@@ -4,7 +4,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';
+import type { UserRoleChangePayload, UserUpdatePayload } from '@/requests';
 import { BadRequestError } from './errors/response-errors/bad-request.error';
 
 /**
@@ -15,7 +15,13 @@ export function getSessionId(req: express.Request): string | undefined {
 }
 
 export async function validateEntity(
-	entity: WorkflowEntity | CredentialsEntity | TagEntity | User | UserUpdatePayload,
+	entity:
+		| WorkflowEntity
+		| CredentialsEntity
+		| TagEntity
+		| User
+		| UserUpdatePayload
+		| UserRoleChangePayload,
 ): Promise<void> {
 	const errors = await validate(entity);
 

diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts
@@ -2,13 +2,27 @@ import { In } from 'typeorm';
 import { User } from '@db/entities/User';
 import { SharedCredentials } from '@db/entities/SharedCredentials';
 import { SharedWorkflow } from '@db/entities/SharedWorkflow';
-import { RequireGlobalScope, Authorized, Delete, Get, RestController, Patch } from '@/decorators';
-import { ListQuery, UserRequest, UserSettingsUpdatePayload } from '@/requests';
+import {
+	RequireGlobalScope,
+	Authorized,
+	Delete,
+	Get,
+	RestController,
+	Patch,
+	Licensed,
+} from '@/decorators';
+import {
+	ListQuery,
+	UserRequest,
+	UserRoleChangePayload,
+	UserSettingsUpdatePayload,
+} from '@/requests';
 import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
 import type { PublicUser, ITelemetryUserDeletionData } from '@/Interfaces';
 import { AuthIdentity } from '@db/entities/AuthIdentity';
 import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository';
 import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository';
+import { UserRepository } from '@db/repositories/user.repository';
 import { plainToInstance } from 'class-transformer';
 import { RoleService } from '@/services/role.service';
 import { UserService } from '@/services/user.service';
@@ -17,10 +31,9 @@ import { Logger } from '@/Logger';
 import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error';
 import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import { BadRequestError } from '@/errors/response-errors/bad-request.error';
-import { License } from '@/License';
 import { ExternalHooks } from '@/ExternalHooks';
 import { InternalHooks } from '@/InternalHooks';
-import { UserRepository } from '@/databases/repositories/user.repository';
+import { validateEntity } from '@/GenericHelpers';
 
 @Authorized()
 @RestController('/users')
@@ -31,22 +44,17 @@ export class UsersController {
 		private readonly internalHooks: InternalHooks,
 		private readonly sharedCredentialsRepository: SharedCredentialsRepository,
 		private readonly sharedWorkflowRepository: SharedWorkflowRepository,
+		private readonly userRepository: UserRepository,
 		private readonly activeWorkflowRunner: ActiveWorkflowRunner,
 		private readonly roleService: RoleService,
 		private readonly userService: UserService,
-		private readonly license: License,
-		private readonly userRepository: UserRepository,
 	) {}
 
 	static ERROR_MESSAGES = {
 		CHANGE_ROLE: {
-			MISSING_NEW_ROLE_KEY: 'Expected `newRole` to exist',
-			MISSING_NEW_ROLE_VALUE: 'Expected `newRole` to have `name` and `scope`',
 			NO_USER: 'Target user not found',
 			NO_ADMIN_ON_OWNER: 'Admin cannot change role on global owner',
 			NO_OWNER_ON_OWNER: 'Owner cannot change role on global owner',
-			NO_USER_TO_OWNER: 'Cannot promote user to global owner',
-			NO_ADMIN_IF_UNLICENSED: 'Admin role is not available without a license',
 		},
 	} as const;
 
@@ -298,74 +306,38 @@ export class UsersController {
 
 	@Patch('/:id/role')
 	@RequireGlobalScope('user:changeRole')
-	async changeRole(req: UserRequest.ChangeRole) {
-		const {
-			MISSING_NEW_ROLE_KEY,
-			MISSING_NEW_ROLE_VALUE,
-			NO_ADMIN_ON_OWNER,
-			NO_USER_TO_OWNER,
-			NO_USER,
-			NO_OWNER_ON_OWNER,
-			NO_ADMIN_IF_UNLICENSED,
-		} = UsersController.ERROR_MESSAGES.CHANGE_ROLE;
-
-		const { newRole } = req.body;
-
-		if (!newRole) {
-			throw new BadRequestError(MISSING_NEW_ROLE_KEY);
-		}
+	@Licensed('feat:advancedPermissions')
+	async changeGlobalRole(req: UserRequest.ChangeRole) {
+		const { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } =
+			UsersController.ERROR_MESSAGES.CHANGE_ROLE;
 
-		if (!newRole.name || !newRole.scope) {
-			throw new BadRequestError(MISSING_NEW_ROLE_VALUE);
-		}
-
-		if (newRole.scope === 'global' && newRole.name === 'owner') {
-			throw new UnauthorizedError(NO_USER_TO_OWNER);
-		}
+		const payload = plainToInstance(UserRoleChangePayload, req.body);
+		await validateEntity(payload);
 
 		const targetUser = await this.userRepository.findOne({
 			where: { id: req.params.id },
 			relations: ['globalRole'],
 		});
-
 		if (targetUser === null) {
 			throw new NotFoundError(NO_USER);
 		}
 
-		if (
-			newRole.scope === 'global' &&
-			newRole.name === 'admin' &&
-			!this.license.isAdvancedPermissionsLicensed()
-		) {
-			throw new UnauthorizedError(NO_ADMIN_IF_UNLICENSED);
-		}
-
-		if (
-			req.user.globalRole.scope === 'global' &&
-			req.user.globalRole.name === 'admin' &&
-			targetUser.globalRole.scope === 'global' &&
-			targetUser.globalRole.name === 'owner'
-		) {
+		if (req.user.globalRole.name === 'admin' && targetUser.globalRole.name === 'owner') {
 			throw new UnauthorizedError(NO_ADMIN_ON_OWNER);
 		}
 
-		if (
-			req.user.globalRole.scope === 'global' &&
-			req.user.globalRole.name === 'owner' &&
-			targetUser.globalRole.scope === 'global' &&
-			targetUser.globalRole.name === 'owner'
-		) {
+		if (req.user.globalRole.name === 'owner' && targetUser.globalRole.name === 'owner') {
 			throw new UnauthorizedError(NO_OWNER_ON_OWNER);
 		}
 
-		const roleToSet = await this.roleService.findCached(newRole.scope, newRole.name);
+		const roleToSet = await this.roleService.findCached('global', payload.newRoleName);
 
-		await this.userService.update(targetUser.id, { globalRole: roleToSet });
+		await this.userService.update(targetUser.id, { globalRoleId: roleToSet.id });
 
 		void this.internalHooks.onUserRoleChange({
 			user: req.user,
 			target_user_id: targetUser.id,
-			target_user_new_role: [newRole.scope, newRole.name].join(' '),
+			target_user_new_role: ['global', payload.newRoleName].join(' '),
 			public_api: false,
 		});
 

diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts
@@ -16,7 +16,7 @@ import type {
 	IWorkflowSettings,
 } from 'n8n-workflow';
 
-import { IsBoolean, IsEmail, IsOptional, IsString, Length } from 'class-validator';
+import { IsBoolean, IsEmail, IsIn, IsOptional, IsString, Length } from 'class-validator';
 import { NoXss } from '@db/utils/customValidators';
 import type {
 	PublicUser,
@@ -25,7 +25,7 @@ import type {
 	SecretsProvider,
 	SecretsProviderState,
 } from '@/Interfaces';
-import type { Role, RoleNames, RoleScopes } from '@db/entities/Role';
+import type { Role, RoleNames } from '@db/entities/Role';
 import type { User } from '@db/entities/User';
 import type { UserManagementMailer } from '@/UserManagement/email';
 import type { Variables } from '@db/entities/Variables';
@@ -47,6 +47,7 @@ export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'la
 	@Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' })
 	lastName: string;
 }
+
 export class UserSettingsUpdatePayload {
 	@IsBoolean({ message: 'userActivated should be a boolean' })
 	@IsOptional()
@@ -57,6 +58,11 @@ export class UserSettingsUpdatePayload {
 	allowSSOManualLogin?: boolean;
 }
 
+export class UserRoleChangePayload {
+	@IsIn(['member', 'admin'])
+	newRoleName: Exclude<RoleNames, 'user' | 'editor' | 'owner'>;
+}
+
 export type AuthlessRequest<
 	RouteParams = {},
 	ResponseBody = {},
@@ -332,12 +338,7 @@ export declare namespace UserRequest {
 		{ transferId?: string; includeRole: boolean }
 	>;
 
-	export type ChangeRole = AuthenticatedRequest<
-		{ id: string },
-		{},
-		{ newRole?: { scope?: RoleScopes; name?: RoleNames } },
-		{}
-	>;
+	export type ChangeRole = AuthenticatedRequest<{ id: string }, {}, UserRoleChangePayload, {}>;
 
 	export type Get = AuthenticatedRequest<
 		{ id: string; email: string; identifier: string },