From 28bf536d857a6560b63e230169db70549ee78519 Mon Sep 17 00:00:00 2001 From: Sam Wooler Date: Mon, 17 Aug 2026 08:10:24 +0000 Subject: [PATCH] feat(API): Add endpoint to update a custom role (#36213) --- packages/@n8n/api-types/src/dto/index.ts | 1 + .../__tests__/update-role-public.dto.test.ts | 116 +++++++ .../src/dto/roles/update-role-public.dto.ts | 10 + .../__tests__/role.controller.test.ts | 19 -- .../cli/src/controllers/role.controller.ts | 8 +- .../v1/controllers/roles.public.controller.ts | 65 ++-- .../roles/spec/paths/createRole.generated.yml | 36 +-- .../roles/spec/paths/updateRole.generated.yml | 52 ++++ .../v1/openapi.decorator-routes.generated.yml | 2 + .../spec/schemas/rolePublicDto.generated.yml | 35 +++ .../role.service.assignments.test.ts | 3 + .../role.service.customRoles.test.ts | 26 +- .../role.service.rolesWithScope.test.ts | 3 + packages/cli/src/services/role.service.ts | 27 +- .../controllers/role.controller.test.ts | 13 +- .../test/integration/public-api/roles.test.ts | 289 +++++++++++++++++- .../integration/services/role.service.test.ts | 70 +++-- .../nodes/N8n/n8n-api-coverage.json | 3 + 18 files changed, 672 insertions(+), 106 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/roles/__tests__/update-role-public.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/roles/update-role-public.dto.ts create mode 100644 packages/cli/src/public-api/v1/handlers/roles/spec/paths/updateRole.generated.yml create mode 100644 packages/cli/src/public-api/v1/shared/spec/schemas/rolePublicDto.generated.yml diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index e0ddf584990..219af08e88d 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -190,6 +190,7 @@ export { } from './user/users-list-filter.dto'; export { UpdateRoleDto } from './roles/update-role.dto'; +export { UpdateRolePublicDto } from './roles/update-role-public.dto'; export { CreateRoleDto } from './roles/create-role.dto'; export { RolePublicDto, diff --git a/packages/@n8n/api-types/src/dto/roles/__tests__/update-role-public.dto.test.ts b/packages/@n8n/api-types/src/dto/roles/__tests__/update-role-public.dto.test.ts new file mode 100644 index 00000000000..f053e0071ae --- /dev/null +++ b/packages/@n8n/api-types/src/dto/roles/__tests__/update-role-public.dto.test.ts @@ -0,0 +1,116 @@ +import assert from 'node:assert'; + +import { UpdateRolePublicDto } from '../update-role-public.dto'; + +describe('updateRolePublicDtoSchema', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'full catalog of fields', + request: { + displayName: 'Updated Role Name', + description: 'Updated role description', + scopes: ['project:read', 'workflow:execute'], + }, + }, + { + name: 'null description', + request: { + displayName: 'Updated Role Name', + description: null, + scopes: ['project:read'], + }, + }, + { + name: 'empty scopes array (clears all scopes)', + request: { + displayName: 'Updated Role Name', + description: null, + scopes: [], + }, + }, + { + name: 'displayName at minimum length', + request: { + displayName: 'Up', + description: null, + scopes: [], + }, + }, + { + name: 'displayName at maximum length', + request: { + displayName: 'B'.repeat(100), + description: null, + scopes: [], + }, + }, + { + name: 'description at maximum length', + request: { + displayName: 'Updated Role Name', + description: 'C'.repeat(500), + scopes: [], + }, + }, + ])('should validate $name', ({ request }) => { + const result = UpdateRolePublicDto.safeParse(request); + expect(result.success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'missing displayName', + request: { description: null, scopes: [] }, + expectedErrorPath: ['displayName'], + }, + { + name: 'missing description', + request: { displayName: 'Updated Role Name', scopes: [] }, + expectedErrorPath: ['description'], + }, + { + name: 'missing scopes', + request: { displayName: 'Updated Role Name', description: null }, + expectedErrorPath: ['scopes'], + }, + { + name: 'empty request body', + request: {}, + expectedErrorPath: ['displayName'], + }, + { + name: 'displayName too short', + request: { displayName: 'A', description: null, scopes: [] }, + expectedErrorPath: ['displayName'], + }, + { + name: 'displayName too long', + request: { displayName: 'A'.repeat(101), description: null, scopes: [] }, + expectedErrorPath: ['displayName'], + }, + { + name: 'description too long', + request: { displayName: 'Updated Role Name', description: 'A'.repeat(501), scopes: [] }, + expectedErrorPath: ['description'], + }, + { + name: 'invalid scope in array', + request: { + displayName: 'Updated Role Name', + description: null, + scopes: ['not:a-real-scope'], + }, + expectedErrorPath: ['scopes', 0], + }, + ])('should fail validation for $name', ({ request, expectedErrorPath }) => { + const result = UpdateRolePublicDto.safeParse(request); + + assert(!result.success, 'Expected validation to fail'); + + expect(result.error.issues[0].path).toEqual(expectedErrorPath); + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/roles/update-role-public.dto.ts b/packages/@n8n/api-types/src/dto/roles/update-role-public.dto.ts new file mode 100644 index 00000000000..ef26b8ff635 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/roles/update-role-public.dto.ts @@ -0,0 +1,10 @@ +import { scopeSchema } from '@n8n/permissions'; +import { z } from 'zod'; + +import { Z } from '../../zod-class'; + +export class UpdateRolePublicDto extends Z.class({ + displayName: z.string().min(2).max(100), + description: z.string().max(500).nullable(), + scopes: z.array(scopeSchema), +}) {} diff --git a/packages/cli/src/controllers/__tests__/role.controller.test.ts b/packages/cli/src/controllers/__tests__/role.controller.test.ts index e5ee85ba6f0..cc6a5a1bdd0 100644 --- a/packages/cli/src/controllers/__tests__/role.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/role.controller.test.ts @@ -40,25 +40,6 @@ describe('RoleController', () => { }); }); - describe('updateRole', () => { - it('should emit custom-role-updated', async () => { - const request = managerRequest(); - roleService.getRole.mockResolvedValue({ roleType: 'project' } as Role); - roleService.updateCustomRole.mockResolvedValue({ - slug: 'custom-editor', - scopes: ['workflow:read', 'workflow:update', 'workflow:delete'], - } as Role); - - await controller.updateRole(request, mock(), 'custom-editor', mock()); - - expect(eventService.emit).toHaveBeenCalledWith('custom-role-updated', { - userId: '123', - roleSlug: 'custom-editor', - scopes: ['workflow:read', 'workflow:update', 'workflow:delete'], - }); - }); - }); - describe('deleteRole', () => { it('should emit custom-role-deleted', async () => { const request = managerRequest(); diff --git a/packages/cli/src/controllers/role.controller.ts b/packages/cli/src/controllers/role.controller.ts index 59a0d26a126..38a7e86641b 100644 --- a/packages/cli/src/controllers/role.controller.ts +++ b/packages/cli/src/controllers/role.controller.ts @@ -128,13 +128,11 @@ export class RoleController { ): Promise { const role = await this.roleService.getRole(slug); assertCanManageRoleType(req.user, role.roleType); - const result = await this.roleService.updateCustomRole(slug, updateRole); - this.eventService.emit('custom-role-updated', { + return await this.roleService.updateCustomRole({ + slug, + newRole: updateRole, userId: req.user.id, - roleSlug: result.slug, - scopes: result.scopes, }); - return result; } @Delete('/:slug') 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 a8386e033b5..75747d40aa9 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 @@ -4,6 +4,7 @@ import { RoleListPublicDto, RoleListQueryPublicDto, RolePublicDto, + UpdateRolePublicDto, } from '@n8n/api-types'; import { LICENSE_FEATURES } from '@n8n/constants'; import { AuthenticatedRequest } from '@n8n/db'; @@ -20,6 +21,7 @@ import { Param, Post, PublicApiController, + Put, Query, } from '@n8n/decorators'; import { RoleNamespace, type Role as RoleDTO } from '@n8n/permissions'; @@ -34,19 +36,23 @@ type PublicRoleNamespace = Extract; const isPublicRole = (role: RoleDTO): role is RoleDTO & { roleType: PublicRoleNamespace } => role.roleType === 'global' || role.roleType === 'project'; -const toPublicRole = ( - role: RoleDTO & { roleType: T }, - withUsageCount = false, -) => ({ +const toRolePublicDto = (role: RoleDTO & { roleType: PublicRoleNamespace }): RolePublicDto => ({ slug: role.slug, displayName: role.displayName, description: role.description, systemRole: role.systemRole, roleType: role.roleType, - licensed: role.licensed, scopes: role.scopes, createdAt: role.createdAt!.toISOString(), updatedAt: role.updatedAt!.toISOString(), +}); + +const toRoleGetPublicDto = ( + role: RoleDTO & { roleType: PublicRoleNamespace }, + withUsageCount: boolean, +): RoleGetPublicDto => ({ + ...toRolePublicDto(role), + licensed: role.licensed, ...(withUsageCount ? { usedByUsers: role.usedByUsers, usedByProjects: role.usedByProjects } : {}), }); @@ -77,7 +83,10 @@ export class RolesPublicController { const groupOf = (roleType: T) => publicRoles .filter((role): role is RoleDTO & { roleType: T } => role.roleType === roleType) - .map((role) => toPublicRole(role, withUsageCount)); + .map((role) => ({ + ...toRoleGetPublicDto({ ...role, roleType }, withUsageCount), + roleType, + })); return { global: groupOf('global'), @@ -105,7 +114,7 @@ export class RolesPublicController { if (!isPublicRole(role)) { throw new NotFoundError('Role not found'); } - return toPublicRole(role, withUsageCount); + return toRoleGetPublicDto({ ...role, roleType: role.roleType }, withUsageCount); } @Post('/') @@ -132,15 +141,37 @@ export class RolesPublicController { scopes: role.scopes, }); - return { - slug: role.slug, - displayName: role.displayName, - description: role.description, - systemRole: role.systemRole, - roleType: createRole.roleType, - scopes: role.scopes, - createdAt: role.createdAt!.toISOString(), - updatedAt: role.updatedAt!.toISOString(), - }; + return toRolePublicDto({ ...role, roleType: createRole.roleType }); + } + + @Put('/:slug') + @ApiKeyScope({ anyOf: ['role:manage', 'role:manageProject'] }) + @Licensed(LICENSE_FEATURES.CUSTOM_ROLES) + @ApiSummary('Update a custom role') + @ApiDescription( + "Replaces a custom role's display name, description, and scopes. System roles cannot be updated.", + ) + @ApiTags(['Role']) + @ApiResponse(200, RolePublicDto) + @ApiErrorResponse(404) + async updateRole( + req: AuthenticatedRequest, + _res: Response, + @Param('slug') slug: string, + @Body updateRole: UpdateRolePublicDto, + ): Promise { + const role = await this.roleService.getRole(slug); + if (!isPublicRole(role)) { + throw new NotFoundError('Role not found'); + } + assertCanManageRoleType(req.user, role.roleType); + + const result = await this.roleService.updateCustomRole({ + slug, + newRole: updateRole, + userId: req.user.id, + }); + + return toRolePublicDto({ ...result, roleType: role.roleType }); } } diff --git a/packages/cli/src/public-api/v1/handlers/roles/spec/paths/createRole.generated.yml b/packages/cli/src/public-api/v1/handlers/roles/spec/paths/createRole.generated.yml index 0fdda6001e6..dd7c4f15377 100644 --- a/packages/cli/src/public-api/v1/handlers/roles/spec/paths/createRole.generated.yml +++ b/packages/cli/src/public-api/v1/handlers/roles/spec/paths/createRole.generated.yml @@ -39,41 +39,7 @@ responses: content: application/json: schema: - type: object - properties: - slug: - type: string - displayName: - type: string - description: - type: string - nullable: true - systemRole: - type: boolean - roleType: - type: string - enum: - - project - - global - scopes: - type: array - items: - type: string - createdAt: - type: string - format: date-time - updatedAt: - type: string - format: date-time - required: - - slug - - displayName - - description - - systemRole - - roleType - - scopes - - createdAt - - updatedAt + $ref: ../../../../shared/spec/schemas/rolePublicDto.generated.yml '400': $ref: ../../../../shared/spec/responses/badRequest.yml '401': diff --git a/packages/cli/src/public-api/v1/handlers/roles/spec/paths/updateRole.generated.yml b/packages/cli/src/public-api/v1/handlers/roles/spec/paths/updateRole.generated.yml new file mode 100644 index 00000000000..a0ade8d629c --- /dev/null +++ b/packages/cli/src/public-api/v1/handlers/roles/spec/paths/updateRole.generated.yml @@ -0,0 +1,52 @@ +operationId: updateRole +tags: + - Role +summary: Update a custom role +description: Replaces a custom role's display name, description, and scopes. System roles cannot be updated. +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 +requestBody: + content: + application/json: + schema: + type: object + properties: + displayName: + type: string + minLength: 2 + maxLength: 100 + description: + type: string + nullable: true + maxLength: 500 + scopes: + type: array + items: + type: string + required: + - displayName + - description + - scopes +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 ca20e142d10..a3dab91c2b8 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 @@ -14,6 +14,8 @@ paths: /roles/{slug}: get: $ref: ./handlers/roles/spec/paths/getRole.generated.yml + put: + $ref: ./handlers/roles/spec/paths/updateRole.generated.yml /tags: get: $ref: ./handlers/tags/spec/paths/getTags.generated.yml diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/rolePublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/rolePublicDto.generated.yml new file mode 100644 index 00000000000..4ec4ae62ef8 --- /dev/null +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/rolePublicDto.generated.yml @@ -0,0 +1,35 @@ +type: object +properties: + slug: + type: string + displayName: + type: string + description: + type: string + nullable: true + systemRole: + type: boolean + roleType: + type: string + enum: + - project + - global + scopes: + type: array + items: + type: string + createdAt: + type: string + format: date-time + updatedAt: + type: string + format: date-time +required: + - slug + - displayName + - description + - systemRole + - roleType + - scopes + - createdAt + - updatedAt diff --git a/packages/cli/src/services/__tests__/role.service.assignments.test.ts b/packages/cli/src/services/__tests__/role.service.assignments.test.ts index 5c596b21a87..0292a3c6bfd 100644 --- a/packages/cli/src/services/__tests__/role.service.assignments.test.ts +++ b/packages/cli/src/services/__tests__/role.service.assignments.test.ts @@ -5,6 +5,7 @@ import { RoleRepository, ScopeRepository } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; +import { EventService } from '@/events/event.service'; import { RoleCacheService } from '@/services/role-cache.service'; import { RoleDeletionCheckProxy } from '@/services/role-deletion-check-proxy.service'; import { RoleService } from '@/services/role.service'; @@ -16,6 +17,7 @@ describe('RoleService.getRoleAssignments and getRoleProjectMembers', () => { const roleCacheService = mockInstance(RoleCacheService); const logger = mockInstance(Logger); const roleDeletionCheckProxy = mockInstance(RoleDeletionCheckProxy); + const eventService = mockInstance(EventService); const roleService = new RoleService( licenseState, @@ -24,6 +26,7 @@ describe('RoleService.getRoleAssignments and getRoleProjectMembers', () => { roleCacheService, logger, roleDeletionCheckProxy, + eventService, ); beforeEach(() => { diff --git a/packages/cli/src/services/__tests__/role.service.customRoles.test.ts b/packages/cli/src/services/__tests__/role.service.customRoles.test.ts index 187f00663aa..98b91f55748 100644 --- a/packages/cli/src/services/__tests__/role.service.customRoles.test.ts +++ b/packages/cli/src/services/__tests__/role.service.customRoles.test.ts @@ -7,6 +7,7 @@ import { RoleRepository, ScopeRepository } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { EventService } from '@/events/event.service'; import { RoleCacheService } from '@/services/role-cache.service'; import { RoleDeletionCheckProxy } from '@/services/role-deletion-check-proxy.service'; import { RoleService } from '@/services/role.service'; @@ -18,6 +19,7 @@ describe('RoleService custom role scope whitelist', () => { const roleCacheService = mockInstance(RoleCacheService); const logger = mockInstance(Logger); const roleDeletionCheckProxy = mockInstance(RoleDeletionCheckProxy); + const eventService = mockInstance(EventService); const roleService = new RoleService( licenseState, @@ -26,6 +28,7 @@ describe('RoleService custom role scope whitelist', () => { roleCacheService, logger, roleDeletionCheckProxy, + eventService, ); beforeEach(() => { @@ -96,19 +99,32 @@ describe('RoleService custom role scope whitelist', () => { it('rejects updating a project role with a global-only scope', async () => { const dto = { scopes: ['user:create'] } as UpdateRoleDto; - await expect(roleService.updateCustomRole('project:custom-abc123', dto)).rejects.toThrow( - BadRequestError, - ); + await expect( + roleService.updateCustomRole({ + slug: 'project:custom-abc123', + newRole: dto, + userId: 'user-id', + }), + ).rejects.toThrow(BadRequestError); expect(roleRepository.updateRole).not.toHaveBeenCalled(); }); - it('accepts updating a project role with project-scoped scopes', async () => { + it('accepts updating a project role with project-scoped scopes and emits custom-role-updated', async () => { const dto = { scopes: ['workflow:create'] } as UpdateRoleDto; await expect( - roleService.updateCustomRole('project:custom-abc123', dto), + roleService.updateCustomRole({ + slug: 'project:custom-abc123', + newRole: dto, + userId: 'user-id', + }), ).resolves.toBeDefined(); expect(roleRepository.updateRole).toHaveBeenCalled(); + expect(eventService.emit).toHaveBeenCalledWith('custom-role-updated', { + userId: 'user-id', + roleSlug: 'project:custom-abc123', + scopes: [], + }); }); }); }); diff --git a/packages/cli/src/services/__tests__/role.service.rolesWithScope.test.ts b/packages/cli/src/services/__tests__/role.service.rolesWithScope.test.ts index bf7325fe2c6..11c0065122e 100644 --- a/packages/cli/src/services/__tests__/role.service.rolesWithScope.test.ts +++ b/packages/cli/src/services/__tests__/role.service.rolesWithScope.test.ts @@ -4,6 +4,7 @@ import { mockInstance } from '@n8n/backend-test-utils'; import { RoleRepository, ScopeRepository } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; +import { EventService } from '@/events/event.service'; import { RoleCacheService } from '@/services/role-cache.service'; import { RoleDeletionCheckProxy } from '@/services/role-deletion-check-proxy.service'; import { RoleService } from '@/services/role.service'; @@ -15,6 +16,7 @@ describe('RoleService.rolesWithScope', () => { const roleCacheService = mockInstance(RoleCacheService); const logger = mockInstance(Logger); const roleDeletionCheckProxy = mockInstance(RoleDeletionCheckProxy); + const eventService = mockInstance(EventService); const roleService = new RoleService( licenseState, @@ -23,6 +25,7 @@ describe('RoleService.rolesWithScope', () => { roleCacheService, logger, roleDeletionCheckProxy, + eventService, ); beforeEach(() => { diff --git a/packages/cli/src/services/role.service.ts b/packages/cli/src/services/role.service.ts index 374798cbaa6..70cc4e96702 100644 --- a/packages/cli/src/services/role.service.ts +++ b/packages/cli/src/services/role.service.ts @@ -3,7 +3,7 @@ import type { RoleMembersResponse, RoleProjectMembersResponse, } from '@n8n/api-types'; -import { CreateRoleDto, UpdateRoleDto } from '@n8n/api-types'; +import { CreateRoleDto } from '@n8n/api-types'; import { LicenseState, Logger } from '@n8n/backend-common'; import { CredentialsEntity, @@ -42,6 +42,7 @@ import { UnexpectedError, UserError } from 'n8n-workflow'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; +import { EventService } from '@/events/event.service'; import { isUniqueConstraintError } from '@/response-helper'; import { RoleCacheService } from './role-cache.service'; @@ -56,6 +57,7 @@ export class RoleService { private readonly roleCacheService: RoleCacheService, private readonly logger: Logger, private readonly roleDeletionCheckProxy: RoleDeletionCheckProxy, + private readonly eventService: EventService, ) {} private dbRoleToRoleDTO(role: Role, usedByUsers?: number, usedByProjects?: number): RoleDTO { @@ -221,8 +223,17 @@ export class RoleService { return scopes; } - async updateCustomRole(slug: string, newData: UpdateRoleDto) { - const { displayName, description, scopes: scopeSlugs } = newData; + async updateCustomRole({ + slug, + newRole, + userId, + }: { + slug: string; + // Optional fields keep this compatible with both the internal PATCH and public PUT endpoints. + newRole: { displayName?: string; description?: string | null; scopes?: string[] }; + userId: string; + }) { + const { displayName, description, scopes: scopeSlugs } = newRole; const roleType = slug.startsWith('project:') ? 'project' : 'global'; @@ -236,7 +247,15 @@ export class RoleService { // Invalidate cache after role update await this.roleCacheService.invalidateCache(); - return this.dbRoleToRoleDTO(updatedRole); + const result = this.dbRoleToRoleDTO(updatedRole); + + this.eventService.emit('custom-role-updated', { + userId, + roleSlug: result.slug, + scopes: result.scopes, + }); + + return result; } catch (error) { if (error instanceof UserError && error.message === 'Role not found') { throw new NotFoundError('Role not found'); diff --git a/packages/cli/test/integration/controllers/role.controller.test.ts b/packages/cli/test/integration/controllers/role.controller.test.ts index 343e3bf876c..7cbf0e1459c 100644 --- a/packages/cli/test/integration/controllers/role.controller.test.ts +++ b/packages/cli/test/integration/controllers/role.controller.test.ts @@ -996,8 +996,11 @@ describe('RoleController', () => { // ASSERT // expect(response.body).toEqual({ data: mockUpdatedRole }); - // Parameter verification skipped - test framework issue - expect(roleService.updateCustomRole).toHaveBeenCalledWith(roleSlug, updateRoleDto); + expect(roleService.updateCustomRole).toHaveBeenCalledWith({ + slug: roleSlug, + newRole: updateRoleDto, + userId: expect.any(String), + }); }); it('should update only provided fields', async () => { @@ -1030,7 +1033,11 @@ describe('RoleController', () => { // ASSERT // expect(response.body).toEqual({ data: mockUpdatedRole }); - expect(roleService.updateCustomRole).toHaveBeenCalledWith(roleSlug, updateRoleDto); + expect(roleService.updateCustomRole).toHaveBeenCalledWith({ + slug: roleSlug, + newRole: updateRoleDto, + userId: expect.any(String), + }); }); 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 7f24edf5f22..4ac78e6a426 100644 --- a/packages/cli/test/integration/public-api/roles.test.ts +++ b/packages/cli/test/integration/public-api/roles.test.ts @@ -1,7 +1,8 @@ import { testDb } from '@n8n/backend-test-utils'; import { RoleRepository, type User } from '@n8n/db'; import { Container } from '@n8n/di'; -import { createCustomRoleWithScopeSlugs } from '@test-integration/db/roles'; + +import { createCustomRoleWithScopeSlugs, createRole } from '@test-integration/db/roles'; import { addApiKey, createOwnerWithApiKey, createUser } from '@test-integration/db/users'; import { setupTestServer } from '@test-integration/utils'; @@ -406,4 +407,290 @@ describe('Roles in Public API', () => { testServer.license.enable('feat:customRoles'); }); }); + + describe('PUT /roles/{slug}', () => { + type CreatedRole = { + slug: string; + displayName: string; + description: string | null; + 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, + scopes: ['user:read'], + ...overrides, + }); + + it('replaces displayName, description, and scopes, returns the full public shape', async () => { + const role = await createGlobalRole('PA put all fields'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send( + fullBody({ + displayName: 'PA put all fields updated', + description: 'An updated description', + scopes: ['user:read', 'user:list'], + }), + ); + + 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 put all fields updated', + description: 'An updated description', + systemRole: false, + roleType: 'global', + scopes: expect.arrayContaining(['user:read', 'user:list']), + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); + expect(response.body.scopes).toHaveLength(2); + }); + + it('replaces scopes entirely rather than merging them with the existing set', async () => { + const role = await createGlobalRole('PA put scopes', ['user:read', 'user:list']); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: role.displayName, scopes: ['user:read'] })); + + expect(response.status).toBe(200); + expect(response.body.scopes).toEqual(['user:read']); + }); + + it('clears an existing description by sending null', async () => { + const created = await testServer + .publicApiAgentFor(owner) + .post('/roles') + .send({ displayName: 'PA put clear description', roleType: 'global', scopes: [] }); + expect(created.status).toBe(201); + const withDescription = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${created.body.slug}`) + .send( + fullBody({ displayName: created.body.displayName, description: 'Has a description' }), + ); + expect(withDescription.body.description).toBe('Has a description'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${created.body.slug}`) + .send(fullBody({ displayName: created.body.displayName, description: null })); + + expect(response.status).toBe(200); + expect(response.body.description).toBeNull(); + }); + + it('accepts a GET response of the role as a PUT body unchanged (round-trip)', async () => { + const role = await createGlobalRole('PA put round trip', ['user:read', 'user:list']); + const getResponse = await testServer.publicApiAgentFor(owner).get('/roles'); + const current = getResponse.body.global.find((r: { slug: string }) => r.slug === role.slug); + + const response = await testServer.publicApiAgentFor(owner).put(`/roles/${role.slug}`).send({ + displayName: current.displayName, + description: current.description, + scopes: current.scopes, + }); + + expect(response.status).toBe(200); + expect(response.body.displayName).toBe(role.displayName); + expect(response.body.description).toBeNull(); + expect(response.body.scopes).toEqual(expect.arrayContaining(['user:read', 'user:list'])); + }); + + it('rejects a body missing a required field with 400', async () => { + const role = await createGlobalRole('PA put missing field'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send({ displayName: 'PA put missing field updated', description: null }); + + expect(response.status).toBe(400); + }); + + it('rejects an empty body with 400', async () => { + const role = await createGlobalRole('PA put empty body'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send({}); + + expect(response.status).toBe(400); + }); + + it('lets a role:manageProject key update a project role (200)', async () => { + const agent = await makeManageProjectUserAgent(); + const created = await agent.post('/roles').send({ + displayName: 'MP put project role', + roleType: 'project', + scopes: ['workflow:read'], + }); + expect(created.status).toBe(201); + + const response = await agent + .put(`/roles/${created.body.slug}`) + .send(fullBody({ displayName: 'MP put project role updated', scopes: ['workflow:read'] })); + + expect(response.status).toBe(200); + expect(response.body.displayName).toBe('MP put project role updated'); + }); + + it('forbids a role:manageProject key from updating a global role (403)', async () => { + const role = await createGlobalRole('PA global role for MP put test'); + const agent = await makeManageProjectUserAgent(); + + const response = await agent + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'MP should not put this' })); + + expect(response.status).toBe(403); + }); + + it('returns 404 for an unknown slug', async () => { + const response = await testServer + .publicApiAgentFor(owner) + .put('/roles/global:does-not-exist') + .send(fullBody()); + + 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) + .put(`/roles/${credentialRole.slug}`) + .send(fullBody({ displayName: 'Should not be reachable' })); + + expect(response.status).toBe(404); + }); + + it('rejects updating a system role with 400', async () => { + const response = await testServer + .publicApiAgentFor(owner) + .put('/roles/global:owner') + .send(fullBody({ displayName: 'Renamed owner role' })); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Cannot update system roles'); + }); + + it('rejects an unknown scope slug with 400', async () => { + const role = await createGlobalRole('PA put bad slug'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: role.displayName, scopes: ['not:a-real-scope'] })); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Invalid scope'); + }); + + it('rejects a scope not allowed for the role type with 400', async () => { + const role = await createGlobalRole('PA put wrong scope'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: role.displayName, scopes: ['workflow:read'] })); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('not allowed for global roles'); + }); + + it('rejects a too-short displayName with 400', async () => { + const role = await createGlobalRole('PA put short name'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'a' })); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('at least 2'); + }); + + it('rejects renaming onto an existing role name with 400', async () => { + await createGlobalRole('PA put existing name'); + const role = await createGlobalRole('PA put rename target'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'PA put existing name' })); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('already exists'); + }); + + it('rejects with 401 without an API key', async () => { + const role = await createGlobalRole('PA put no key'); + + const response = await testServer + .publicApiAgentWithoutApiKey() + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'Should not work' })); + + expect(response.status).toBe(401); + }); + + it('rejects with 401 with an invalid API key', async () => { + const role = await createGlobalRole('PA put bad key'); + + const response = await testServer + .publicApiAgentWithApiKey('invalid-key') + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'Should not work' })); + + expect(response.status).toBe(401); + }); + + it('rejects with 403 when the key lacks a role scope', async () => { + const role = await createGlobalRole('PA put no scope'); + const scopedOwner = await createOwnerWithApiKey({ scopes: ['user:read'] }); + + const response = await testServer + .publicApiAgentFor(scopedOwner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'Should not work' })); + + expect(response.status).toBe(403); + }); + + it('rejects with 403 when the custom roles feature is not licensed', async () => { + const role = await createGlobalRole('PA put unlicensed'); + testServer.license.disable('feat:customRoles'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/roles/${role.slug}`) + .send(fullBody({ displayName: 'Should not work' })); + + 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 db82cd0d2a1..83d8c9a6ea5 100644 --- a/packages/cli/test/integration/services/role.service.test.ts +++ b/packages/cli/test/integration/services/role.service.test.ts @@ -1038,7 +1038,11 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.updateCustomRole(existingRole.slug, updateRoleDto); + const result = await roleService.updateCustomRole({ + slug: existingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }); // // ASSERT @@ -1077,7 +1081,11 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.updateCustomRole(existingRole.slug, updateRoleDto); + const result = await roleService.updateCustomRole({ + slug: existingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }); // // ASSERT @@ -1111,12 +1119,20 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.updateCustomRole(systemRole.slug, updateRoleDto)).rejects.toThrow( - BadRequestError, - ); - await expect(roleService.updateCustomRole(systemRole.slug, updateRoleDto)).rejects.toThrow( - 'Cannot update system roles', - ); + await expect( + roleService.updateCustomRole({ + slug: systemRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }), + ).rejects.toThrow(BadRequestError); + await expect( + roleService.updateCustomRole({ + slug: systemRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }), + ).rejects.toThrow('Cannot update system roles'); }); it('should update displayName when provided', async () => { @@ -1136,7 +1152,11 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.updateCustomRole(existingRole.slug, updateRoleDto); + const result = await roleService.updateCustomRole({ + slug: existingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }); // // ASSERT @@ -1161,7 +1181,11 @@ describe('RoleService', () => { // // ACT // - const result = await roleService.updateCustomRole(existingRole.slug, updateRoleDto); + const result = await roleService.updateCustomRole({ + slug: existingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }); // // ASSERT @@ -1181,9 +1205,13 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.updateCustomRole(nonExistentSlug, updateRoleDto)).rejects.toThrow( - 'Role not found', - ); + await expect( + roleService.updateCustomRole({ + slug: nonExistentSlug, + newRole: updateRoleDto, + userId: 'test-user-id', + }), + ).rejects.toThrow('Role not found'); }); it('should throw error when invalid scopes are provided', async () => { @@ -1198,9 +1226,13 @@ describe('RoleService', () => { // // ACT & ASSERT // - await expect(roleService.updateCustomRole(existingRole.slug, updateRoleDto)).rejects.toThrow( - 'The following scopes are invalid: invalid:scope', - ); + await expect( + roleService.updateCustomRole({ + slug: existingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }), + ).rejects.toThrow('The following scopes are invalid: invalid:scope'); }); it('should throw error when a role with the same display name already exists', async () => { @@ -1218,7 +1250,11 @@ describe('RoleService', () => { // ACT & ASSERT // await expect( - roleService.updateCustomRole(otherExistingRole.slug, updateRoleDto), + roleService.updateCustomRole({ + slug: otherExistingRole.slug, + newRole: updateRoleDto, + userId: 'test-user-id', + }), ).rejects.toThrow(`A role with the name "${existingRole.displayName}" already exists`); }); }); diff --git a/packages/nodes-base/nodes/N8n/n8n-api-coverage.json b/packages/nodes-base/nodes/N8n/n8n-api-coverage.json index bba689824b6..adc70cc72f1 100644 --- a/packages/nodes-base/nodes/N8n/n8n-api-coverage.json +++ b/packages/nodes-base/nodes/N8n/n8n-api-coverage.json @@ -14,6 +14,9 @@ "POST /roles": { "status": "gap" }, + "PUT /roles/{slug}": { + "status": "gap" + }, "POST /role-mapping-rules": { "status": "gap" },