From 88a034d0664cbd2c9696755a3955211e869c9b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 23 Nov 2023 10:23:21 +0000 Subject: [PATCH] fix: dedupe any duplicate permissions (#5397) https://linear.app/unleash/issue/2-1656/create-db-migration-that-ensures-correct-state-of-permissions This adds a migration that dedupes any duplicate permissions. --- ...121456-dedupe-any-duplicate-permissions.js | 72 ++++++++ src/test/e2e/dedupe-permissions.e2e.test.ts | 173 ++++++++++++++++++ 2 files changed, 245 insertions(+) create mode 100644 src/migrations/20231122121456-dedupe-any-duplicate-permissions.js create mode 100644 src/test/e2e/dedupe-permissions.e2e.test.ts diff --git a/src/migrations/20231122121456-dedupe-any-duplicate-permissions.js b/src/migrations/20231122121456-dedupe-any-duplicate-permissions.js new file mode 100644 index 0000000000..65f1ae1079 --- /dev/null +++ b/src/migrations/20231122121456-dedupe-any-duplicate-permissions.js @@ -0,0 +1,72 @@ +exports.up = function (db, cb) { + db.runSql( + ` +-- STEP 1: Update references in the role_permission table, setting the permission to the lowest ID for each duplicate permission + +WITH Duplicates AS ( + SELECT + MIN(id) AS min_id, + permission + FROM + permissions + GROUP BY + permission + HAVING + COUNT(*) > 1 +) + +UPDATE + role_permission +SET + permission_id = d.min_id +FROM + Duplicates d +JOIN + permissions p ON d.permission = p.permission +WHERE + role_permission.permission_id = p.id +AND + role_permission.permission_id != d.min_id; + + +-- STEP 2: Delete redundant role_permission entries + +DELETE FROM role_permission +WHERE ctid IN ( + SELECT ctid + FROM ( + SELECT ctid, ROW_NUMBER() OVER (PARTITION BY role_id, permission_id, environment ORDER BY created_at) as rn + FROM role_permission + ) t + WHERE t.rn > 1 +); + +-- STEP 3: Delete the duplicate permissions, keeping the ones with the lowest ID + +WITH Duplicates AS ( + SELECT + MIN(id) AS min_id, + permission + FROM + permissions + GROUP BY + permission + HAVING + COUNT(*) > 1 +) + +DELETE FROM permissions +WHERE id NOT IN ( + SELECT min_id FROM Duplicates +) +AND permission IN ( + SELECT permission FROM Duplicates +); + `, + cb + ); +}; + +exports.down = function (db, callback) { + callback(); +}; diff --git a/src/test/e2e/dedupe-permissions.e2e.test.ts b/src/test/e2e/dedupe-permissions.e2e.test.ts new file mode 100644 index 0000000000..6e0442017d --- /dev/null +++ b/src/test/e2e/dedupe-permissions.e2e.test.ts @@ -0,0 +1,173 @@ +import { getDbConfig } from './helpers/database-config'; +import { createTestConfig } from '../config/test-config'; +import { getInstance } from 'db-migrate'; +import { log } from 'db-migrate-shared'; +import { Client } from 'pg'; +import { IDBOption } from 'lib/types'; + +log.setLogLevel('error'); + +async function initSchema(db: IDBOption): Promise { + const client = new Client(db); + await client.connect(); + await client.query(`DROP SCHEMA IF EXISTS ${db.schema} CASCADE`); + await client.query(`CREATE SCHEMA IF NOT EXISTS ${db.schema}`); + await client.end(); +} + +test('Dedupe permissions migration correctly dedupes permissions', async () => { + jest.setTimeout(15000); + const config = createTestConfig({ + db: { + ...getDbConfig(), + pool: { min: 1, max: 4 }, + schema: 'dedupe_permissions_test', + ssl: false, + }, + }); + + await initSchema(config.db); + + const e2e = { + ...config.db, + connectionTimeoutMillis: 2000, + }; + + // disable Intellij/WebStorm from setting verbose CLI argument to db-migrator + process.argv = process.argv.filter((it) => !it.includes('--verbose')); + const dbm = getInstance(true, { + cwd: `${__dirname}/../../`, // relative to src/test/e2e + config: { e2e }, + env: 'e2e', + }); + + // Run all migrations up to, and including, this one, the last one before the dedupe migration + await dbm.up('20231121153304-add-permission-create-tag-type.js'); + + // Set up the test data + const client = new Client(config.db); + await client.connect(); + await client.query(` + DELETE FROM "dedupe_permissions_test"."role_permission"; + + INSERT INTO "dedupe_permissions_test"."roles" (id, name, description, type) VALUES (101, 'Role 1', 'A test role', 'custom'); + INSERT INTO "dedupe_permissions_test"."roles" (id, name, description, type) VALUES (102, 'Role 2', 'A test role', 'custom'); + INSERT INTO "dedupe_permissions_test"."roles" (id, name, description, type) VALUES (103, 'Role 3', 'A test role', 'custom'); + INSERT INTO "dedupe_permissions_test"."roles" (id, name, description, type) VALUES (104, 'Role 4', 'A test role', 'custom'); + `); + await client.query(` + DELETE FROM "dedupe_permissions_test"."permissions"; + + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (101, 'TEST_PERMISSION_1', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (102, 'TEST_PERMISSION_DUPLICATE_1', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (103, 'TEST_PERMISSION_DUPLICATE_1', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (104, 'TEST_PERMISSION_2', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (105, 'TEST_PERMISSION_3', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (106, 'TEST_PERMISSION_DUPLICATE_2', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (107, 'TEST_PERMISSION_DUPLICATE_2', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (108, 'TEST_PERMISSION_4', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (109, 'TEST_PERMISSION_DUPLICATE_3', 'A test permission', 'root'); + INSERT INTO "dedupe_permissions_test"."permissions" (id, permission, display_name, type) VALUES (110, 'TEST_PERMISSION_DUPLICATE_3', 'A test permission', 'root'); + `); + await client.query(` + DELETE FROM "dedupe_permissions_test"."role_permission"; + + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 101); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 102); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 103); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 104); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 105); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 106); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 107); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 108); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 109); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (101, 110); + + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (102, 102); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (102, 103); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (102, 106); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (102, 107); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (102, 109); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (102, 110); + + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (103, 101); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (103, 104); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (103, 105); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, permission_id) VALUES (103, 108); + + -- Duplicate permission assignments, where role_id, permission_id, and environment are the same, as they should be deduped to the min created_at + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, created_at, permission_id, environment) VALUES (104, '2021-01-01T00:00:00Z', 102, 'dev'); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, created_at, permission_id, environment) VALUES (104, '2021-01-02T00:00:00Z', 102, 'dev'); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, created_at, permission_id, environment) VALUES (104, '2021-01-02T00:00:00Z', 102, 'prod'); + INSERT INTO "dedupe_permissions_test"."role_permission" (role_id, created_at, permission_id, environment) VALUES (104, '2021-01-01T00:00:00Z', 102, 'prod'); + `); + + // Run the dedupe migration + await dbm.up('20231122121456-dedupe-any-duplicate-permissions.js'); + + // Check the results + const { rows: resultsPermissions } = await client.query(` + SELECT permission FROM "dedupe_permissions_test"."permissions" ORDER BY id; + `); + + const { rows: resultsRolePermission } = await client.query(` + SELECT role_id, permission_id FROM "dedupe_permissions_test"."role_permission" WHERE role_id IN (101, 102, 103) ORDER BY role_id, permission_id; + `); + + expect(resultsPermissions.length).toEqual(7); + expect(resultsPermissions).toEqual([ + { permission: 'TEST_PERMISSION_1' }, + { permission: 'TEST_PERMISSION_DUPLICATE_1' }, + { permission: 'TEST_PERMISSION_2' }, + { permission: 'TEST_PERMISSION_3' }, + { permission: 'TEST_PERMISSION_DUPLICATE_2' }, + { permission: 'TEST_PERMISSION_4' }, + { permission: 'TEST_PERMISSION_DUPLICATE_3' }, + ]); + + expect(resultsRolePermission.length).toEqual(14); + expect(resultsRolePermission).toEqual([ + // Role 101, we set all permissions and expect to see all permissions, where the duplicate permissions have been deduped to the min ID of each duplicate permission + { role_id: 101, permission_id: 101 }, + { role_id: 101, permission_id: 102 }, + { role_id: 101, permission_id: 104 }, + { role_id: 101, permission_id: 105 }, + { role_id: 101, permission_id: 106 }, + { role_id: 101, permission_id: 108 }, + { role_id: 101, permission_id: 109 }, + // Role 102, we set all duplicate permissions and expect to see the deduped permissions to the min ID of each duplicate permission + { role_id: 102, permission_id: 102 }, + { role_id: 102, permission_id: 106 }, + { role_id: 102, permission_id: 109 }, + // Role 103, we set all unique permissions and expect to see no changes + { role_id: 103, permission_id: 101 }, + { role_id: 103, permission_id: 104 }, + { role_id: 103, permission_id: 105 }, + { role_id: 103, permission_id: 108 }, + ]); + + // Test duplicate permission assignments, where role_id, permission_id, and environment are the same, as they should be deduped to the min created_at + const { rows: resultsDedupedRolePermissionAssignments } = + await client.query(` + SELECT role_id, created_at, permission_id, environment FROM "dedupe_permissions_test"."role_permission" WHERE role_id = 104 ORDER BY role_id, permission_id; + `); + + expect(resultsDedupedRolePermissionAssignments.length).toEqual(2); + expect(resultsDedupedRolePermissionAssignments).toEqual([ + { + role_id: 104, + created_at: new Date('2021-01-01T00:00:00.000Z'), + permission_id: 102, + environment: 'dev', + }, + { + role_id: 104, + created_at: new Date('2021-01-01T00:00:00.000Z'), + permission_id: 102, + environment: 'prod', + }, + ]); + + await client.end(); + await dbm.reset(); +});