Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add foreign key constraint to role_permissions table #8271

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/lib/features/access/createAccessService.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withTransactional expects a function that takes config and returns an instantiated service

createAccessService(db, config);

export const createAccessService = (
db: Db,
config: IUnleashConfig,
Expand All @@ -33,6 +38,7 @@ export const createAccessService = (
{ getLogger },
eventService,
);

return new AccessService(
{ accessStore, accountStore, roleStore, environmentStore },
{ getLogger },
Expand Down
16 changes: 14 additions & 2 deletions src/lib/services/access-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,15 @@ export interface AccessWithRoles {

const isProjectPermission = (permission) => PROJECT_ADMIN.includes(permission);

export const cleanPermissions = (permissions: PermissionRef[] | undefined) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure all incoming permissions use null instead of empty string in the service layer

return permissions?.map((permission) => {
if (permission.environment === '') {
return { ...permission, environment: null };
}
return permission;
});
};

export class AccessService {
private store: IAccessStore;

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
59 changes: 59 additions & 0 deletions src/lib/services/clean-permissions.test.ts
Original file line number Diff line number Diff line change
@@ -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,
},
]);
});
9 changes: 9 additions & 0 deletions src/lib/services/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type {
IUnleash,
IUnleashConfig,
IUnleashServices,
IUnleashStores,
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -403,6 +411,7 @@ export const createServices = (

return {
accessService,
transactionalAccessService,
accountService,
addonService,
eventAnnouncerService,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/types/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ export interface IPermission {
name: string;
displayName: string;
type: string;
environment?: string;
environment?: string | null;
}

export interface IEnvironmentPermission {
Expand Down
1 change: 1 addition & 0 deletions src/lib/types/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: AccessService;
accountService: AccountService;
addonService: AddonService;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
exports.up = function (db, cb) {
db.runSql(
`
UPDATE role_permission SET environment = null where environment = '';
Copy link
Contributor Author

@FredrikOseberg FredrikOseberg Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update all permissions in role_permissions where environment column is set to empty string to be null instead

DELETE FROM role_permission WHERE environment IS NOT NULL AND environment NOT IN (SELECT name FROM environments);
Copy link
Contributor Author

@FredrikOseberg FredrikOseberg Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete all permissions from role_permission where environment is not null and the environment does not exist in the environments table

ALTER TABLE role_permission ADD CONSTRAINT fk_role_permission_environment FOREIGN KEY (environment) REFERENCES environments(name) ON DELETE CASCADE;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally add the foreign key now that we no longer have any hanging permissions and no empty string values in the environments column

`,
cb
);
};

exports.down = function (db, cb) {
db.runSql(
`
ALTER TABLE role_permission
DROP CONSTRAINT IF EXISTS fk_role_permission_environment;
`,
cb
);
};
Loading