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.

Sign up for GitHub

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

feat: add migration #8891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b19870
feat: add migration
FredrikOseberg Nov 29, 2024
05ba580
test: adjust after role permissions are transitively deleted
kwasniew Dec 30, 2024
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
27 changes: 25 additions & 2 deletions src/lib/features/feature-search/feature.search.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import {
} from '../../../test/e2e/helpers/test-helper';
import getLogger from '../../../test/fixtures/no-logger';
import type { FeatureSearchQueryParameters } from '../../openapi/spec/feature-search-query-parameters';
import { DEFAULT_PROJECT, type IUnleashStores } from '../../types';
import {
CREATE_FEATURE_STRATEGY,
DEFAULT_PROJECT,
type IUnleashStores,
UPDATE_FEATURE_ENVIRONMENT,
} from '../../types';
import { DEFAULT_ENV } from '../../util';

let app: IUnleashTest;
Expand All @@ -29,7 +34,7 @@ beforeAll(async () => {
);
stores = db.stores;

await app.request
const { body } = await app.request
.post(`/auth/demo/login`)
.send({
email: '[email protected]',
Expand All @@ -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

body.rootRole,
[
{ name: UPDATE_FEATURE_ENVIRONMENT },
{ name: CREATE_FEATURE_STRATEGY },
],
'development',
);

await stores.environmentStore.create({
name: 'production',
type: 'production',
});

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

await stores.accessStore.addPermissionsToRole(
body.rootRole,
[
{ 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

);
});

afterAll(async () => {
Expand Down
5 changes: 5 additions & 0 deletions src/lib/features/project/project-service.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,11 @@ describe('Managing Project access', () => {
mode: 'open' as const,
defaultStickiness: 'clientId',
};
await db.stores.environmentStore.create({
name: 'production',
type: 'production',
enabled: true,
});

const auditUser = extractAuditInfoFromUser(user);
await projectService.createProject(project, user, auditUser);
Expand Down
20 changes: 20 additions & 0 deletions src/migrations/20241129102205-add-foreign-key-to-role-permissions.js
View file
Open in desktop
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 = '';
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,
);
};
17 changes: 16 additions & 1 deletion src/test/e2e/helpers/database-init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,19 @@ delete process.env.DATABASE_URL;
// because of db-migrate bug (https://github.com/Unleash/unleash/issues/171)
process.setMaxListeners(0);

async function getDefaultEnvRolePermissions(knex) {
return knex.table('role_permission').whereIn('environment', ['default']);
}

async function restoreRolePermissions(knex, rolePermissions) {
await knex.table('role_permission').insert(rolePermissions);
}

async function resetDatabase(knex) {
return Promise.all([
knex.table('environments').del(),
knex
.table('environments')
.del(), // deletes role permissions transitively
knex.table('strategies').del(),
knex.table('features').del(),
knex.table('client_applications').del(),
Expand Down Expand Up @@ -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


return {
rawDatabase: testDb,
stores,
reset: async () => {
const defaultRolePermissions =
await getDefaultEnvRolePermissions(testDb);
await resetDatabase(testDb);
await setupDatabase(stores);
await restoreRolePermissions(testDb, defaultRolePermissions);
},
destroy: async () => {
return new Promise<void>((resolve, reject) => {
Expand Down
78 changes: 6 additions & 72 deletions src/test/e2e/services/access-service.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

const productionEnv = 'production';

const {
CREATE_FEATURE,
Expand Down Expand Up @@ -155,70 +153,6 @@ const hasCommonProjectAccess = async (user, projectName, condition) => {
defaultEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
CREATE_FEATURE_STRATEGY,
projectName,
developmentEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
UPDATE_FEATURE_STRATEGY,
projectName,
developmentEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
DELETE_FEATURE_STRATEGY,
projectName,
developmentEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
UPDATE_FEATURE_ENVIRONMENT,
projectName,
developmentEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
CREATE_FEATURE_STRATEGY,
projectName,
productionEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
UPDATE_FEATURE_STRATEGY,
projectName,
productionEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
DELETE_FEATURE_STRATEGY,
projectName,
productionEnv,
),
).toBe(condition);
expect(
await accessService.hasPermission(
user,
UPDATE_FEATURE_ENVIRONMENT,
projectName,
productionEnv,
),
).toBe(condition);
};

const hasFullProjectAccess = async (user, projectName: string, condition) => {
Expand Down Expand Up @@ -378,7 +312,7 @@ test('should remove CREATE_FEATURE on default environment', async () => {
await accessService.addPermissionToRole(
editRole.id,
permissions.CREATE_FEATURE,
'*',
'default',
);

// TODO: to validate the remove works, we should make sure that we had permission before removing it
Expand Down Expand Up @@ -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

);
await accessStore.addUserToRole(user.id, customRole.id, ALL_PROJECTS);

const hasAccess = await accessService.hasPermission(
user,
CREATE_FEATURE_STRATEGY,
'default',
'production',
'default',
);

expect(hasAccess).toBe(true);
Expand All @@ -667,7 +601,7 @@ test('Should have access to create a strategy in an environment', async () => {
user,
CREATE_FEATURE_STRATEGY,
'default',
'development',
'default',
),
).toBe(true);
});
Expand All @@ -693,7 +627,7 @@ test('Should have access to edit a strategy in an environment', async () => {
user,
UPDATE_FEATURE_STRATEGY,
'default',
'development',
'default',
),
).toBe(true);
});
Expand All @@ -706,7 +640,7 @@ test('Should have access to delete a strategy in an environment', async () => {
user,
DELETE_FEATURE_STRATEGY,
'default',
'development',
'default',
),
).toBe(true);
});
Expand Down
Loading