fix: add foreign key constraint to role_permissions table #8271
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
| @@ -17,6 +17,11 @@ import { | |||
| createFakeEventsService, | |||
| } from '../events/createEventsService'; | |||
|
|
|||
| // We need this function to satisfy the type expectations of withTransactional | |||
| export const curriedCreateAccessService = | |||
There was a problem hiding this comment.
withTransactional expects a function that takes config and returns an instantiated service
| exports.up = function (db, cb) { | ||
| db.runSql( | ||
| ` | ||
| UPDATE role_permission SET environment = null where environment = ''; |
There was a problem hiding this comment.
Update all permissions in role_permissions where environment column is set to empty string to be null instead
| 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); |
There was a problem hiding this comment.
Delete all permissions from role_permission where environment is not null and the environment does not exist in the environments table
| ` | ||
| 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; |
There was a problem hiding this comment.
Finally add the foreign key now that we no longer have any hanging permissions and no empty string values in the environments column
| @@ -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.
Ensure all incoming permissions use null instead of empty string in the service layer
Continuing splitting #8271 into smaller pieces. This PR adds transactional support for access service.
Background
#8084 surfaced an issue with permissions. 22 months ago we added a permission that replaced UPDATE_FEATURE_VARIANTS with UPDATE_FEATURE_ENVIRONMENT_VARIANTS. 17 months later we added this permission to logic in enterprise related to creating and deleting environments. In between those 17 months, if we created and deleted an environment the UPDATE_FEATURE_ENVIRONMENT_VARIANTS permission would not be deleted, and it would hang around associated to an environment that no longer existed.
This also applies for environment permissions that are not automatically applied to built in roles, such as: SKIP_CHANGE_REQUEST, APPLY_CHANGE_REQUEST and APPROVE_CHANGE_REQUEST. Creating a custom role with these permissions in a specific environment and then deleting the environment would also cause these permissions to hang around forever in the database.
Further complicating the situation, many of permissions in the database have empty strings in the environment column instead of null.
Solution
This PR attempts to solve the situation by doing the following:
We create a migration that first (1) converts all empty string values in the role_permission table to null, then (2) delete all permissions in role_permission where environment is not null and environments value does not exist in the name column of the environments table and (3) finally add the foreign key constraint on role_permission environments column that references name column in environments table with an on delete cascade.
Secondly, we guard all incoming data when we create or update a role by cleaning the permissions and setting environment to null in the access service, to keep backwards compatability and because the UI is currently sending empty string values.
Thirdly, we add a transaction around updating and creating a role since these are not atomic actions. Especially update is dangerous because we first wipe all permissions, then re-add all the new permissions. Failing to re-add the permissions puts the role in a state where it has lost all permissions.