From 7a86d3606852fcbc537533af24eef34279b229c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 24 Nov 2023 11:40:08 +0100 Subject: [PATCH] feat(core): Allow user role modification (#7797) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://linear.app/n8n/issue/PAY-985 ``` PATCH /users/:id/role unauthenticated user ✓ should receive 401 (349 ms) member ✓ should fail to demote owner to member (349 ms) ✓ should fail to demote owner to admin (359 ms) ✓ should fail to demote admin to member (381 ms) ✓ should fail to promote other member to owner (353 ms) ✓ should fail to promote other member to admin (377 ms) ✓ should fail to promote self to admin (354 ms) ✓ should fail to promote self to owner (371 ms) admin ✓ should receive 400 on invalid payload (351 ms) ✓ should receive 404 on unknown target user (351 ms) ✓ should fail to demote owner to admin (349 ms) ✓ should fail to demote owner to member (347 ms) ✓ should fail to promote member to owner (384 ms) ✓ should fail to promote admin to owner (350 ms) ✓ should be able to demote admin to member (354 ms) ✓ should be able to demote self to member (350 ms) ✓ should be able to promote member to admin (349 ms) owner ✓ should be able to promote member to admin (349 ms) ✓ should be able to demote admin to member (349 ms) ✓ should fail to demote self to admin (348 ms) ✓ should fail to demote self to member (354 ms) ``` --- .../cli/src/controllers/users.controller.ts | 90 +++++- packages/cli/src/databases/entities/Role.ts | 2 +- packages/cli/src/requests.ts | 9 +- packages/cli/src/services/role.service.ts | 7 +- .../cli/test/integration/shared/db/roles.ts | 4 + .../cli/test/integration/shared/db/users.ts | 12 +- .../cli/test/integration/users.api.test.ts | 279 +++++++++++++++++- 7 files changed, 384 insertions(+), 19 deletions(-) diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index fb9b0edce6712..46f098736c687 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -4,7 +4,7 @@ import { User } from '@db/entities/User'; import { SharedCredentials } from '@db/entities/SharedCredentials'; import { SharedWorkflow } from '@db/entities/SharedWorkflow'; import { Authorized, Delete, Get, RestController, Patch } from '@/decorators'; -import { BadRequestError, NotFoundError } from '@/ResponseHelper'; +import { BadRequestError, NotFoundError, UnauthorizedError } from '@/ResponseHelper'; import { ListQuery, UserRequest, UserSettingsUpdatePayload } from '@/requests'; import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; import { IExternalHooksClass, IInternalHooksClass } from '@/Interfaces'; @@ -18,7 +18,7 @@ import { UserService } from '@/services/user.service'; import { listQueryMiddleware } from '@/middlewares'; import { Logger } from '@/Logger'; -@Authorized(['global', 'owner']) +@Authorized() @RestController('/users') export class UsersController { constructor( @@ -32,6 +32,18 @@ export class UsersController { private readonly userService: UserService, ) {} + static ERROR_MESSAGES = { + CHANGE_ROLE: { + NO_MEMBER: 'Member cannot change role for any user', + 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_ADMIN_TO_OWNER: 'Admin cannot promote user to global owner', + }, + } as const; + private async toFindManyOptions(listQueryOptions?: ListQuery.Options) { const findManyOptions: FindManyOptions = {}; @@ -70,7 +82,7 @@ export class UsersController { return findManyOptions; } - removeSupplementaryFields( + private removeSupplementaryFields( publicUsers: Array>, listQueryOptions: ListQuery.Options, ) { @@ -152,6 +164,7 @@ export class UsersController { /** * Delete a user. Optionally, designate a transferee for their workflows and credentials. */ + @Authorized(['global', 'owner']) @Delete('/:id') async deleteUser(req: UserRequest.Delete) { const { id: idToDelete } = req.params; @@ -306,4 +319,75 @@ export class UsersController { await this.externalHooks.run('user.deleted', [await this.userService.toPublic(userToDelete)]); return { success: true }; } + + // @TODO: Add scope check `@RequireGlobalScope('user:changeRole')` + // once this has been merged: https://github.com/n8n-io/n8n/pull/7737 + @Authorized('any') + @Patch('/:id/role') + async changeRole(req: UserRequest.ChangeRole) { + const { + NO_MEMBER, + MISSING_NEW_ROLE_KEY, + MISSING_NEW_ROLE_VALUE, + NO_ADMIN_ON_OWNER, + NO_ADMIN_TO_OWNER, + NO_USER, + NO_OWNER_ON_OWNER, + } = UsersController.ERROR_MESSAGES.CHANGE_ROLE; + + if (req.user.globalRole.scope === 'global' && req.user.globalRole.name === 'member') { + throw new UnauthorizedError(NO_MEMBER); + } + + const { newRole } = req.body; + + if (!newRole) { + throw new BadRequestError(MISSING_NEW_ROLE_KEY); + } + + if (!newRole.name || !newRole.scope) { + throw new BadRequestError(MISSING_NEW_ROLE_VALUE); + } + + if ( + req.user.globalRole.scope === 'global' && + req.user.globalRole.name === 'admin' && + newRole.scope === 'global' && + newRole.name === 'owner' + ) { + throw new UnauthorizedError(NO_ADMIN_TO_OWNER); + } + + const targetUser = await this.userService.findOne({ + where: { id: req.params.id }, + }); + + if (targetUser === null) { + throw new NotFoundError(NO_USER); + } + + if ( + req.user.globalRole.scope === 'global' && + req.user.globalRole.name === 'admin' && + targetUser.globalRole.scope === 'global' && + 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' + ) { + throw new UnauthorizedError(NO_OWNER_ON_OWNER); + } + + const roleToSet = await this.roleService.findCached(newRole.scope, newRole.name); + + await this.userService.update(targetUser.id, { globalRole: roleToSet }); + + return { success: true }; + } } diff --git a/packages/cli/src/databases/entities/Role.ts b/packages/cli/src/databases/entities/Role.ts index 7d5e287307204..070e8e6be8745 100644 --- a/packages/cli/src/databases/entities/Role.ts +++ b/packages/cli/src/databases/entities/Role.ts @@ -7,7 +7,7 @@ import type { SharedCredentials } from './SharedCredentials'; import { WithTimestamps } from './AbstractEntity'; import { idStringifier } from '../utils/transformers'; -export type RoleNames = 'owner' | 'member' | 'user' | 'editor'; +export type RoleNames = 'owner' | 'member' | 'user' | 'editor' | 'admin'; export type RoleScopes = 'global' | 'workflow' | 'credential'; @Entity() diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index c0c28a6738fc4..61857409a4d71 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -25,7 +25,7 @@ import type { SecretsProvider, SecretsProviderState, } from '@/Interfaces'; -import type { Role } from '@db/entities/Role'; +import type { Role, RoleNames, RoleScopes } from '@db/entities/Role'; import type { User } from '@db/entities/User'; import type { UserManagementMailer } from '@/UserManagement/email'; import type { Variables } from '@db/entities/Variables'; @@ -322,6 +322,13 @@ export declare namespace UserRequest { { transferId?: string; includeRole: boolean } >; + export type ChangeRole = AuthenticatedRequest< + { id: string }, + {}, + { newRole?: { scope?: RoleScopes; name?: RoleNames } }, + {} + >; + export type Get = AuthenticatedRequest< { id: string; email: string; identifier: string }, {}, diff --git a/packages/cli/src/services/role.service.ts b/packages/cli/src/services/role.service.ts index d27f658174989..8be8350dccdcb 100644 --- a/packages/cli/src/services/role.service.ts +++ b/packages/cli/src/services/role.service.ts @@ -24,7 +24,7 @@ export class RoleService { void this.cacheService.setMany(allRoles.map((r) => [r.cacheKey, r])); } - private async findCached(scope: RoleScopes, name: RoleNames) { + async findCached(scope: RoleScopes, name: RoleNames) { const cacheKey = `role:${scope}:${name}`; const cachedRole = await this.cacheService.get(cacheKey); @@ -50,6 +50,7 @@ export class RoleService { private roles: Array<{ name: RoleNames; scope: RoleScopes }> = [ { scope: 'global', name: 'owner' }, { scope: 'global', name: 'member' }, + { scope: 'global', name: 'admin' }, { scope: 'workflow', name: 'owner' }, { scope: 'credential', name: 'owner' }, { scope: 'credential', name: 'user' }, @@ -68,6 +69,10 @@ export class RoleService { return this.findCached('global', 'member'); } + async findGlobalAdminRole() { + return this.findCached('global', 'admin'); + } + async findWorkflowOwnerRole() { return this.findCached('workflow', 'owner'); } diff --git a/packages/cli/test/integration/shared/db/roles.ts b/packages/cli/test/integration/shared/db/roles.ts index 9392fc3cc5dd9..9be0c43193b95 100644 --- a/packages/cli/test/integration/shared/db/roles.ts +++ b/packages/cli/test/integration/shared/db/roles.ts @@ -9,6 +9,10 @@ export async function getGlobalMemberRole() { return Container.get(RoleService).findGlobalMemberRole(); } +export async function getGlobalAdminRole() { + return Container.get(RoleService).findGlobalAdminRole(); +} + export async function getWorkflowOwnerRole() { return Container.get(RoleService).findWorkflowOwnerRole(); } diff --git a/packages/cli/test/integration/shared/db/users.ts b/packages/cli/test/integration/shared/db/users.ts index 27140b526d1ca..25a8d7df184f3 100644 --- a/packages/cli/test/integration/shared/db/users.ts +++ b/packages/cli/test/integration/shared/db/users.ts @@ -9,7 +9,7 @@ import { TOTPService } from '@/Mfa/totp.service'; import { MfaService } from '@/Mfa/mfa.service'; import { randomApiKey, randomEmail, randomName, randomValidPassword } from '../random'; -import { getGlobalMemberRole, getGlobalOwnerRole } from './roles'; +import { getGlobalAdminRole, getGlobalMemberRole, getGlobalOwnerRole } from './roles'; /** * Store a user in the DB, defaulting to a `member`. @@ -76,6 +76,10 @@ export async function createMember() { return createUser({ globalRole: await getGlobalMemberRole() }); } +export async function createAdmin() { + return createUser({ globalRole: await getGlobalAdminRole() }); +} + export async function createUserShell(globalRole: Role): Promise { if (globalRole.scope !== 'global') { throw new Error(`Invalid role received: ${JSON.stringify(globalRole)}`); @@ -128,6 +132,12 @@ export const getAllUsers = async () => relations: ['globalRole', 'authIdentities'], }); +export const getUserById = async (id: string) => + Container.get(UserRepository).findOneOrFail({ + where: { id }, + relations: ['globalRole', 'authIdentities'], + }); + export const getLdapIdentities = async () => Container.get(AuthIdentityRepository).find({ where: { providerType: 'ldap' }, diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index 65af440ab0494..53f70dca9439b 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -19,10 +19,14 @@ import * as testDb from './shared/testDb'; import * as utils from './shared/utils/'; import { saveCredential } from './shared/db/credentials'; import { getAllRoles } from './shared/db/roles'; -import { createMember, createOwner, createUser } from './shared/db/users'; +import { createAdmin, createMember, createOwner, createUser, getUserById } from './shared/db/users'; import { createWorkflow } from './shared/db/workflows'; import type { PublicUser } from '@/Interfaces'; import { InternalHooks } from '@/InternalHooks'; +import { UsersController } from '@/controllers/users.controller'; +import { ExternalHooks } from '@/ExternalHooks'; +import { Logger } from '@/Logger'; +import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; const { any } = expect; @@ -31,19 +35,25 @@ let globalMemberRole: Role; let workflowOwnerRole: Role; let owner: User; +let admin: User; +let otherAdmin: User; let member: User; +let otherMember: User; + let authOwnerAgent: SuperAgentTest; +let authAdminAgent: SuperAgentTest; +let authMemberAgent: SuperAgentTest; let authlessAgent: SuperAgentTest; +let usersCount: number; + mockInstance(InternalHooks); +mockInstance(ExternalHooks); +mockInstance(Logger); +mockInstance(ActiveWorkflowRunner); const testServer = utils.setupTestServer({ endpointGroups: ['users'] }); -type UserInvitationResponse = { - user: Pick & { inviteAcceptUrl: string; emailSent: boolean }; - error?: string; -}; - beforeAll(async () => { const [_, fetchedGlobalMemberRole, fetchedWorkflowOwnerRole, fetchedCredentialOwnerRole] = await getAllRoles(); @@ -51,6 +61,8 @@ beforeAll(async () => { credentialOwnerRole = fetchedCredentialOwnerRole; globalMemberRole = fetchedGlobalMemberRole; workflowOwnerRole = fetchedWorkflowOwnerRole; + + usersCount = [owner, admin, otherAdmin, member, otherMember].length; }); beforeEach(async () => { @@ -58,7 +70,12 @@ beforeEach(async () => { await testDb.truncate(['User', 'SharedCredentials', 'SharedWorkflow', 'Workflow', 'Credentials']); owner = await createOwner(); member = await createMember(); + otherMember = await createMember(); + admin = await createAdmin(); + otherAdmin = await createAdmin(); authOwnerAgent = testServer.authAgentFor(owner); + authAdminAgent = testServer.authAgentFor(admin); + authMemberAgent = testServer.authAgentFor(member); authlessAgent = testServer.authlessAgent; }); @@ -80,7 +97,7 @@ describe('GET /users', () => { test('should return all users', async () => { const response = await authOwnerAgent.get('/users').expect(200); - expect(response.body.data).toHaveLength(2); + expect(response.body.data).toHaveLength(usersCount); response.body.data.forEach(validatePublicUser); }); @@ -172,7 +189,7 @@ describe('GET /users', () => { .query('filter={ "isOwner": false }') .expect(200); - expect(_response.body.data).toHaveLength(1); + expect(_response.body.data).toHaveLength(usersCount - 1); const [_user] = _response.body.data; @@ -185,7 +202,7 @@ describe('GET /users', () => { const response = await authOwnerAgent.get('/users').query('select=["id"]').expect(200); expect(response.body).toEqual({ - data: [{ id: any(String) }, { id: any(String) }], + data: new Array(usersCount).fill({ id: any(String) }), }); }); @@ -193,7 +210,7 @@ describe('GET /users', () => { const response = await authOwnerAgent.get('/users').query('select=["email"]').expect(200); expect(response.body).toEqual({ - data: [{ email: any(String) }, { email: any(String) }], + data: new Array(usersCount).fill({ email: any(String) }), }); }); @@ -201,7 +218,7 @@ describe('GET /users', () => { const response = await authOwnerAgent.get('/users').query('select=["firstName"]').expect(200); expect(response.body).toEqual({ - data: [{ firstName: any(String) }, { firstName: any(String) }], + data: new Array(usersCount).fill({ firstName: any(String) }), }); }); @@ -209,7 +226,7 @@ describe('GET /users', () => { const response = await authOwnerAgent.get('/users').query('select=["lastName"]').expect(200); expect(response.body).toEqual({ - data: [{ lastName: any(String) }, { lastName: any(String) }], + data: new Array(usersCount).fill({ lastName: any(String) }), }); }); }); @@ -379,3 +396,241 @@ describe('DELETE /users/:id', () => { expect(deletedUser).toBeNull(); }); }); + +describe('PATCH /users/:id/role', () => { + const { + NO_MEMBER, + MISSING_NEW_ROLE_KEY, + MISSING_NEW_ROLE_VALUE, + NO_ADMIN_ON_OWNER, + NO_ADMIN_TO_OWNER, + NO_USER, + NO_OWNER_ON_OWNER, + } = UsersController.ERROR_MESSAGES.CHANGE_ROLE; + + describe('unauthenticated user', () => { + test('should receive 401', async () => { + const response = await authlessAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(401); + }); + }); + + describe('member', () => { + test('should fail to demote owner to member', async () => { + const response = await authMemberAgent.patch(`/users/${owner.id}/role`).send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + + test('should fail to demote owner to admin', async () => { + const response = await authMemberAgent.patch(`/users/${owner.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + + test('should fail to demote admin to member', async () => { + const response = await authMemberAgent.patch(`/users/${admin.id}/role`).send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + + test('should fail to promote other member to owner', async () => { + const response = await authMemberAgent.patch(`/users/${otherMember.id}/role`).send({ + newRole: { scope: 'global', name: 'owner' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + + test('should fail to promote other member to admin', async () => { + const response = await authMemberAgent.patch(`/users/${otherMember.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + + test('should fail to promote self to admin', async () => { + const response = await authMemberAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + + test('should fail to promote self to owner', async () => { + const response = await authMemberAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'owner' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_MEMBER); + }); + }); + + describe('admin', () => { + test('should receive 400 on invalid payload', async () => { + const response = await authAdminAgent.patch(`/users/${member.id}/role`).send({}); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe(MISSING_NEW_ROLE_KEY); + + const _response = await authAdminAgent.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 authAdminAgent.patch('/users/99999/role').send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(404); + expect(response.body.message).toBe(NO_USER); + }); + + test('should fail to demote owner to admin', async () => { + const response = await authAdminAgent.patch(`/users/${owner.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_ADMIN_ON_OWNER); + }); + + test('should fail to demote owner to member', async () => { + const response = await authAdminAgent.patch(`/users/${owner.id}/role`).send({ + newRole: { scope: 'global', name: '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 authAdminAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'owner' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_ADMIN_TO_OWNER); + }); + + test('should fail to promote admin to owner', async () => { + const response = await authAdminAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'owner' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_ADMIN_TO_OWNER); + }); + + test('should be able to demote admin to member', async () => { + const response = await authAdminAgent.patch(`/users/${otherAdmin.id}/role`).send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toStrictEqual({ success: true }); + + const user = await getUserById(otherAdmin.id); + + expect(user.globalRole.scope).toBe('global'); + expect(user.globalRole.name).toBe('member'); + }); + + test('should be able to demote self to member', async () => { + const response = await authAdminAgent.patch(`/users/${admin.id}/role`).send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toStrictEqual({ success: true }); + + const user = await getUserById(admin.id); + + expect(user.globalRole.scope).toBe('global'); + expect(user.globalRole.name).toBe('member'); + }); + + test('should be able to promote member to admin', async () => { + const response = await authAdminAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toStrictEqual({ success: true }); + + const user = await getUserById(admin.id); + + expect(user.globalRole.scope).toBe('global'); + expect(user.globalRole.name).toBe('admin'); + }); + }); + + describe('owner', () => { + test('should be able to promote member to admin', async () => { + const response = await authOwnerAgent.patch(`/users/${member.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toStrictEqual({ success: true }); + + const user = await getUserById(admin.id); + + expect(user.globalRole.scope).toBe('global'); + expect(user.globalRole.name).toBe('admin'); + }); + + test('should be able to demote admin to member', async () => { + const response = await authOwnerAgent.patch(`/users/${admin.id}/role`).send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toStrictEqual({ success: true }); + + const user = await getUserById(admin.id); + + expect(user.globalRole.scope).toBe('global'); + expect(user.globalRole.name).toBe('member'); + }); + + test('should fail to demote self to admin', async () => { + const response = await authOwnerAgent.patch(`/users/${owner.id}/role`).send({ + newRole: { scope: 'global', name: 'admin' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_OWNER_ON_OWNER); + }); + + test('should fail to demote self to member', async () => { + const response = await authOwnerAgent.patch(`/users/${owner.id}/role`).send({ + newRole: { scope: 'global', name: 'member' }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe(NO_OWNER_ON_OWNER); + }); + }); +});