From 5be3181f2b506bb08188949ede755eb72bbfbc1c Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Tue, 2 Sep 2025 19:08:04 +0200 Subject: [PATCH] fix(core): Fix role management controller no-changelog (#19107) --- packages/@n8n/api-types/src/dto/index.ts | 4 +- .../roles/__tests__/create-role.dto.test.ts | 10 +- .../roles/__tests__/update-role.dto.test.ts | 18 +- .../src/dto/roles/create-role.dto.ts | 7 +- .../src/dto/roles/update-role.dto.ts | 7 +- .../cli/src/controllers/role.controller.ts | 30 +++- .../controllers/role.controller-db.test.ts | 167 ++++++++++++++++++ .../controllers/role.controller.test.ts | 10 +- 8 files changed, 219 insertions(+), 34 deletions(-) create mode 100644 packages/cli/test/integration/controllers/role.controller-db.test.ts diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index d54e4d558c9..87d2c81ae2e 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -78,8 +78,8 @@ export { USERS_LIST_SORT_OPTIONS, } from './user/users-list-filter.dto'; -export type { UpdateRoleDto } from './roles/update-role.dto'; -export type { CreateRoleDto } from './roles/create-role.dto'; +export { UpdateRoleDto } from './roles/update-role.dto'; +export { CreateRoleDto } from './roles/create-role.dto'; export { OidcConfigDto } from './oidc/config.dto'; diff --git a/packages/@n8n/api-types/src/dto/roles/__tests__/create-role.dto.test.ts b/packages/@n8n/api-types/src/dto/roles/__tests__/create-role.dto.test.ts index 00918b30338..3bbee6b2505 100644 --- a/packages/@n8n/api-types/src/dto/roles/__tests__/create-role.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/roles/__tests__/create-role.dto.test.ts @@ -1,6 +1,6 @@ import { ALL_SCOPES } from '@n8n/permissions'; -import { createRoleDtoSchema } from '../create-role.dto'; +import { CreateRoleDto } from '../create-role.dto'; describe('createRoleDtoSchema', () => { describe('Valid requests', () => { @@ -120,7 +120,7 @@ describe('createRoleDtoSchema', () => { }, }, ])('should validate $name', ({ request }) => { - const result = createRoleDtoSchema.safeParse(request); + const result = CreateRoleDto.safeParse(request); expect(result.success).toBe(true); }); }); @@ -307,7 +307,7 @@ describe('createRoleDtoSchema', () => { expectedErrorPath: ['scopes', 1], }, ])('should fail validation for $name', ({ request, expectedErrorPath }) => { - const result = createRoleDtoSchema.safeParse(request); + const result = CreateRoleDto.safeParse(request); expect(result.success).toBe(false); @@ -328,7 +328,7 @@ describe('createRoleDtoSchema', () => { scopes: [scope], }; - const result = createRoleDtoSchema.safeParse(request); + const result = CreateRoleDto.safeParse(request); expect(result.success).toBe(true); } }); @@ -354,7 +354,7 @@ describe('createRoleDtoSchema', () => { scopes: [scope], }; - const result = createRoleDtoSchema.safeParse(request); + const result = CreateRoleDto.safeParse(request); expect(result.success).toBe(false); } }); diff --git a/packages/@n8n/api-types/src/dto/roles/__tests__/update-role.dto.test.ts b/packages/@n8n/api-types/src/dto/roles/__tests__/update-role.dto.test.ts index ec7a76598be..8833f81b09d 100644 --- a/packages/@n8n/api-types/src/dto/roles/__tests__/update-role.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/roles/__tests__/update-role.dto.test.ts @@ -1,6 +1,6 @@ import { ALL_SCOPES } from '@n8n/permissions'; -import { updateRoleDtoSchema } from '../update-role.dto'; +import { UpdateRoleDto } from '../update-role.dto'; describe('updateRoleDtoSchema', () => { describe('Valid requests', () => { @@ -144,7 +144,7 @@ describe('updateRoleDtoSchema', () => { }, }, ])('should validate $name', ({ request }) => { - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(true); }); }); @@ -327,7 +327,7 @@ describe('updateRoleDtoSchema', () => { expectedErrorPath: ['scopes', 1], }, ])('should fail validation for $name', ({ request, expectedErrorPath }) => { - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(false); @@ -346,7 +346,7 @@ describe('updateRoleDtoSchema', () => { scopes: [scope], }; - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(true); } }); @@ -370,7 +370,7 @@ describe('updateRoleDtoSchema', () => { scopes: [scope], }; - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(false); } }); @@ -380,7 +380,7 @@ describe('updateRoleDtoSchema', () => { scopes: ['project:read', 'invalid-scope', 'workflow:execute'], }; - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(false); expect(result.error?.issues[0].path).toEqual(['scopes', 1]); }); @@ -403,7 +403,7 @@ describe('updateRoleDtoSchema', () => { ]; for (const request of validCombinations) { - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(true); } }); @@ -415,7 +415,7 @@ describe('updateRoleDtoSchema', () => { scopes: ['*'], // global wildcard }; - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); expect(result.success).toBe(true); }); }); @@ -450,7 +450,7 @@ describe('updateRoleDtoSchema', () => { request: { scopes: undefined }, }, ])('should handle $name correctly', ({ request, expectedErrorPath }) => { - const result = updateRoleDtoSchema.safeParse(request); + const result = UpdateRoleDto.safeParse(request); if (expectedErrorPath) { expect(result.success).toBe(false); diff --git a/packages/@n8n/api-types/src/dto/roles/create-role.dto.ts b/packages/@n8n/api-types/src/dto/roles/create-role.dto.ts index 0b8c7e54255..0f096a1abb2 100644 --- a/packages/@n8n/api-types/src/dto/roles/create-role.dto.ts +++ b/packages/@n8n/api-types/src/dto/roles/create-role.dto.ts @@ -1,11 +1,10 @@ import { scopeSchema } from '@n8n/permissions'; import { z } from 'zod'; +import { Z } from 'zod-class'; -export const createRoleDtoSchema = z.object({ +export class CreateRoleDto extends Z.class({ displayName: z.string().min(2).max(100), description: z.string().max(500).optional(), roleType: z.enum(['project']), scopes: z.array(scopeSchema), -}); - -export type CreateRoleDto = z.infer; +}) {} diff --git a/packages/@n8n/api-types/src/dto/roles/update-role.dto.ts b/packages/@n8n/api-types/src/dto/roles/update-role.dto.ts index d600faf9f54..fd743715b1a 100644 --- a/packages/@n8n/api-types/src/dto/roles/update-role.dto.ts +++ b/packages/@n8n/api-types/src/dto/roles/update-role.dto.ts @@ -1,10 +1,9 @@ import { scopeSchema } from '@n8n/permissions'; import { z } from 'zod'; +import { Z } from 'zod-class'; -export const updateRoleDtoSchema = z.object({ +export class UpdateRoleDto extends Z.class({ displayName: z.string().min(2).max(100).optional(), description: z.string().max(500).optional(), scopes: z.array(scopeSchema).optional(), -}); - -export type UpdateRoleDto = z.infer; +}) {} diff --git a/packages/cli/src/controllers/role.controller.ts b/packages/cli/src/controllers/role.controller.ts index ae45857e9dd..2821a0afb6a 100644 --- a/packages/cli/src/controllers/role.controller.ts +++ b/packages/cli/src/controllers/role.controller.ts @@ -1,5 +1,6 @@ import { CreateRoleDto, UpdateRoleDto } from '@n8n/api-types'; import { LICENSE_FEATURES } from '@n8n/constants'; +import { AuthenticatedRequest } from '@n8n/db'; import { Body, Delete, @@ -31,28 +32,45 @@ export class RoleController { } @Get('/:slug') - async getRoleBySlug(@Param('slug') slug: string): Promise { + async getRoleBySlug( + _req: AuthenticatedRequest, + _res: Response, + @Param('slug') slug: string, + ): Promise { return await this.roleService.getRole(slug); } @Patch('/:slug') @GlobalScope('role:manage') @Licensed(LICENSE_FEATURES.CUSTOM_ROLES) - async updateRole(@Param('slug') slug: string, @Body body: UpdateRoleDto): Promise { - return await this.roleService.updateCustomRole(slug, body); + async updateRole( + _req: AuthenticatedRequest, + _res: Response, + @Param('slug') slug: string, + @Body updateRole: UpdateRoleDto, + ): Promise { + return await this.roleService.updateCustomRole(slug, updateRole); } @Delete('/:slug') @GlobalScope('role:manage') @Licensed(LICENSE_FEATURES.CUSTOM_ROLES) - async deleteRole(@Param('slug') slug: string): Promise { + async deleteRole( + _req: AuthenticatedRequest, + _res: Response, + @Param('slug') slug: string, + ): Promise { return await this.roleService.removeCustomRole(slug); } @Post('/') @GlobalScope('role:manage') @Licensed(LICENSE_FEATURES.CUSTOM_ROLES) - async createRole(@Body body: CreateRoleDto): Promise { - return await this.roleService.createCustomRole(body); + async createRole( + _req: AuthenticatedRequest, + _res: Response, + @Body createRole: CreateRoleDto, + ): Promise { + return await this.roleService.createCustomRole(createRole); } } diff --git a/packages/cli/test/integration/controllers/role.controller-db.test.ts b/packages/cli/test/integration/controllers/role.controller-db.test.ts new file mode 100644 index 00000000000..44572f22324 --- /dev/null +++ b/packages/cli/test/integration/controllers/role.controller-db.test.ts @@ -0,0 +1,167 @@ +import type { CreateRoleDto, UpdateRoleDto } from '@n8n/api-types'; +import { testDb } from '@n8n/backend-test-utils'; + +import { cleanupRolesAndScopes } from '../shared/db/roles'; +import { createMember, createOwner } from '../shared/db/users'; +import type { SuperAgentTest } from '../shared/types'; +import { setupTestServer } from '../shared/utils'; +import { + PROJECT_ADMIN_ROLE, + PROJECT_EDITOR_ROLE, + PROJECT_OWNER_ROLE, + PROJECT_VIEWER_ROLE, +} from '@n8n/db'; + +describe('RoleController - Integration Tests', () => { + const testServer = setupTestServer({ endpointGroups: ['role'] }); + let ownerAgent: SuperAgentTest; + let memberAgent: SuperAgentTest; + + beforeAll(async () => { + await testDb.init(); + const owner = await createOwner(); + const member = await createMember(); + ownerAgent = testServer.authAgentFor(owner); + memberAgent = testServer.authAgentFor(member); + }); + + beforeEach(() => { + // Enable CUSTOM_ROLES license for all tests by default + testServer.license.enable('feat:customRoles'); + }); + + afterEach(async () => { + await cleanupRolesAndScopes(); + }); + + afterAll(async () => { + await testDb.terminate(); + }); + + it.each([PROJECT_ADMIN_ROLE, PROJECT_EDITOR_ROLE, PROJECT_VIEWER_ROLE, PROJECT_OWNER_ROLE])( + 'should return 200 and the role data for role $slug', + async (role) => { + // + // ACT + // + const response = await memberAgent.get(`/roles/${role.slug}`).expect(200); + + // + // ASSERT + // + response.body.data.scopes.sort(); + expect(response.body).toEqual({ + data: { + slug: role.slug, + displayName: role.displayName, + description: role.description, + systemRole: role.systemRole, + roleType: role.roleType, + scopes: role.scopes.map((scope) => scope.slug).sort(), + licensed: expect.any(Boolean), + }, + }); + }, + ); + + it('should create a custom role', async () => { + // + // ARRANGE + // + const createRoleDto: CreateRoleDto = { + displayName: 'Custom Project Role', + description: 'A custom role for project management', + roleType: 'project', + scopes: ['workflow:read', 'workflow:create'].sort(), + }; + + // + // ACT + // + const response = await ownerAgent.post('/roles').send(createRoleDto).expect(200); + + // + // ASSERT + // + response.body.data.scopes.sort(); + expect(response.body).toEqual({ + data: { + ...createRoleDto, + slug: expect.any(String), + licensed: expect.any(Boolean), + systemRole: false, + }, + }); + + const availableRole = await memberAgent.get(`/roles/${response.body.data.slug}`).expect(200); + + availableRole.body.data.scopes.sort(); + expect(availableRole.body).toEqual({ + data: { + ...createRoleDto, + slug: response.body.data.slug, + licensed: expect.any(Boolean), + systemRole: false, + }, + }); + }); + + it('should update a custom role', async () => { + // + // ARRANGE + // + const createRoleDto: CreateRoleDto = { + displayName: 'Custom Project Role', + description: 'A custom role for project management', + roleType: 'project', + scopes: ['workflow:read', 'workflow:create'].sort(), + }; + + const createResponse = await ownerAgent.post('/roles').send(createRoleDto).expect(200); + + expect(createResponse.body?.data?.slug).toBeDefined(); + const generatedRoleSlug = createResponse.body.data.slug; + + const updateRoleDto: UpdateRoleDto = { + displayName: 'Custom Project Role Updated', + description: 'A custom role for project management - updated', + }; + + // + // ACT + // + const response = await ownerAgent + .patch(`/roles/${generatedRoleSlug}`) + .send(updateRoleDto) + .expect(200); + + // + // ASSERT + // + response.body.data.scopes.sort(); + expect(response.body).toEqual({ + data: { + ...updateRoleDto, + scopes: ['workflow:read', 'workflow:create'].sort(), + slug: generatedRoleSlug, + roleType: 'project', + licensed: expect.any(Boolean), + systemRole: false, + }, + }); + + const availableRole = await memberAgent.get(`/roles/${response.body.data.slug}`).expect(200); + + availableRole.body.data.scopes.sort(); + expect(availableRole.body).toEqual({ + data: { + ...updateRoleDto, + scopes: ['workflow:read', 'workflow:create'].sort(), + slug: generatedRoleSlug, + roleType: 'project', + licensed: expect.any(Boolean), + systemRole: false, + }, + }); + }); +}); diff --git a/packages/cli/test/integration/controllers/role.controller.test.ts b/packages/cli/test/integration/controllers/role.controller.test.ts index 4fbffc972cb..44ca87bceb7 100644 --- a/packages/cli/test/integration/controllers/role.controller.test.ts +++ b/packages/cli/test/integration/controllers/role.controller.test.ts @@ -1,6 +1,6 @@ import type { CreateRoleDto, UpdateRoleDto } from '@n8n/api-types'; import { mockInstance } from '@n8n/backend-test-utils'; -import type { Role } from '@n8n/permissions'; +import { type Role } from '@n8n/permissions'; import { RoleService } from '@/services/role.service'; @@ -314,7 +314,7 @@ describe('RoleController', () => { // expect(response.body).toEqual({ data: mockCreatedRole }); // Parameter verification skipped - test framework issue - expect(roleService.createCustomRole).toHaveBeenCalledTimes(1); + expect(roleService.createCustomRole).toHaveBeenCalledWith(createRoleDto); }); it('should create role without description', async () => { @@ -348,6 +348,7 @@ describe('RoleController', () => { // ASSERT // expect(response.body).toEqual({ data: mockCreatedRole }); + expect(roleService.createCustomRole).toHaveBeenCalledWith(createRoleDto); }); it('should handle service errors gracefully', async () => { @@ -398,7 +399,7 @@ describe('RoleController', () => { const updateRoleDto: UpdateRoleDto = { displayName: 'Updated Role Name', description: 'Updated description', - scopes: ['workflow:read', 'workflow:edit'], + scopes: ['workflow:read', 'workflow:update'], }; const mockUpdatedRole: Role = { @@ -423,7 +424,7 @@ describe('RoleController', () => { // expect(response.body).toEqual({ data: mockUpdatedRole }); // Parameter verification skipped - test framework issue - expect(roleService.updateCustomRole).toHaveBeenCalledTimes(1); + expect(roleService.updateCustomRole).toHaveBeenCalledWith(roleSlug, updateRoleDto); }); it('should update only provided fields', async () => { @@ -456,6 +457,7 @@ describe('RoleController', () => { // ASSERT // expect(response.body).toEqual({ data: mockUpdatedRole }); + expect(roleService.updateCustomRole).toHaveBeenCalledWith(roleSlug, updateRoleDto); }); it('should handle service errors gracefully', async () => {