From 59b8a069682e66e33ee9a09aa4ee0c33e38f411a Mon Sep 17 00:00:00 2001 From: sjaanus Date: Wed, 10 Aug 2022 10:45:59 +0300 Subject: [PATCH] Deleting project does not delete entry group_role table (#1896) * Add constraint for project * Add constraint for project * Add constraint for project * Add constraint for project * Add constraint for project * Revert eslint * Fix eslint * Fix tests --- ...20220808110415-add-projects-foreign-key.js | 20 ++++++ .../e2e/services/access-service.e2e.test.ts | 61 +++++++++++++------ .../e2e/services/project-service.e2e.test.ts | 17 ++++-- 3 files changed, 74 insertions(+), 24 deletions(-) create mode 100644 src/migrations/20220808110415-add-projects-foreign-key.js diff --git a/src/migrations/20220808110415-add-projects-foreign-key.js b/src/migrations/20220808110415-add-projects-foreign-key.js new file mode 100644 index 0000000000..e88fe3d40f --- /dev/null +++ b/src/migrations/20220808110415-add-projects-foreign-key.js @@ -0,0 +1,20 @@ +exports.up = function (db, cb) { + db.runSql( + ` + delete from group_role where project not in (select id from projects); + ALTER TABLE group_role + ADD CONSTRAINT fk_group_role_project + FOREIGN KEY(project) + REFERENCES projects(id) ON DELETE CASCADE; `, + cb, + ); +}; + +exports.down = function (db, cb) { + db.runSql( + ` + ALTER TABLE group_role DROP CONSTRAINT fk_group_role_project; +`, + cb, + ); +}; diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 0753f899ff..f36e3e3133 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -170,7 +170,7 @@ const hasCommonProjectAccess = async (user, projectName, condition) => { ).toBe(condition); }; -const hasFullProjectAccess = async (user, projectName, condition) => { +const hasFullProjectAccess = async (user, projectName: string, condition) => { const { DELETE_PROJECT, UPDATE_PROJECT, MOVE_FEATURE_TOGGLE } = permissions; expect( @@ -862,13 +862,19 @@ test('Should not be allowed to delete a project role', async () => { }); test('Should be allowed move feature toggle to project when given access through group', async () => { - const project = 'yet-another-project'; + const project = { + id: 'yet-another-project1', + name: 'yet-another-project1', + }; + const groupStore = stores.groupStore; const viewerUser = await createUserViewerAccess( 'Victoria Viewer', 'vickyv@getunleash.io', ); + await projectService.createProject(project, editorUser); + const groupWithProjectAccess = await groupStore.create({ name: 'Project Editors', description: '', @@ -882,24 +888,29 @@ test('Should be allowed move feature toggle to project when given access through const projectRole = await accessService.getRoleByName(RoleName.MEMBER); - await hasCommonProjectAccess(viewerUser, project, false); + await hasCommonProjectAccess(viewerUser, project.id, false); await accessService.addGroupToRole( groupWithProjectAccess.id, projectRole.id, 'SomeAdminUser', - project, + project.id, ); - await hasCommonProjectAccess(viewerUser, project, true); + await hasCommonProjectAccess(viewerUser, project.id, true); }); test('Should not lose user role access when given permissions from a group', async () => { - const project = 'yet-another-project'; + const project = { + id: 'yet-another-project-lose', + name: 'yet-another-project-lose', + }; const user = editorUser; const groupStore = stores.groupStore; - await accessService.createDefaultProjectRoles(user, project); + await projectService.createProject(project, user); + + // await accessService.createDefaultProjectRoles(user, project.id); const groupWithNoAccess = await groupStore.create({ name: 'ViewersOnly', @@ -908,7 +919,7 @@ test('Should not lose user role access when given permissions from a group', asy await groupStore.addNewUsersToGroup( groupWithNoAccess.id, - [{ user: editorUser, role: 'Owner' }], + [{ user: user, role: 'Owner' }], 'Admin', ); @@ -918,23 +929,33 @@ test('Should not lose user role access when given permissions from a group', asy groupWithNoAccess.id, viewerRole.id, 'SomeAdminUser', - project, + project.id, ); - await hasFullProjectAccess(editorUser, project, true); + await hasFullProjectAccess(user, project.id, true); }); test('Should allow user to take multiple group roles and have expected permissions on each project', async () => { - const projectForCreate = - 'project-that-should-have-create-toggle-permission'; - const projectForDelete = - 'project-that-should-have-delete-toggle-permission'; + const projectForCreate = { + id: 'project-that-should-have-create-toggle-permission', + name: 'project-that-should-have-create-toggle-permission', + description: 'Blah', + }; + const projectForDelete = { + id: 'project-that-should-have-delete-toggle-permission', + name: 'project-that-should-have-delete-toggle-permission', + description: 'Blah', + }; + const groupStore = stores.groupStore; const viewerUser = await createUserViewerAccess( 'Victor Viewer', 'victore@getunleash.io', ); + await projectService.createProject(projectForCreate, editorUser); + await projectService.createProject(projectForDelete, editorUser); + const groupWithCreateAccess = await groupStore.create({ name: 'ViewersOnly', description: '', @@ -989,28 +1010,28 @@ test('Should allow user to take multiple group roles and have expected permissio groupWithCreateAccess.id, deleteFeatureRole.id, 'SomeAdminUser', - projectForDelete, + projectForDelete.id, ); await accessService.addGroupToRole( groupWithDeleteAccess.id, createFeatureRole.id, 'SomeAdminUser', - projectForCreate, + projectForCreate.id, ); expect( await accessService.hasPermission( viewerUser, permissions.CREATE_FEATURE, - projectForCreate, + projectForCreate.id, ), ).toBe(true); expect( await accessService.hasPermission( viewerUser, permissions.DELETE_FEATURE, - projectForCreate, + projectForCreate.id, ), ).toBe(false); @@ -1018,14 +1039,14 @@ test('Should allow user to take multiple group roles and have expected permissio await accessService.hasPermission( viewerUser, permissions.CREATE_FEATURE, - projectForDelete, + projectForDelete.id, ), ).toBe(false); expect( await accessService.hasPermission( viewerUser, permissions.DELETE_FEATURE, - projectForDelete, + projectForDelete.id, ), ).toBe(true); }); diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index 4c3a9030a1..df43e62698 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -834,7 +834,11 @@ test('should not update role for user on project when she is the owner', async ( }); test('Should allow bulk update of group permissions', async () => { - const project = 'bulk-update-project'; + const project = { + id: 'bulk-update-project', + name: 'bulk-update-project', + }; + await projectService.createProject(project, user.id); const groupStore = stores.groupStore; const user1 = await stores.userStore.insert({ @@ -862,7 +866,7 @@ test('Should allow bulk update of group permissions', async () => { }); await projectService.addAccess( - project, + project.id, createFeatureRole.id, { users: [{ id: user1.id }], @@ -906,9 +910,14 @@ test('Should bulk update of only users', async () => { }); test('Should allow bulk update of only groups', async () => { - const project = 'bulk-update-project'; + const project = { + id: 'bulk-update-project-only', + name: 'bulk-update-project-only', + }; const groupStore = stores.groupStore; + await projectService.createProject(project, user.id); + const group1 = await groupStore.create({ name: 'ViewersOnly', description: '', @@ -929,7 +938,7 @@ test('Should allow bulk update of only groups', async () => { }); await projectService.addAccess( - project, + project.id, createFeatureRole.id, { users: [],