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

feat: add migration #8891

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

FredrikOseberg
Copy link
Contributor

Creates a data migration that allows us to remove all hanging permissions tied to deleted environments. In addition, this migration adds a foreign key relationship to environment column that references environments name. See rationale here.

Copy link

vercel bot commented Nov 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 30, 2024 1:22pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Dec 30, 2024 1:22pm

Copy link
Contributor

github-actions bot commented Nov 29, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

@kwasniew
Copy link
Contributor

kwasniew commented Dec 30, 2024

Notable changes:

  • the new migration transitively deletes role_permissions when then env is deleted. Then it re-creates the default environment from a file called database.json with hardcoded data. The hardcoded data has no role permissions
  • one option would be to replicate all 15 role permissions in the database.json when we re-create the default env
  • in this PR what I did instead was to capture all role permissions and re-create them in-memory (read query + permissions DB insert). This way our tests are verifying role_permission setup in migrations not in some in-memory replica
  • some of the tests were relying on non default env permissions being preset even when the env didn't exist (possible because the FK from permissions to envs was missing before the migration). I'm updating all those tests and adding comments inline

@@ -43,12 +48,30 @@ beforeAll(async () => {

await app.linkProjectToEnvironment('default', 'development');

await stores.accessStore.addPermissionsToRole(
Copy link
Contributor

Choose a reason for hiding this comment

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

we were enabling feature in development and it requires two permissions. Previously they were present from a default migration that adds permissions to default, development and production envs. However we delete all environments now and all role permissions are gone. We restore all 15 permissions for the default env but for all envs that we create explicitly in tests we need to add permissions explicitly

{ name: UPDATE_FEATURE_ENVIRONMENT },
{ name: CREATE_FEATURE_STRATEGY },
],
'production',
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the previous comment for development

@@ -110,15 +120,20 @@ export default async function init(
const testDb = createDb(config);
const stores = await createStores(config, testDb);
stores.eventStore.setMaxListeners(0);
const defaultRolePermissions = await getDefaultEnvRolePermissions(testDb);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the main fix. Read all env permissions for the default environment

await resetDatabase(testDb);
await setupDatabase(stores);
await restoreRolePermissions(testDb, defaultRolePermissions);
Copy link
Contributor

Choose a reason for hiding this comment

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

re-apply the permissions

Copy link
Contributor

Choose a reason for hiding this comment

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

One alternative solution would be to store all 15 permissions in a file. What are the tradeoffs?

  • with DB query and insert approach our tests are verifying actual permissions in migrations
  • one drawback of DB query and insert is that it's different from all the other data that we seed from database.json

@@ -94,8 +94,6 @@ const createRole = async (rolePermissions: PermissionRef[]) => {

const hasCommonProjectAccess = async (user, projectName, condition) => {
const defaultEnv = 'default';
const developmentEnv = 'development';
Copy link
Contributor

Choose a reason for hiding this comment

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

those 2 environments were tested directly against user permissions table that has changes and when we delete development and production in DB seed we also loose role permissions for those two environments

@@ -637,15 +571,15 @@ test('should support permission with "ALL" environment requirement', async () =>
await accessStore.addPermissionsToRole(
customRole.id,
[{ name: CREATE_FEATURE_STRATEGY }],
'production',
'default',
Copy link
Contributor

Choose a reason for hiding this comment

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

use the env that we actually have in tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants