From cfe9525dd4e2dbf2496bd86ad854bb744b5dc8fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 3 Jan 2024 09:33:35 +0100 Subject: [PATCH] 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 --- packages/cli/src/GenericHelpers.ts | 10 +- .../cli/src/controllers/users.controller.ts | 86 +++++-------- packages/cli/src/requests.ts | 17 +-- .../cli/test/integration/users.api.test.ts | 119 ++++++------------ packages/editor-ui/src/api/users.ts | 12 +- packages/editor-ui/src/stores/users.store.ts | 9 +- .../editor-ui/src/views/SettingsUsersView.vue | 5 +- 7 files changed, 100 insertions(+), 158 deletions(-) diff --git a/packages/cli/src/GenericHelpers.ts b/packages/cli/src/GenericHelpers.ts index b37d395882505..92c7ad71ca7aa 100644 --- 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 { const errors = await validate(entity); diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index 9a62f0eba4bda..8ca0321b0f0e7 100644 --- 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 index 585b900356830..e7f9c33484e50 100644 --- 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; +} + 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 }, diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index 2ca6bb5350bd7..94818e034e84f 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -361,15 +361,8 @@ describe('PATCH /users/:id/role', () => { let memberAgent: SuperAgentTest; let authlessAgent: SuperAgentTest; - 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 { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } = + UsersController.ERROR_MESSAGES.CHANGE_ROLE; const UNAUTHORIZED = 'Unauthorized'; @@ -393,17 +386,31 @@ describe('PATCH /users/:id/role', () => { describe('unauthenticated user', () => { test('should receive 401', async () => { const response = await authlessAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(401); }); }); + describe('Invalid payload should return 400 when newRoleName', () => { + test.each([ + ['is missing', {}], + ['is `owner`', { newRoleName: 'owner' }], + ['is an array', { newRoleName: ['owner'] }], + ])('%s', async (_, payload) => { + const response = await adminAgent.patch(`/users/${member.id}/role`).send(payload); + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe( + 'newRoleName must be one of the following values: member, admin', + ); + }); + }); + describe('member', () => { test('should fail to demote owner to member', async () => { const response = await memberAgent.patch(`/users/${owner.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(403); @@ -412,7 +419,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to demote owner to admin', async () => { const response = await memberAgent.patch(`/users/${owner.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); @@ -421,7 +428,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to demote admin to member', async () => { const response = await memberAgent.patch(`/users/${admin.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(403); @@ -430,7 +437,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to promote other member to owner', async () => { const response = await memberAgent.patch(`/users/${otherMember.id}/role`).send({ - newRole: { scope: 'global', name: 'owner' }, + newRoleName: 'owner', }); expect(response.statusCode).toBe(403); @@ -439,7 +446,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to promote other member to admin', async () => { const response = await memberAgent.patch(`/users/${otherMember.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); @@ -448,7 +455,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to promote self to admin', async () => { const response = await memberAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); @@ -457,7 +464,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to promote self to owner', async () => { const response = await memberAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'owner' }, + newRoleName: 'owner', }); expect(response.statusCode).toBe(403); @@ -466,25 +473,11 @@ describe('PATCH /users/:id/role', () => { }); describe('admin', () => { - test('should receive 400 on invalid payload', async () => { - const response = await adminAgent.patch(`/users/${member.id}/role`).send({}); - - expect(response.statusCode).toBe(400); - expect(response.body.message).toBe(MISSING_NEW_ROLE_KEY); - - const _response = await adminAgent.patch(`/users/${member.id}/role`).send({ - newRole: {}, - }); - - expect(_response.statusCode).toBe(400); - expect(_response.body.message).toBe(MISSING_NEW_ROLE_VALUE); - }); - test('should receive 404 on unknown target user', async () => { const response = await adminAgent .patch('/users/c2317ff3-7a9f-4fd4-ad2b-7331f6359260/role') .send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(404); @@ -493,7 +486,7 @@ describe('PATCH /users/:id/role', () => { test('should fail to demote owner to admin', async () => { const response = await adminAgent.patch(`/users/${owner.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); @@ -502,45 +495,27 @@ describe('PATCH /users/:id/role', () => { test('should fail to demote owner to member', async () => { const response = await adminAgent.patch(`/users/${owner.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(403); expect(response.body.message).toBe(NO_ADMIN_ON_OWNER); }); - test('should fail to promote member to owner', async () => { - const response = await adminAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'owner' }, - }); - - expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_USER_TO_OWNER); - }); - - test('should fail to promote admin to owner', async () => { - const response = await adminAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'owner' }, - }); - - expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_USER_TO_OWNER); - }); - test('should fail to promote member to admin if not licensed', async () => { testServer.license.disable('feat:advancedPermissions'); const response = await adminAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_ADMIN_IF_UNLICENSED); + expect(response.body.message).toBe('Plan lacks license for this feature'); }); test('should be able to demote admin to member', async () => { const response = await adminAgent.patch(`/users/${otherAdmin.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(200); @@ -559,7 +534,7 @@ describe('PATCH /users/:id/role', () => { test('should be able to demote self to member', async () => { const response = await adminAgent.patch(`/users/${admin.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(200); @@ -578,7 +553,7 @@ describe('PATCH /users/:id/role', () => { test('should be able to promote member to admin if licensed', async () => { const response = await adminAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(200); @@ -599,7 +574,7 @@ describe('PATCH /users/:id/role', () => { describe('owner', () => { test('should fail to demote self to admin', async () => { const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); @@ -608,45 +583,27 @@ describe('PATCH /users/:id/role', () => { test('should fail to demote self to member', async () => { const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(403); expect(response.body.message).toBe(NO_OWNER_ON_OWNER); }); - test('should fail to promote admin to owner', async () => { - const response = await ownerAgent.patch(`/users/${admin.id}/role`).send({ - newRole: { scope: 'global', name: 'owner' }, - }); - - expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_USER_TO_OWNER); - }); - - test('should fail to promote member to owner', async () => { - const response = await ownerAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'owner' }, - }); - - expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_USER_TO_OWNER); - }); - test('should fail to promote member to admin if not licensed', async () => { testServer.license.disable('feat:advancedPermissions'); const response = await ownerAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_ADMIN_IF_UNLICENSED); + expect(response.body.message).toBe('Plan lacks license for this feature'); }); test('should be able to promote member to admin if licensed', async () => { const response = await ownerAgent.patch(`/users/${member.id}/role`).send({ - newRole: { scope: 'global', name: 'admin' }, + newRoleName: 'admin', }); expect(response.statusCode).toBe(200); @@ -665,7 +622,7 @@ describe('PATCH /users/:id/role', () => { test('should be able to demote admin to member', async () => { const response = await ownerAgent.patch(`/users/${admin.id}/role`).send({ - newRole: { scope: 'global', name: 'member' }, + newRoleName: 'member', }); expect(response.statusCode).toBe(200); diff --git a/packages/editor-ui/src/api/users.ts b/packages/editor-ui/src/api/users.ts index 679625d72060d..c6b8e361bd4cc 100644 --- a/packages/editor-ui/src/api/users.ts +++ b/packages/editor-ui/src/api/users.ts @@ -7,7 +7,6 @@ import type { } from '@/Interface'; import type { IDataObject } from 'n8n-workflow'; import { makeRestApiRequest } from '@/utils/apiUtils'; -import type { ScopeLevel } from '@n8n/permissions'; export async function loginCurrentUser( context: IRestApiContext, @@ -146,9 +145,14 @@ export async function submitPersonalizationSurvey( await makeRestApiRequest(context, 'POST', '/me/survey', params as unknown as IDataObject); } -export async function updateRole( +export interface UpdateGlobalRolePayload { + id: string; + newRoleName: Exclude; +} + +export async function updateGlobalRole( context: IRestApiContext, - { id, role }: { id: string; role: { scope: ScopeLevel; name: IRole } }, + { id, newRoleName }: UpdateGlobalRolePayload, ): Promise { - return makeRestApiRequest(context, 'PATCH', `/users/${id}/role`, { newRole: role }); + return makeRestApiRequest(context, 'PATCH', `/users/${id}/role`, { newRoleName }); } diff --git a/packages/editor-ui/src/stores/users.store.ts b/packages/editor-ui/src/stores/users.store.ts index 6d20dda8e4611..ab27063e5d80d 100644 --- a/packages/editor-ui/src/stores/users.store.ts +++ b/packages/editor-ui/src/stores/users.store.ts @@ -1,3 +1,4 @@ +import type { UpdateGlobalRolePayload } from '@/api/users'; import { changePassword, deleteUser, @@ -15,7 +16,7 @@ import { updateOtherUserSettings, validatePasswordToken, validateSignupToken, - updateRole, + updateGlobalRole, } from '@/api/users'; import { PERSONALIZATION_MODAL_KEY, STORES } from '@/constants'; import type { @@ -40,7 +41,7 @@ import { useCloudPlanStore } from './cloudPlan.store'; import { disableMfa, enableMfa, getMfaQR, verifyMfaToken } from '@/api/mfa'; import { confirmEmail, getCloudUserInfo } from '@/api/cloudPlans'; import { useRBACStore } from '@/stores/rbac.store'; -import type { Scope, ScopeLevel } from '@n8n/permissions'; +import type { Scope } from '@n8n/permissions'; import { inviteUsers, acceptInvitation } from '@/api/invitation'; const isPendingUser = (user: IUserResponse | null) => !!user?.isPending; @@ -379,9 +380,9 @@ export const useUsersStore = defineStore(STORES.USERS, { await confirmEmail(useRootStore().getRestApiContext); }, - async updateRole({ id, role }: { id: string; role: { scope: ScopeLevel; name: IRole } }) { + async updateGlobalRole({ id, newRoleName }: UpdateGlobalRolePayload) { const rootStore = useRootStore(); - await updateRole(rootStore.getRestApiContext, { id, role }); + await updateGlobalRole(rootStore.getRestApiContext, { id, newRoleName }); await this.fetchUsers(); }, }, diff --git a/packages/editor-ui/src/views/SettingsUsersView.vue b/packages/editor-ui/src/views/SettingsUsersView.vue index 9c552fa27fa7e..d35f9b539075f 100644 --- a/packages/editor-ui/src/views/SettingsUsersView.vue +++ b/packages/editor-ui/src/views/SettingsUsersView.vue @@ -99,6 +99,7 @@ import { useSSOStore } from '@/stores/sso.store'; import { hasPermission } from '@/rbac/permissions'; import { ROLE } from '@/utils/userUtils'; import { useClipboard } from '@/composables/useClipboard'; +import type { UpdateGlobalRolePayload } from '@/api/users'; export default defineComponent({ name: 'SettingsUsersView', @@ -280,8 +281,8 @@ export default defineComponent({ goToUpgradeAdvancedPermissions() { void this.uiStore.goToUpgrade('settings-users', 'upgrade-advanced-permissions'); }, - async onRoleChange(user: IUser, name: IRole) { - await this.usersStore.updateRole({ id: user.id, role: { scope: 'global', name } }); + async onRoleChange(user: IUser, newRoleName: UpdateGlobalRolePayload['newRoleName']) { + await this.usersStore.updateGlobalRole({ id: user.id, newRoleName }); }, }, });