diff --git a/packages/cli/src/controllers/__tests__/role.controller.test.ts b/packages/cli/src/controllers/__tests__/role.controller.test.ts index cc6a5a1bdd0..a33664554d2 100644 --- a/packages/cli/src/controllers/__tests__/role.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/role.controller.test.ts @@ -41,21 +41,6 @@ describe('RoleController', () => { }); describe('deleteRole', () => { - it('should emit custom-role-deleted', async () => { - const request = managerRequest(); - roleService.getRole.mockResolvedValue({ roleType: 'project' } as Role); - roleService.removeCustomRole.mockResolvedValue({ - slug: 'custom-editor', - } as Role); - - await controller.deleteRole(request, mock(), 'custom-editor', mock()); - - expect(eventService.emit).toHaveBeenCalledWith('custom-role-deleted', { - userId: '123', - roleSlug: 'custom-editor', - }); - }); - it('should pass the reassignment role through to the service for an entitled caller', async () => { const request = mock({ user: { @@ -76,10 +61,11 @@ describe('RoleController', () => { reassignRoleSlug: 'global:member', }); - expect(roleService.removeCustomRole).toHaveBeenCalledWith( - 'global:custom-editor', - 'global:member', - ); + expect(roleService.removeCustomRole).toHaveBeenCalledWith({ + slug: 'global:custom-editor', + reassignRoleSlug: 'global:member', + userId: '123', + }); }); it('should ignore the reassignment role when the caller lacks user:changeRole', async () => { @@ -99,10 +85,11 @@ describe('RoleController', () => { reassignRoleSlug: 'global:member', }); - expect(roleService.removeCustomRole).toHaveBeenCalledWith( - 'global:custom-editor', - undefined, - ); + expect(roleService.removeCustomRole).toHaveBeenCalledWith({ + slug: 'global:custom-editor', + reassignRoleSlug: undefined, + userId: '123', + }); }); it('should ignore the reassignment role when the caller holds the role being deleted', async () => { @@ -125,10 +112,11 @@ describe('RoleController', () => { reassignRoleSlug: 'global:member', }); - expect(roleService.removeCustomRole).toHaveBeenCalledWith( - 'global:custom-editor', - undefined, - ); + expect(roleService.removeCustomRole).toHaveBeenCalledWith({ + slug: 'global:custom-editor', + reassignRoleSlug: undefined, + userId: '123', + }); }); it('should ignore the reassignment role for project roles', async () => { @@ -151,10 +139,11 @@ describe('RoleController', () => { reassignRoleSlug: 'project:admin', }); - expect(roleService.removeCustomRole).toHaveBeenCalledWith( - 'project:custom-editor', - undefined, - ); + expect(roleService.removeCustomRole).toHaveBeenCalledWith({ + slug: 'project:custom-editor', + reassignRoleSlug: undefined, + userId: '123', + }); }); }); }); diff --git a/packages/cli/src/controllers/role.controller.ts b/packages/cli/src/controllers/role.controller.ts index 38a7e86641b..323c5fba3e1 100644 --- a/packages/cli/src/controllers/role.controller.ts +++ b/packages/cli/src/controllers/role.controller.ts @@ -14,7 +14,7 @@ import type { RoleProjectMembersResponse, } from '@n8n/api-types'; import { LICENSE_FEATURES } from '@n8n/constants'; -import { AuthenticatedRequest, User } from '@n8n/db'; +import { AuthenticatedRequest } from '@n8n/db'; import { Body, Delete, @@ -27,10 +27,10 @@ import { Query, RestController, } from '@n8n/decorators'; -import { hasGlobalScope, Role as RoleDTO } from '@n8n/permissions'; +import { Role as RoleDTO } from '@n8n/permissions'; import { EventService } from '@/events/event.service'; -import { assertCanManageRoleType } from '@/services/role-authorization'; +import { assertCanManageRoleType, canReassignUsers } from '@/services/role-authorization'; import { RoleService } from '@/services/role.service'; @RestController('/roles') @@ -40,23 +40,6 @@ export class RoleController { private readonly eventService: EventService, ) {} - /** - * Reassigning a deleted role's users is effectively a bulk instance-role change, - * so it is only honored for callers entitled to change users' instance roles - * (the same scopes the dedicated role-change endpoint requires), never for - * project roles, and never for the caller's own role. When it is not honored the - * reassignment target is ignored, so a role that still has users cannot be - * deleted. - */ - private canReassignUsers(user: User, role: RoleDTO): boolean { - return ( - role.roleType === 'global' && - user.role.slug !== role.slug && - hasGlobalScope(user, 'role:manage') && - hasGlobalScope(user, 'user:changeRole') - ); - } - @Get('/') async getAllRoles( _req: AuthenticatedRequest, @@ -80,7 +63,10 @@ export class RoleController { @Param('projectId') projectId: string, ): Promise { const role = await this.roleService.getRole(slug); - assertCanManageRoleType(req.user, role.roleType); + assertCanManageRoleType({ + roleType: role.roleType, + user: req.user, + }); const result = await this.roleService.getRoleProjectMembers(slug, projectId); return RoleProjectMembersResponseDto.parse(result); } @@ -92,7 +78,10 @@ export class RoleController { @Param('slug') slug: string, ): Promise { const role = await this.roleService.getRole(slug); - assertCanManageRoleType(req.user, role.roleType); + assertCanManageRoleType({ + roleType: role.roleType, + user: req.user, + }); const result = await this.roleService.getRoleAssignments(slug); return RoleAssignmentsResponseDto.parse(result); } @@ -127,7 +116,10 @@ export class RoleController { @Body updateRole: UpdateRoleDto, ): Promise { const role = await this.roleService.getRole(slug); - assertCanManageRoleType(req.user, role.roleType); + assertCanManageRoleType({ + roleType: role.roleType, + user: req.user, + }); return await this.roleService.updateCustomRole({ slug, newRole: updateRole, @@ -144,16 +136,18 @@ export class RoleController { @Query query: RoleDeleteQueryDto, ): Promise { const role = await this.roleService.getRole(slug); - assertCanManageRoleType(req.user, role.roleType); - const reassignRoleSlug = this.canReassignUsers(req.user, role) + assertCanManageRoleType({ + roleType: role.roleType, + user: req.user, + }); + const reassignRoleSlug = canReassignUsers({ role, user: req.user }) ? query.reassignRoleSlug : undefined; - const result = await this.roleService.removeCustomRole(slug, reassignRoleSlug); - this.eventService.emit('custom-role-deleted', { + return await this.roleService.removeCustomRole({ + slug, + reassignRoleSlug, userId: req.user.id, - roleSlug: result.slug, }); - return result; } @Post('/') @@ -163,7 +157,10 @@ export class RoleController { _res: Response, @Body createRole: CreateRoleDto, ): Promise { - assertCanManageRoleType(req.user, createRole.roleType); + assertCanManageRoleType({ + roleType: createRole.roleType, + user: req.user, + }); const result = await this.roleService.createCustomRole(createRole); this.eventService.emit('custom-role-created', { userId: req.user.id, diff --git a/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts b/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts index 75747d40aa9..e64de4def54 100644 --- a/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts +++ b/packages/cli/src/public-api/v1/controllers/roles.public.controller.ts @@ -1,5 +1,6 @@ import { CreateRoleDto, + RoleDeleteQueryDto, RoleGetPublicDto, RoleListPublicDto, RoleListQueryPublicDto, @@ -16,6 +17,7 @@ import { ApiSummary, ApiTags, Body, + Delete, Get, Licensed, Param, @@ -29,7 +31,7 @@ import type { Response } from 'express'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { EventService } from '@/events/event.service'; -import { assertCanManageRoleType } from '@/services/role-authorization'; +import { assertCanManageRoleType, canReassignUsers } from '@/services/role-authorization'; import { RoleService } from '@/services/role.service'; type PublicRoleNamespace = Extract; @@ -131,7 +133,11 @@ export class RolesPublicController { _res: Response, @Body createRole: CreateRoleDto, ): Promise { - assertCanManageRoleType(req.user, createRole.roleType); + assertCanManageRoleType({ + apiKeyScopes: req.tokenGrant?.apiKeyScopes ?? [], + roleType: createRole.roleType, + user: req.user, + }); const role = await this.roleService.createCustomRole(createRole); @@ -164,7 +170,12 @@ export class RolesPublicController { if (!isPublicRole(role)) { throw new NotFoundError('Role not found'); } - assertCanManageRoleType(req.user, role.roleType); + + assertCanManageRoleType({ + apiKeyScopes: req.tokenGrant?.apiKeyScopes ?? [], + roleType: role.roleType, + user: req.user, + }); const result = await this.roleService.updateCustomRole({ slug, @@ -174,4 +185,46 @@ export class RolesPublicController { return toRolePublicDto({ ...result, roleType: role.roleType }); } + + @Delete('/:slug') + @ApiKeyScope({ anyOf: ['role:manage', 'role:manageProject'] }) + @Licensed(LICENSE_FEATURES.CUSTOM_ROLES) + @ApiSummary('Delete a custom role') + @ApiDescription( + 'Deletes a custom role. System roles cannot be deleted. A role with users assigned cannot be deleted unless `reassignRoleSlug` is set to move those users to another role first.', + ) + @ApiTags(['Role']) + @ApiResponse(200, RolePublicDto) + @ApiErrorResponse(404) + async deleteRole( + req: AuthenticatedRequest, + _res: Response, + @Param('slug') slug: string, + @Query query: RoleDeleteQueryDto, + ): Promise { + const role = await this.roleService.getRole(slug); + if (!isPublicRole(role)) { + throw new NotFoundError('Role not found'); + } + + const apiKeyScopes = req.tokenGrant?.apiKeyScopes ?? []; + + assertCanManageRoleType({ + apiKeyScopes, + roleType: role.roleType, + user: req.user, + }); + + const reassignRoleSlug = canReassignUsers({ apiKeyScopes, role, user: req.user }) + ? query.reassignRoleSlug + : undefined; + + const result = await this.roleService.removeCustomRole({ + slug, + reassignRoleSlug, + userId: req.user.id, + }); + + return toRolePublicDto({ ...result, roleType: role.roleType }); + } } diff --git a/packages/cli/src/public-api/v1/handlers/roles/spec/paths/deleteRole.generated.yml b/packages/cli/src/public-api/v1/handlers/roles/spec/paths/deleteRole.generated.yml new file mode 100644 index 00000000000..0057fe00582 --- /dev/null +++ b/packages/cli/src/public-api/v1/handlers/roles/spec/paths/deleteRole.generated.yml @@ -0,0 +1,42 @@ +operationId: deleteRole +tags: + - Role +summary: Delete a custom role +description: Deletes a custom role. System roles cannot be deleted. A role with users assigned cannot be deleted unless `reassignRoleSlug` is set to move those users to another role first. +x-required-scope: role:manage,role:manageProject +x-eov-operation-id: unreachable +x-eov-operation-handler: v1/handlers/decorator-routed.handler +x-decorator-routed: true +parameters: + - schema: + type: string + required: true + name: slug + in: path + - schema: + anyOf: + - type: string + enum: + - global:admin + - global:member + - global:chatUser + - type: string + minLength: 1 + required: false + name: reassignRoleSlug + in: query +responses: + '200': + description: Operation successful. + content: + application/json: + schema: + $ref: ../../../../shared/spec/schemas/rolePublicDto.generated.yml + '400': + $ref: ../../../../shared/spec/responses/badRequest.yml + '401': + $ref: ../../../../shared/spec/responses/unauthorized.yml + '403': + $ref: ../../../../shared/spec/responses/forbidden.yml + '404': + $ref: ../../../../shared/spec/responses/notFound.yml diff --git a/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml b/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml index a3dab91c2b8..ef42e527503 100644 --- a/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml +++ b/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml @@ -16,6 +16,8 @@ paths: $ref: ./handlers/roles/spec/paths/getRole.generated.yml put: $ref: ./handlers/roles/spec/paths/updateRole.generated.yml + delete: + $ref: ./handlers/roles/spec/paths/deleteRole.generated.yml /tags: get: $ref: ./handlers/tags/spec/paths/getTags.generated.yml diff --git a/packages/cli/src/services/__tests__/role-authorization.test.ts b/packages/cli/src/services/__tests__/role-authorization.test.ts index 0fb2a5fdf91..51becdf94e2 100644 --- a/packages/cli/src/services/__tests__/role-authorization.test.ts +++ b/packages/cli/src/services/__tests__/role-authorization.test.ts @@ -9,24 +9,68 @@ describe('assertCanManageRoleType', () => { const userWithScopes = (...scopes: Scope[]) => mock({ role: { scopes: scopes.map((slug) => ({ slug })) } }); - it('allows any role type for a user with role:manage', () => { - const user = userWithScopes('role:manage'); + describe('without apiKeyScopes (Internal API - user scopes only)', () => { + it('allows any role type for a user with role:manage', () => { + const user = userWithScopes('role:manage'); - expect(() => assertCanManageRoleType(user, 'global')).not.toThrow(); - expect(() => assertCanManageRoleType(user, 'project')).not.toThrow(); + expect(() => assertCanManageRoleType({ user, roleType: 'global' })).not.toThrow(); + expect(() => assertCanManageRoleType({ user, roleType: 'project' })).not.toThrow(); + }); + + it('allows only project roles for a user with role:manageProject', () => { + const user = userWithScopes('role:manageProject'); + + expect(() => assertCanManageRoleType({ user, roleType: 'project' })).not.toThrow(); + expect(() => assertCanManageRoleType({ user, roleType: 'global' })).toThrow(ForbiddenError); + }); + + it('rejects a user with neither scope', () => { + const user = userWithScopes('role:read'); + + expect(() => assertCanManageRoleType({ user, roleType: 'project' })).toThrow(ForbiddenError); + expect(() => assertCanManageRoleType({ user, roleType: 'global' })).toThrow(ForbiddenError); + }); }); - it('allows only project roles for a user with role:manageProject', () => { - const user = userWithScopes('role:manageProject'); - - expect(() => assertCanManageRoleType(user, 'project')).not.toThrow(); - expect(() => assertCanManageRoleType(user, 'global')).toThrow(ForbiddenError); - }); - - it('rejects a user with neither scope', () => { + describe('with apiKeyScopes (Public API - decided entirely by the API key)', () => { const user = userWithScopes('role:read'); - expect(() => assertCanManageRoleType(user, 'project')).toThrow(ForbiddenError); - expect(() => assertCanManageRoleType(user, 'global')).toThrow(ForbiddenError); + it('allows a global role when the API key holds role:manage', () => { + expect(() => + assertCanManageRoleType({ user, roleType: 'global', apiKeyScopes: ['role:manage'] }), + ).not.toThrow(); + }); + + it('allows a project role when the API key holds role:manageProject', () => { + expect(() => + assertCanManageRoleType({ + user, + roleType: 'project', + apiKeyScopes: ['role:manageProject'], + }), + ).not.toThrow(); + }); + + it('rejects a global role when the API key is scoped down to role:manageProject only', () => { + expect(() => + assertCanManageRoleType({ + user, + roleType: 'global', + apiKeyScopes: ['role:manageProject'], + }), + ).toThrow(ForbiddenError); + }); + + it('rejects when the API key has no scopes at all', () => { + expect(() => + assertCanManageRoleType({ user, roleType: 'project', apiKeyScopes: [] }), + ).toThrow(ForbiddenError); + }); + + it('rejects when the API key holds neither role-management scope', () => { + expect(() => + assertCanManageRoleType({ user, roleType: 'project', apiKeyScopes: ['role:read'] }), + ).toThrow(ForbiddenError); + }); }); }); diff --git a/packages/cli/src/services/role-authorization.ts b/packages/cli/src/services/role-authorization.ts index 4f83cbac50f..9212d263612 100644 --- a/packages/cli/src/services/role-authorization.ts +++ b/packages/cli/src/services/role-authorization.ts @@ -1,15 +1,50 @@ import type { User } from '@n8n/db'; -import { hasGlobalScope, type RoleNamespace } from '@n8n/permissions'; +import { getAuthPrincipalScopes, type Role as RoleDTO, type RoleNamespace } from '@n8n/permissions'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; /** - * Managing a role requires `role:manage`; `role:manageProject` grants it for - * project roles only. Shared by the internal and public API controllers. + * Whether a caller can manage a role requires `role:manage`; `role:manageProject` grants it for + * project roles only. */ -export function assertCanManageRoleType(user: User, roleType: RoleNamespace): void { - if (hasGlobalScope(user, 'role:manage')) return; - if (roleType === 'project' && hasGlobalScope(user, 'role:manageProject')) return; - throw new ForbiddenError(RESPONSE_ERROR_MESSAGES.MISSING_SCOPE); +export function assertCanManageRoleType({ + apiKeyScopes, + roleType, + user, +}: { + apiKeyScopes?: readonly string[]; + roleType: RoleNamespace; + user: User; +}): void { + const canManage = (scopes: readonly string[]) => + scopes.includes('role:manage') || + (roleType === 'project' && scopes.includes('role:manageProject')); + + const scopes = apiKeyScopes ?? getAuthPrincipalScopes(user); + + if (!canManage(scopes)) { + throw new ForbiddenError(RESPONSE_ERROR_MESSAGES.MISSING_SCOPE); + } +} + +/** + * Whether the caller may reassign a global role's users to another role before deleting it. + */ +export function canReassignUsers({ + apiKeyScopes, + role, + user, +}: { + apiKeyScopes?: readonly string[]; + role: RoleDTO; + user: User; +}): boolean { + if (role.roleType !== 'global' || user.role.slug === role.slug) { + return false; + } + + const scopes = apiKeyScopes ?? getAuthPrincipalScopes(user); + + return scopes.includes('role:manage') && scopes.includes('user:changeRole'); } diff --git a/packages/cli/src/services/role.service.ts b/packages/cli/src/services/role.service.ts index 70cc4e96702..a9c540f8262 100644 --- a/packages/cli/src/services/role.service.ts +++ b/packages/cli/src/services/role.service.ts @@ -141,7 +141,15 @@ export class RoleService { return { members }; } - async removeCustomRole(slug: string, reassignRoleSlug?: string) { + async removeCustomRole({ + slug, + reassignRoleSlug, + userId, + }: { + slug: string; + reassignRoleSlug?: string; + userId: string; + }) { const role = await this.roleRepository.findBySlug(slug); if (!role) { throw new NotFoundError('Role not found'); @@ -172,7 +180,14 @@ export class RoleService { // Invalidate cache after role deletion await this.roleCacheService.invalidateCache(); - return this.dbRoleToRoleDTO(role); + const result = this.dbRoleToRoleDTO(role); + + this.eventService.emit('custom-role-deleted', { + userId, + roleSlug: result.slug, + }); + + return result; } private async reassignUsersAndRemoveRole(role: Role, reassignRoleSlug: string) { diff --git a/packages/cli/test/integration/controllers/role.controller.test.ts b/packages/cli/test/integration/controllers/role.controller.test.ts index 7cbf0e1459c..0b33cda5f83 100644 --- a/packages/cli/test/integration/controllers/role.controller.test.ts +++ b/packages/cli/test/integration/controllers/role.controller.test.ts @@ -15,12 +15,14 @@ describe('RoleController', () => { const testServer = setupTestServer({ endpointGroups: ['role'] }); let ownerAgent: SuperAgentTest; let memberAgent: SuperAgentTest; + let ownerId: string; beforeAll(async () => { const owner = await createOwner(); const member = await createMember(); ownerAgent = testServer.authAgentFor(owner); memberAgent = testServer.authAgentFor(member); + ownerId = owner.id; }); beforeEach(() => { @@ -1160,7 +1162,11 @@ describe('RoleController', () => { // // ASSERT // - expect(roleService.removeCustomRole).toHaveBeenCalledWith(roleSlug, 'global:member'); + expect(roleService.removeCustomRole).toHaveBeenCalledWith({ + slug: roleSlug, + reassignRoleSlug: 'global:member', + userId: ownerId, + }); }); it('should reject reassignment to the owner role', async () => { @@ -1197,7 +1203,11 @@ describe('RoleController', () => { // // ASSERT // - expect(roleService.removeCustomRole).toHaveBeenCalledWith(roleSlug, undefined); + expect(roleService.removeCustomRole).toHaveBeenCalledWith({ + slug: roleSlug, + reassignRoleSlug: undefined, + userId: ownerId, + }); }); it('should handle service errors gracefully', async () => { diff --git a/packages/cli/test/integration/public-api/roles.test.ts b/packages/cli/test/integration/public-api/roles.test.ts index 4ac78e6a426..b22b5775571 100644 --- a/packages/cli/test/integration/public-api/roles.test.ts +++ b/packages/cli/test/integration/public-api/roles.test.ts @@ -3,7 +3,12 @@ import { RoleRepository, type User } from '@n8n/db'; import { Container } from '@n8n/di'; import { createCustomRoleWithScopeSlugs, createRole } from '@test-integration/db/roles'; -import { addApiKey, createOwnerWithApiKey, createUser } from '@test-integration/db/users'; +import { + addApiKey, + createOwnerWithApiKey, + createUser, + getUserById, +} from '@test-integration/db/users'; import { setupTestServer } from '@test-integration/utils'; describe('Roles in Public API', () => { @@ -24,6 +29,23 @@ describe('Roles in Public API', () => { return testServer.publicApiAgentFor(user); }; + const createGlobalRole = async ( + displayName: string, + scopes: string[] = ['user:read'], + ): Promise<{ + slug: string; + displayName: string; + description: string | null; + scopes: string[]; + }> => { + const response = await testServer + .publicApiAgentFor(owner) + .post('/roles') + .send({ displayName, roleType: 'global', scopes }); + expect(response.status).toBe(201); + return response.body; + }; + beforeAll(async () => { await testDb.init(); }); @@ -320,6 +342,24 @@ describe('Roles in Public API', () => { expect(response.status).toBe(403); }); + it('forbids creating a global role when the owner API key is scoped down to role:manageProject only (403)', async () => { + const scopedOwner = await createOwnerWithApiKey({ scopes: ['role:manageProject'] }); + + const response = await testServer + .publicApiAgentFor(scopedOwner) + .post('/roles') + .send({ + displayName: 'PA global role via scoped key', + roleType: 'global', + scopes: ['user:read'], + }); + + expect(response.status).toBe(403); + expect(response.body.message).toEqual( + 'User is missing a scope required to perform this action', + ); + }); + it('rejects an unknown scope slug with 400', async () => { const response = await testServer .publicApiAgentFor(owner) @@ -416,18 +456,6 @@ describe('Roles in Public API', () => { scopes: string[]; }; - const createGlobalRole = async ( - displayName: string, - scopes: string[] = ['user:read'], - ): Promise => { - const response = await testServer - .publicApiAgentFor(owner) - .post('/roles') - .send({ displayName, roleType: 'global', scopes }); - expect(response.status).toBe(201); - return response.body; - }; - const fullBody = (overrides: Partial = {}) => ({ displayName: 'PA updated role', description: null, @@ -566,6 +594,21 @@ describe('Roles in Public API', () => { expect(response.status).toBe(403); }); + it('forbids updating a global role when the owner API key is scoped down to role:manageProject only (403)', async () => { + const role = await createGlobalRole('PA global role for scoped-key put test'); + const scopedOwner = await createOwnerWithApiKey({ scopes: ['role:manageProject'] }); + + const response = await testServer + .publicApiAgentFor(scopedOwner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'Should not work via scoped key' })); + + expect(response.status).toBe(403); + expect(response.body.message).toEqual( + 'User is missing a scope required to perform this action', + ); + }); + it('returns 404 for an unknown slug', async () => { const response = await testServer .publicApiAgentFor(owner) @@ -693,4 +736,247 @@ describe('Roles in Public API', () => { testServer.license.enable('feat:customRoles'); }); }); + + describe('DELETE /roles/{slug}', () => { + it('deletes a custom global role, returns its full public shape, and the slug becomes unreachable', async () => { + const role = await createGlobalRole('PA delete all fields', ['user:read', 'user:list']); + + const response = await testServer.publicApiAgentFor(owner).delete(`/roles/${role.slug}`); + + expect(response.status).toBe(200); + // Full-shape assertion also proves no extra fields leak (e.g. licensed, usedByUsers). + expect(response.body).toEqual({ + slug: role.slug, + displayName: 'PA delete all fields', + description: null, + systemRole: false, + roleType: 'global', + scopes: expect.arrayContaining(['user:read', 'user:list']), + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); + + const getResponse = await testServer.publicApiAgentFor(owner).get(`/roles/${role.slug}`); + expect(getResponse.status).toBe(404); + }); + + it('deletes a custom project role', async () => { + const created = await testServer + .publicApiAgentFor(owner) + .post('/roles') + .send({ + displayName: 'PA delete project role', + roleType: 'project', + scopes: ['workflow:read'], + }); + expect(created.status).toBe(201); + + const response = await testServer + .publicApiAgentFor(owner) + .delete(`/roles/${created.body.slug}`); + + expect(response.status).toBe(200); + expect(response.body.roleType).toBe('project'); + }); + + it('lets a role:manageProject key delete a project role (200)', async () => { + const agent = await makeManageProjectUserAgent(); + const created = await agent.post('/roles').send({ + displayName: 'MP delete project role', + roleType: 'project', + scopes: ['workflow:read'], + }); + expect(created.status).toBe(201); + + const response = await agent.delete(`/roles/${created.body.slug}`); + + expect(response.status).toBe(200); + expect(response.body.slug).toBe(created.body.slug); + }); + + it('forbids a role:manageProject key from deleting a global role (403)', async () => { + const role = await createGlobalRole('PA global role for MP delete test'); + const agent = await makeManageProjectUserAgent(); + + const response = await agent.delete(`/roles/${role.slug}`); + + expect(response.status).toBe(403); + }); + + it('forbids deleting a global role when the owner API key is scoped down to role:manageProject only (403)', async () => { + const role = await createGlobalRole('PA global role for scoped-key delete test'); + const scopedOwner = await createOwnerWithApiKey({ scopes: ['role:manageProject'] }); + + const response = await testServer + .publicApiAgentFor(scopedOwner) + .delete(`/roles/${role.slug}`); + + expect(response.status).toBe(403); + expect(response.body.message).toEqual( + 'User is missing a scope required to perform this action', + ); + }); + + it('rejects deleting a role assigned to users without reassignRoleSlug (400)', async () => { + const roleInUse = await createCustomRoleWithScopeSlugs(['user:read'], { roleType: 'global' }); + await createUser({ role: roleInUse }); + + const response = await testServer.publicApiAgentFor(owner).delete(`/roles/${roleInUse.slug}`); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Cannot delete role assigned to users'); + }); + + it('reassigns users to another role and deletes it, when the caller and the API key both hold user:changeRole', async () => { + const roleInUse = await createCustomRoleWithScopeSlugs(['user:read'], { roleType: 'global' }); + const targetRole = await createCustomRoleWithScopeSlugs(['user:read'], { + roleType: 'global', + }); + const user = await createUser({ role: roleInUse }); + + const response = await testServer + .publicApiAgentFor(owner) + .delete(`/roles/${roleInUse.slug}`) + .query({ reassignRoleSlug: targetRole.slug }); + + expect(response.status).toBe(200); + expect(response.body.slug).toBe(roleInUse.slug); + + const reassignedUser = await getUserById(user.id); + expect(reassignedUser.role.slug).toBe(targetRole.slug); + + const getResponse = await testServer.publicApiAgentFor(owner).get(`/roles/${roleInUse.slug}`); + expect(getResponse.status).toBe(404); + }); + + it('ignores the reassignment target when the API key lacks user:changeRole (400)', async () => { + const roleInUse = await createCustomRoleWithScopeSlugs(['user:read'], { roleType: 'global' }); + const targetRole = await createCustomRoleWithScopeSlugs(['user:read'], { + roleType: 'global', + }); + await createUser({ role: roleInUse }); + // The owner user holds user:changeRole, but this key was granted only role scopes. + const scopedOwner = await createOwnerWithApiKey({ + scopes: ['role:manage', 'role:manageProject', 'role:list', 'role:read'], + }); + + const response = await testServer + .publicApiAgentFor(scopedOwner) + .delete(`/roles/${roleInUse.slug}`) + .query({ reassignRoleSlug: targetRole.slug }); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Cannot delete role assigned to users'); + }); + + it('ignores the reassignment target when the caller holds the role being deleted (400)', async () => { + const roleInUse = await createCustomRoleWithScopeSlugs( + ['role:manage', 'user:changeRole', 'role:read'], + { roleType: 'global' }, + ); + const targetRole = await createCustomRoleWithScopeSlugs(['user:read'], { + roleType: 'global', + }); + const caller = await createUser({ role: roleInUse }); + caller.apiKeys = [await addApiKey(caller, { scopes: ['role:manage', 'user:changeRole'] })]; + + const response = await testServer + .publicApiAgentFor(caller) + .delete(`/roles/${roleInUse.slug}`) + .query({ reassignRoleSlug: targetRole.slug }); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Cannot delete role assigned to users'); + }); + + it('rejects reassigning users onto the role being deleted with 400', async () => { + const roleInUse = await createCustomRoleWithScopeSlugs(['user:read'], { roleType: 'global' }); + await createUser({ role: roleInUse }); + + const response = await testServer + .publicApiAgentFor(owner) + .delete(`/roles/${roleInUse.slug}`) + .query({ reassignRoleSlug: roleInUse.slug }); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Cannot reassign users to the role being deleted'); + }); + + it('rejects a reassignRoleSlug of global:owner with 400', async () => { + const roleInUse = await createCustomRoleWithScopeSlugs(['user:read'], { roleType: 'global' }); + await createUser({ role: roleInUse }); + + const response = await testServer + .publicApiAgentFor(owner) + .delete(`/roles/${roleInUse.slug}`) + .query({ reassignRoleSlug: 'global:owner' }); + + expect(response.status).toBe(400); + }); + + it('returns 404 for an unknown slug', async () => { + const response = await testServer + .publicApiAgentFor(owner) + .delete('/roles/global:does-not-exist'); + + expect(response.status).toBe(404); + }); + + it('returns 404 for a role type not exposed by the public API (e.g. credential)', async () => { + const credentialRole = await createRole({ roleType: 'credential', scopes: [] }); + + const response = await testServer + .publicApiAgentFor(owner) + .delete(`/roles/${credentialRole.slug}`); + + expect(response.status).toBe(404); + }); + + it('rejects deleting a system role with 400', async () => { + const response = await testServer.publicApiAgentFor(owner).delete('/roles/global:owner'); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Cannot delete system roles'); + }); + + it('rejects with 401 without an API key', async () => { + const role = await createGlobalRole('PA delete no key'); + + const response = await testServer.publicApiAgentWithoutApiKey().delete(`/roles/${role.slug}`); + + expect(response.status).toBe(401); + }); + + it('rejects with 401 with an invalid API key', async () => { + const role = await createGlobalRole('PA delete bad key'); + + const response = await testServer + .publicApiAgentWithApiKey('invalid-key') + .delete(`/roles/${role.slug}`); + + expect(response.status).toBe(401); + }); + + it('rejects with 403 when the key lacks a role scope', async () => { + const role = await createGlobalRole('PA delete no scope'); + const scopedOwner = await createOwnerWithApiKey({ scopes: ['user:read'] }); + + const response = await testServer + .publicApiAgentFor(scopedOwner) + .delete(`/roles/${role.slug}`); + + expect(response.status).toBe(403); + }); + + it('rejects with 403 when the custom roles feature is not licensed', async () => { + const role = await createGlobalRole('PA delete unlicensed'); + testServer.license.disable('feat:customRoles'); + + const response = await testServer.publicApiAgentFor(owner).delete(`/roles/${role.slug}`); + + expect(response.status).toBe(403); + + testServer.license.enable('feat:customRoles'); + }); + }); }); diff --git a/packages/cli/test/integration/services/role.service.test.ts b/packages/cli/test/integration/services/role.service.test.ts index 83d8c9a6ea5..9688d71e6f8 100644 --- a/packages/cli/test/integration/services/role.service.test.ts +++ b/packages/cli/test/integration/services/role.service.test.ts @@ -1272,7 +1272,10 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.removeCustomRole(customRole.slug); + const result = await roleService.removeCustomRole({ + slug: customRole.slug, + userId: 'test-user-id', + }); // // ASSERT @@ -1301,7 +1304,10 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.removeCustomRole(customRole.slug); + const result = await roleService.removeCustomRole({ + slug: customRole.slug, + userId: 'test-user-id', + }); // // ASSERT @@ -1326,8 +1332,12 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.removeCustomRole(nonExistentSlug)).rejects.toThrow(NotFoundError); - await expect(roleService.removeCustomRole(nonExistentSlug)).rejects.toThrow('Role not found'); + await expect( + roleService.removeCustomRole({ slug: nonExistentSlug, userId: 'test-user-id' }), + ).rejects.toThrow(NotFoundError); + await expect( + roleService.removeCustomRole({ slug: nonExistentSlug, userId: 'test-user-id' }), + ).rejects.toThrow('Role not found'); }); it('should throw BadRequestError when trying to delete system role', async () => { @@ -1341,10 +1351,12 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.removeCustomRole(systemRole.slug)).rejects.toThrow(BadRequestError); - await expect(roleService.removeCustomRole(systemRole.slug)).rejects.toThrow( - 'Cannot delete system roles', - ); + await expect( + roleService.removeCustomRole({ slug: systemRole.slug, userId: 'test-user-id' }), + ).rejects.toThrow(BadRequestError); + await expect( + roleService.removeCustomRole({ slug: systemRole.slug, userId: 'test-user-id' }), + ).rejects.toThrow('Cannot delete system roles'); // Verify system role still exists const stillExistsRole = await roleRepository.findBySlug(systemRole.slug); @@ -1370,10 +1382,12 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.removeCustomRole(roleInUse.slug)).rejects.toThrow(BadRequestError); - await expect(roleService.removeCustomRole(roleInUse.slug)).rejects.toThrow( - 'Cannot delete role assigned to users', - ); + await expect( + roleService.removeCustomRole({ slug: roleInUse.slug, userId: 'test-user-id' }), + ).rejects.toThrow(BadRequestError); + await expect( + roleService.removeCustomRole({ slug: roleInUse.slug, userId: 'test-user-id' }), + ).rejects.toThrow('Cannot delete role assigned to users'); }); it('should throw error when trying to delete role assigned to users on project', async () => { @@ -1401,10 +1415,12 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.removeCustomRole(roleInUse.slug)).rejects.toThrow(BadRequestError); - await expect(roleService.removeCustomRole(roleInUse.slug)).rejects.toThrow( - 'Cannot delete role assigned to users', - ); + await expect( + roleService.removeCustomRole({ slug: roleInUse.slug, userId: 'test-user-id' }), + ).rejects.toThrow(BadRequestError); + await expect( + roleService.removeCustomRole({ slug: roleInUse.slug, userId: 'test-user-id' }), + ).rejects.toThrow('Cannot delete role assigned to users'); }); it('should reassign globally assigned users to another role, then delete', async () => { @@ -1429,7 +1445,11 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.removeCustomRole(roleInUse.slug, targetRole.slug); + const result = await roleService.removeCustomRole({ + slug: roleInUse.slug, + reassignRoleSlug: targetRole.slug, + userId: 'test-user-id', + }); // // ASSERT @@ -1455,7 +1475,11 @@ describe('RoleService', () => { await userRepository.save(user); await expect( - roleService.removeCustomRole(roleInUse.slug, 'global:does-not-exist'), + roleService.removeCustomRole({ + slug: roleInUse.slug, + reassignRoleSlug: 'global:does-not-exist', + userId: 'test-user-id', + }), ).rejects.toThrow('Reassignment role "global:does-not-exist" does not exist'); // Role is preserved when reassignment fails. @@ -1477,7 +1501,11 @@ describe('RoleService', () => { await userRepository.save(user); await expect( - roleService.removeCustomRole(roleInUse.slug, projectTarget.slug), + roleService.removeCustomRole({ + slug: roleInUse.slug, + reassignRoleSlug: projectTarget.slug, + userId: 'test-user-id', + }), ).rejects.toThrow('Reassignment role must be of the same type as the deleted role'); }); @@ -1491,9 +1519,13 @@ describe('RoleService', () => { user.role = roleInUse; await userRepository.save(user); - await expect(roleService.removeCustomRole(roleInUse.slug, roleInUse.slug)).rejects.toThrow( - 'Cannot reassign users to the role being deleted', - ); + await expect( + roleService.removeCustomRole({ + slug: roleInUse.slug, + reassignRoleSlug: roleInUse.slug, + userId: 'test-user-id', + }), + ).rejects.toThrow('Cannot reassign users to the role being deleted'); }); it('should ignore the reassignment role when no users are assigned', async () => { @@ -1508,7 +1540,11 @@ describe('RoleService', () => { systemRole: false, }); - const result = await roleService.removeCustomRole(unusedRole.slug, targetRole.slug); + const result = await roleService.removeCustomRole({ + slug: unusedRole.slug, + reassignRoleSlug: targetRole.slug, + userId: 'test-user-id', + }); expect(result.slug).toBe(unusedRole.slug); expect(await roleRepository.findBySlug(unusedRole.slug)).toBeNull(); @@ -1551,10 +1587,12 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.removeCustomRole(role.slug)).rejects.toThrow(BadRequestError); - await expect(roleService.removeCustomRole(role.slug)).rejects.toThrow( - 'Cannot delete role: referenced by 1 role mapping rule', - ); + await expect( + roleService.removeCustomRole({ slug: role.slug, userId: 'test-user-id' }), + ).rejects.toThrow(BadRequestError); + await expect( + roleService.removeCustomRole({ slug: role.slug, userId: 'test-user-id' }), + ).rejects.toThrow('Cannot delete role: referenced by 1 role mapping rule'); // Role is preserved, not silently orphaned. const stillExists = await roleRepository.findBySlug(role.slug); @@ -1568,7 +1606,10 @@ describe('RoleService', () => { systemRole: false, }); - const result = await roleService.removeCustomRole(role.slug); + const result = await roleService.removeCustomRole({ + slug: role.slug, + userId: 'test-user-id', + }); expect(result.slug).toEqual(role.slug); const deletedRole = await roleRepository.findBySlug(role.slug); diff --git a/packages/nodes-base/nodes/N8n/n8n-api-coverage.json b/packages/nodes-base/nodes/N8n/n8n-api-coverage.json index adc70cc72f1..33da9732209 100644 --- a/packages/nodes-base/nodes/N8n/n8n-api-coverage.json +++ b/packages/nodes-base/nodes/N8n/n8n-api-coverage.json @@ -17,6 +17,9 @@ "PUT /roles/{slug}": { "status": "gap" }, + "DELETE /roles/{slug}": { + "status": "gap" + }, "POST /role-mapping-rules": { "status": "gap" },