-
-
Notifications
You must be signed in to change notification settings - Fork 721
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,15 @@ export interface AccessWithRoles { | |
|
||
const isProjectPermission = (permission) => PROJECT_ADMIN.includes(permission); | ||
|
||
export const cleanPermissions = (permissions: PermissionRef[] | undefined) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
@@ -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, | ||
|
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, | ||
}, | ||
]); | ||
}); |
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 = ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update all permissions in |
||
DELETE FROM role_permission WHERE environment IS NOT NULL AND environment NOT IN (SELECT name FROM environments); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
); | ||
}; |
There was a problem hiding this comment.
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