From d84092e08a073af8975600dfd3f0756acbb4537e Mon Sep 17 00:00:00 2001 From: FredrikOseberg Date: Thu, 26 Sep 2024 10:37:42 +0200 Subject: [PATCH] fix: add foreign key constraint to role_permissions table --- .../features/access/createAccessService.ts | 8 ++- src/lib/services/access-service.ts | 16 ++++- src/lib/services/clean-permissions.test.ts | 59 +++++++++++++++++++ src/lib/services/index.ts | 9 +++ src/lib/types/model.ts | 2 +- src/lib/types/services.ts | 1 + ...436-add-foreign-key-to-role-permissions.js | 20 +++++++ 7 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 src/lib/services/clean-permissions.test.ts create mode 100644 src/migrations/20240917071436-add-foreign-key-to-role-permissions.js diff --git a/src/lib/features/access/createAccessService.ts b/src/lib/features/access/createAccessService.ts index 26791fc82f..436ab48d0b 100644 --- a/src/lib/features/access/createAccessService.ts +++ b/src/lib/features/access/createAccessService.ts @@ -1,4 +1,4 @@ -import type { Db, IUnleashConfig } from '../../server-impl'; +import type { Db, IUnleashConfig, Knex } from '../../server-impl'; import GroupStore from '../../db/group-store'; import { AccountStore } from '../../db/account-store'; import RoleStore from '../../db/role-store'; @@ -17,6 +17,11 @@ import { createFakeEventsService, } from '../events/createEventsService'; +// We need this function to satisfy the type expectations of withTransactional +export const curriedCreateAccessService = + (config: IUnleashConfig) => (db: Knex) => + createAccessService(db, config); + export const createAccessService = ( db: Db, config: IUnleashConfig, @@ -33,6 +38,7 @@ export const createAccessService = ( { getLogger }, eventService, ); + return new AccessService( { accessStore, accountStore, roleStore, environmentStore }, { getLogger }, diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 9842085b78..fdd5779180 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -108,6 +108,15 @@ export interface AccessWithRoles { const isProjectPermission = (permission) => PROJECT_ADMIN.includes(permission); +export const cleanPermissions = (permissions: PermissionRef[] | undefined) => { + return permissions?.map((permission) => { + if (permission.environment === '') { + return { ...permission, environment: null }; + } + return permission; + }); +}; + export class AccessService { private store: IAccessStore; @@ -721,7 +730,8 @@ export class AccessService { roleType, }; - const rolePermissions = role.permissions; + const rolePermissions = cleanPermissions(role.permissions); + const newRole = await this.roleStore.create(baseRole); if (rolePermissions) { if (roleType === CUSTOM_ROOT_ROLE_TYPE) { @@ -770,7 +780,9 @@ export class AccessService { description: role.description, roleType, }; - const rolePermissions = role.permissions; + + const rolePermissions = cleanPermissions(role.permissions); + const updatedRole = await this.roleStore.update(baseRole); const existingPermissions = await this.store.getPermissionsForRole( role.id, diff --git a/src/lib/services/clean-permissions.test.ts b/src/lib/services/clean-permissions.test.ts new file mode 100644 index 0000000000..5f2d826bd0 --- /dev/null +++ b/src/lib/services/clean-permissions.test.ts @@ -0,0 +1,59 @@ +import { cleanPermissions } from './access-service'; + +test('should convert all empty strings to null', () => { + const permissions = [ + { + name: 'UPDATE_PROJECT', + environment: '', + }, + { + name: 'UPDATE_FEATURE_VARIANTS', + environment: '', + }, + { + name: 'READ_PROJECT_API_TOKEN', + environment: '', + }, + { + name: 'CREATE_PROJECT_API_TOKEN', + environment: '', + }, + { + name: 'DELETE_PROJECT_API_TOKEN', + environment: '', + }, + { + name: 'UPDATE_PROJECT_SEGMENT', + environment: '', + }, + ]; + + const result = cleanPermissions(permissions); + + expect(result).toEqual([ + { + name: 'UPDATE_PROJECT', + environment: null, + }, + { + name: 'UPDATE_FEATURE_VARIANTS', + environment: null, + }, + { + name: 'READ_PROJECT_API_TOKEN', + environment: null, + }, + { + name: 'CREATE_PROJECT_API_TOKEN', + environment: null, + }, + { + name: 'DELETE_PROJECT_API_TOKEN', + environment: null, + }, + { + name: 'UPDATE_PROJECT_SEGMENT', + environment: null, + }, + ]); +}); diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index ff98fc293c..e8ea95a28d 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -1,4 +1,5 @@ import type { + IUnleash, IUnleashConfig, IUnleashServices, IUnleashStores, @@ -65,12 +66,14 @@ import ConfigurationRevisionService from '../features/feature-toggle/configurati import { createEnvironmentService, createEventsService, + createFakeAccessService, createFakeEnvironmentService, createFakeEventsService, createFakeProjectService, createFeatureLifecycleService, createFeatureToggleService, createProjectService, + curriedCreateAccessService, } from '../features'; import EventAnnouncerService from './event-announcer-service'; import { createGroupService } from '../features/group/createGroupService'; @@ -166,6 +169,11 @@ export const createServices = ( groupService, eventService, ); + + const transactionalAccessService = db + ? withTransactional(curriedCreateAccessService(config), db) + : withFakeTransactional(createFakeAccessService(config).accessService); + const apiTokenService = db ? createApiTokenService(db, config) : createFakeApiTokenService(config).apiTokenService; @@ -403,6 +411,7 @@ export const createServices = ( return { accessService, + transactionalAccessService, accountService, addonService, eventAnnouncerService, diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts index 5f87996301..aa14146658 100644 --- a/src/lib/types/model.ts +++ b/src/lib/types/model.ts @@ -419,7 +419,7 @@ export interface IPermission { name: string; displayName: string; type: string; - environment?: string; + environment?: string | null; } export interface IEnvironmentPermission { diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index 6386b686f6..b1aa2203e6 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -58,6 +58,7 @@ import type { IntegrationEventsService } from '../features/integration-events/in import type { OnboardingService } from '../features/onboarding/onboarding-service'; export interface IUnleashServices { + transactionalAccessService: WithTransactional; accessService: AccessService; accountService: AccountService; addonService: AddonService; diff --git a/src/migrations/20240917071436-add-foreign-key-to-role-permissions.js b/src/migrations/20240917071436-add-foreign-key-to-role-permissions.js new file mode 100644 index 0000000000..630158d00b --- /dev/null +++ b/src/migrations/20240917071436-add-foreign-key-to-role-permissions.js @@ -0,0 +1,20 @@ +exports.up = function (db, cb) { + db.runSql( + ` + UPDATE role_permission SET environment = null where environment = ''; + DELETE FROM role_permission WHERE environment IS NOT NULL AND environment NOT IN (SELECT name FROM environments); + ALTER TABLE role_permission ADD CONSTRAINT fk_role_permission_environment FOREIGN KEY (environment) REFERENCES environments(name) ON DELETE CASCADE; + `, + cb + ); +}; + +exports.down = function (db, cb) { + db.runSql( + ` + ALTER TABLE role_permission + DROP CONSTRAINT IF EXISTS fk_role_permission_environment; + `, + cb + ); +};