From c29983d810a1fe97df4521b96620f4da40ad36ea Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 20 Nov 2025 13:32:49 +0100 Subject: [PATCH] fix: handle invalid permissions in role creation (#11003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `create` and `update` role methods used to blindly accept any incoming permissions, but if the permissions don't exist in the database, then the database would throw, yielding a 500 error to the user. To fix this, we can validate that all the permissions exist before we try to add the incoming permissions. The http error only manifests in enterprise, but the fix requires modifying the access service. Therefore, I've added the tests to the access service too, such that if you break something, then you don't need to wait for it to propagate to enterprise. --------- Co-authored-by: Gastón Fournier --- .../features/access/createAccessService.ts | 11 +++- src/lib/services/access-service.test.ts | 32 +++++++++-- src/lib/services/access-service.ts | 33 ++++++++++++ .../e2e/services/access-service.e2e.test.ts | 53 +++++++++++++++++++ src/test/fixtures/fake-access-store.ts | 11 +++- 5 files changed, 131 insertions(+), 9 deletions(-) diff --git a/src/lib/features/access/createAccessService.ts b/src/lib/features/access/createAccessService.ts index de08bb29fd..ccf745b029 100644 --- a/src/lib/features/access/createAccessService.ts +++ b/src/lib/features/access/createAccessService.ts @@ -10,7 +10,9 @@ import FakeEventStore from '../../../test/fixtures/fake-event-store.js'; import { FakeAccountStore } from '../../../test/fixtures/fake-account-store.js'; import FakeRoleStore from '../../../test/fixtures/fake-role-store.js'; import FakeEnvironmentStore from '../project-environments/fake-environment-store.js'; -import FakeAccessStore from '../../../test/fixtures/fake-access-store.js'; +import FakeAccessStore, { + type FakeAccessStoreConfig, +} from '../../../test/fixtures/fake-access-store.js'; import type { IAccessStore, IEventStore, @@ -45,8 +47,13 @@ export const createAccessService = ( ); }; +export type FakeAccessServiceConfig = { + accessStoreConfig?: FakeAccessStoreConfig; +}; + export const createFakeAccessService = ( config: IUnleashConfig, + { accessStoreConfig }: FakeAccessServiceConfig = {}, ): { accessService: AccessService; eventStore: IEventStore; @@ -59,7 +66,7 @@ export const createFakeAccessService = ( const accountStore = new FakeAccountStore(); const roleStore = new FakeRoleStore(); const environmentStore = new FakeEnvironmentStore(); - const accessStore = new FakeAccessStore(roleStore); + const accessStore = new FakeAccessStore(roleStore, accessStoreConfig); const eventService = createFakeEventsService(config, { eventStore }); const groupService = new GroupService( { groupStore, accountStore }, diff --git a/src/lib/services/access-service.test.ts b/src/lib/services/access-service.test.ts index 71af9591d6..41eb9469c0 100644 --- a/src/lib/services/access-service.test.ts +++ b/src/lib/services/access-service.test.ts @@ -1,6 +1,9 @@ import NameExistsError from '../error/name-exists-error.js'; import getLogger from '../../test/fixtures/no-logger.js'; -import { createFakeAccessService } from '../features/access/createAccessService.js'; +import { + createFakeAccessService, + type FakeAccessServiceConfig, +} from '../features/access/createAccessService.js'; import { AccessService, type IRoleCreation, @@ -29,13 +32,15 @@ import { createFakeAccessReadModel } from '../features/access/createAccessReadMo import { ROLE_CREATED } from '../events/index.js'; import { expect } from 'vitest'; -function getSetup() { +function getSetup(accessServiceConfig?: FakeAccessServiceConfig) { const config = createTestConfig({ getLogger, }); - const { accessService, eventStore, accessStore } = - createFakeAccessService(config); + const { accessService, eventStore, accessStore } = createFakeAccessService( + config, + accessServiceConfig, + ); return { accessService, @@ -213,7 +218,24 @@ test('should be able to validate and cleanup with additional properties', async }); test('user with custom root role should get a user root role', async () => { - const { accessService, eventStore } = getSetup(); + const availablePermissions = [ + { + id: 1, + environment: 'development', + name: 'fake', + displayName: 'fake', + type: '', + }, + { + id: 2, + name: 'root-fake-permission', + displayName: '', + type: '', + }, + ]; + const { accessService, eventStore } = getSetup({ + accessStoreConfig: { availablePermissions }, + }); const createRoleInput: IRoleCreation = { name: 'custom-root-role', description: 'test custom root role', diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 81b730c81c..e7f934d2cd 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -707,6 +707,8 @@ export class AccessService { roleType, }; + await this.validatePermissions(role.permissions); + const rolePermissions = cleanPermissionEnvironment(role.permissions); const newRole = await this.roleStore.create(baseRole); if (rolePermissions) { @@ -756,6 +758,8 @@ export class AccessService { description: role.description, roleType, }; + + await this.validatePermissions(role.permissions); const rolePermissions = cleanPermissionEnvironment(role.permissions); const updatedRole = await this.roleStore.update(baseRole); const existingPermissions = await this.store.getPermissionsForRole( @@ -878,4 +882,33 @@ export class AccessService { async getUserAccessOverview(): Promise { return this.store.getUserAccessOverview(); } + + async validatePermissions(permissions?: PermissionRef[]): Promise { + if (!permissions?.length) { + return; + } + const availablePermissions = await this.store.getAvailablePermissions(); + const invalidPermissions = permissions.filter( + (permission) => + !availablePermissions.some((availablePermission) => + 'id' in permission + ? availablePermission.id === permission.id + : availablePermission.name === permission.name, + ), + ); + + if (invalidPermissions.length > 0) { + const invalidPermissionList = invalidPermissions + .map((permission) => + 'id' in permission + ? `permission with ID: ${permission.id}` + : permission.name, + ) + .join(', '); + + throw new BadDataError( + `Invalid permissions supplied. The following permissions don't exist: ${invalidPermissionList}.`, + ); + } + } } diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 51bf8c87c9..ed988aefc2 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -1848,3 +1848,56 @@ test('access overview should include users with custom root roles', async () => expect(userAccess.userId).toBe(user.id); expect(userAccess.rootRole).toBe('Mischievous Messenger'); }); + +test("creating a role with permissions that don't exist should throw a bad data error", async () => { + await expect(() => + accessService.createRole( + { + name: 'Oogus Boogus', + type: CUSTOM_ROOT_ROLE_TYPE, + description: + "Well, well, well ... what have we here? Sandy Claws, huh? Oooh, I'm really scared!", + permissions: [{ name: 'BOGUS' }], + createdByUserId: 1, + }, + SYSTEM_USER_AUDIT, + ), + ).rejects.toThrow( + expect.objectContaining({ + name: 'BadDataError', + message: expect.stringMatching(/BOGUS/), + }), + ); +}); + +test("Updating a role with permissions that don't exist should throw a bad data error", async () => { + const custom_role = await accessService.createRole( + { + name: 'Legit custom role', + type: CUSTOM_ROOT_ROLE_TYPE, + description: '', + permissions: [{ name: permissions.CREATE_ADDON }], + createdByUserId: 1, + }, + SYSTEM_USER_AUDIT, + ); + await expect(() => + accessService.updateRole( + { + id: custom_role.id, + name: 'Oogus Boogus', + type: CUSTOM_ROOT_ROLE_TYPE, + description: + 'This might be the last time that you hear the Boogus song', + permissions: [{ name: 'BOGUS' }], + createdByUserId: 1, + }, + SYSTEM_USER_AUDIT, + ), + ).rejects.toThrow( + expect.objectContaining({ + name: 'BadDataError', + message: expect.stringMatching(/BOGUS/), + }), + ); +}); diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index 3dcb4f0be6..20067c15a4 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -18,6 +18,10 @@ import { import FakeRoleStore from './fake-role-store.js'; import type { PermissionRef } from '../../lib/services/access-service.js'; +export type FakeAccessStoreConfig = Partial<{ + availablePermissions: IPermission[]; +}>; + export class FakeAccessStore implements IAccessStore { fakeRolesStore: IRoleStore; @@ -25,8 +29,11 @@ export class FakeAccessStore implements IAccessStore { rolePermissions: Map = new Map(); - constructor(roleStore?: IRoleStore) { + availablePermissions: IPermission[] = []; + + constructor(roleStore?: IRoleStore, config?: FakeAccessStoreConfig) { this.fakeRolesStore = roleStore ?? new FakeRoleStore(); + this.availablePermissions = config?.availablePermissions ?? []; } getProjectUserAndGroupCountsForRole( @@ -112,7 +119,7 @@ export class FakeAccessStore implements IAccessStore { } getAvailablePermissions(): Promise { - throw new Error('Method not implemented.'); + return Promise.resolve(this.availablePermissions); } getPermissionsForUser(userId: Number): Promise {