From 2612a3d41c56b39bcc2af24b5024d99e66876907 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Tue, 9 Jan 2024 13:30:06 -0500 Subject: [PATCH 1/2] fix(rbac): handle preexisting policies by adding a legacy source --- .../src/service/enforcer-delegate.ts | 54 +++++++++++++------ .../src/service/permission-policy.ts | 23 +++++++- .../src/service/policy-builder.ts | 2 + plugins/rbac-common/src/types.ts | 3 +- 4 files changed, 64 insertions(+), 18 deletions(-) diff --git a/plugins/rbac-backend/src/service/enforcer-delegate.ts b/plugins/rbac-backend/src/service/enforcer-delegate.ts index a17c7f6376..e415f179b5 100644 --- a/plugins/rbac-backend/src/service/enforcer-delegate.ts +++ b/plugins/rbac-backend/src/service/enforcer-delegate.ts @@ -64,9 +64,9 @@ export class EnforcerDelegate { if (!ok) { throw new Error(`failed to create policy ${policyToString(policy)}`); } - addMetadataTrx.commit(); + await addMetadataTrx.commit(); } catch (err) { - addMetadataTrx.rollback(); + await addMetadataTrx.rollback(); throw err; } } @@ -87,9 +87,9 @@ export class EnforcerDelegate { `Failed to store policies ${policiesToString(policies)}`, ); } - addMetadataTrx.commit(); + await addMetadataTrx.commit(); } catch (err) { - addMetadataTrx.rollback(); + await addMetadataTrx.rollback(); throw err; } } @@ -107,9 +107,9 @@ export class EnforcerDelegate { if (!ok) { throw new Error(`failed to create policy ${policyToString(policy)}`); } - addMetadataTrx.commit(); + await addMetadataTrx.commit(); } catch (err) { - addMetadataTrx.rollback(); + await addMetadataTrx.rollback(); throw err; } } @@ -134,9 +134,9 @@ export class EnforcerDelegate { `Failed to store policies ${policiesToString(policies)}`, ); } - addMetadataTrx.commit(); + await addMetadataTrx.commit(); } catch (err) { - addMetadataTrx.rollback(); + await addMetadataTrx.rollback(); throw err; } } @@ -152,9 +152,9 @@ export class EnforcerDelegate { if (!ok) { throw new Error(`fail to delete policy ${policy}`); } - rmMetadataTrx.commit(); + await rmMetadataTrx.commit(); } catch (err) { - rmMetadataTrx.rollback(err); + await rmMetadataTrx.rollback(err); throw err; } } @@ -176,9 +176,9 @@ export class EnforcerDelegate { `Failed to delete policies ${policiesToString(policies)}`, ); } - rmMetadataTrx.commit(); + await rmMetadataTrx.commit(); } catch (err) { - rmMetadataTrx.rollback(err); + await rmMetadataTrx.rollback(err); throw err; } } @@ -196,9 +196,9 @@ export class EnforcerDelegate { if (!ok) { throw new Error(`Failed to delete policy ${policyToString(policy)}`); } - rmMetadataTrx.commit(); + await rmMetadataTrx.commit(); } catch (err) { - rmMetadataTrx.rollback(err); + await rmMetadataTrx.rollback(err); throw err; } } @@ -219,9 +219,9 @@ export class EnforcerDelegate { `Failed to delete grouping policies: ${policiesToString(policies)}`, ); } - rmMetadataTrx.commit(); + await rmMetadataTrx.commit(); } catch (err) { - rmMetadataTrx.rollback(err); + await rmMetadataTrx.rollback(err); throw err; } } @@ -265,4 +265,26 @@ export class EnforcerDelegate { ): Promise { return await this.metadataStorage.findPolicyMetadataBySource(source); } + + async getPoliciesWithoutSource(): Promise { + const policiesWithoutSource: string[][] = []; + const allPolicies = await this.enforcer.getPolicy(); + for (const policy of allPolicies) { + const sourcePolicy = + await this.metadataStorage.findPolicyMetadata(policy); + if (!sourcePolicy) { + policiesWithoutSource.push(policy); + } + } + return policiesWithoutSource; + } + + async migratePreexistingPolicies(enforcer: Enforcer): Promise { + const policies = await this.getPoliciesWithoutSource(); + + for (const policy of policies) { + await enforcer.removePolicy(...policy); + await this.addPolicy(policy, 'legacy'); + } + } } diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts index 785094b76d..52fcf96436 100644 --- a/plugins/rbac-backend/src/service/permission-policy.ts +++ b/plugins/rbac-backend/src/service/permission-policy.ts @@ -42,6 +42,15 @@ async function addRoleMetadata( } } +const legacyPolicies = async (enf: EnforcerDelegate, policy: string[]) => { + if ( + (await enf.hasPolicy(...policy)) && + (await enf.getMetadata(policy)).source === 'legacy' + ) { + await enf.removePolicy(policy); + } +}; + const useAdmins = async ( admins: Config[], enf: EnforcerDelegate, @@ -55,7 +64,7 @@ const useAdmins = async ( if (!adminRoleMeta) { const trx = await knex.transaction(); await roleMetadataStorage.createRoleMetadata( - { source: 'default' }, + { source: 'configuration' }, adminRoleName, trx, ); @@ -71,15 +80,20 @@ const useAdmins = async ( }); const adminReadPermission = [adminRoleName, 'policy-entity', 'read', 'allow']; + + await legacyPolicies(enf, adminReadPermission); if (!(await enf.hasPolicy(...adminReadPermission))) { await enf.addPolicy(adminReadPermission, 'configuration'); } + const adminCreatePermission = [ adminRoleName, 'policy-entity', 'create', 'allow', ]; + + await legacyPolicies(enf, adminCreatePermission); if (!(await enf.hasPolicy(...adminCreatePermission))) { await enf.addPolicy(adminCreatePermission, 'configuration'); } @@ -90,6 +104,8 @@ const useAdmins = async ( 'delete', 'allow', ]; + + await legacyPolicies(enf, adminDeletePermission); if (!(await enf.hasPolicy(...adminDeletePermission))) { await enf.addPolicy(adminDeletePermission, 'configuration'); } @@ -100,6 +116,8 @@ const useAdmins = async ( 'update', 'allow', ]; + + await legacyPolicies(enf, adminUpdatePermission); if (!(await enf.hasPolicy(...adminUpdatePermission))) { await enf.addPolicy(adminUpdatePermission, 'configuration'); } @@ -166,6 +184,9 @@ const addPredefinedPoliciesAndGroupPolicies = async ( `Failed to validate policy from file ${preDefinedPoliciesFile}. Cause: ${err.message}`, ); } + + await legacyPolicies(enf, policy); + if (!(await enf.hasPolicy(...policy))) { await enf.addPolicy(policy, 'csv-file'); } diff --git a/plugins/rbac-backend/src/service/policy-builder.ts b/plugins/rbac-backend/src/service/policy-builder.ts index 9023dba66c..3874a7384b 100644 --- a/plugins/rbac-backend/src/service/policy-builder.ts +++ b/plugins/rbac-backend/src/service/policy-builder.ts @@ -92,6 +92,8 @@ export class PolicyBuilder { knex, ); + await enforcerDelegate.migratePreexistingPolicies(enf); + const options: RouterOptions = { config: env.config, logger: env.logger, diff --git a/plugins/rbac-common/src/types.ts b/plugins/rbac-common/src/types.ts index 07d92ae420..d8c5c73c11 100644 --- a/plugins/rbac-common/src/types.ts +++ b/plugins/rbac-common/src/types.ts @@ -1,7 +1,8 @@ export type Source = | 'rest' // created via REST API | 'csv-file' // created via policies-csv-file with defined path in the application configuration - | 'configuration'; // created from application configuration + | 'configuration' // created from application configuration + | 'legacy'; // preexisting policies export type RoleSource = Source | 'default'; // hard coded in the plugin code From aa82ecbe2cadfc15e8ca6500ca2844e0cda8a845 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Tue, 9 Jan 2024 14:28:53 -0500 Subject: [PATCH 2/2] fix(rbac): handle preexisting grouping policies --- .../src/service/enforcer-delegate.ts | 44 +++++++++++++++---- .../src/service/permission-policy.ts | 44 +++---------------- .../src/service/policy-builder.ts | 1 + 3 files changed, 42 insertions(+), 47 deletions(-) diff --git a/plugins/rbac-backend/src/service/enforcer-delegate.ts b/plugins/rbac-backend/src/service/enforcer-delegate.ts index e415f179b5..c495391c00 100644 --- a/plugins/rbac-backend/src/service/enforcer-delegate.ts +++ b/plugins/rbac-backend/src/service/enforcer-delegate.ts @@ -12,12 +12,14 @@ import { PermissionPolicyMetadataDao, PolicyMetadataStorage, } from '../database/policy-metadata-storage'; +import { RoleMetadataStorage } from '../database/role-metadata'; import { policiesToString, policyToString } from '../helper'; export class EnforcerDelegate { constructor( private readonly enforcer: Enforcer, private readonly metadataStorage: PolicyMetadataStorage, + private readonly roleMetadataStorage: RoleMetadataStorage, private readonly knex: Knex, ) {} @@ -98,9 +100,9 @@ export class EnforcerDelegate { const addMetadataTrx = await this.knex.transaction(); try { - await this.metadataStorage.createPolicyMetadata( - source, - policy, + await this.roleMetadataStorage.createRoleMetadata( + { source: source }, + policy.at(1)!, addMetadataTrx, ); const ok = await this.enforcer.addGroupingPolicy(...policy); @@ -122,9 +124,9 @@ export class EnforcerDelegate { try { for (const policy of policies) { - await this.metadataStorage.createPolicyMetadata( - source, - policy, + await this.roleMetadataStorage.createRoleMetadata( + { source: source }, + policy.at(1)!, addMetadataTrx, ); } @@ -191,7 +193,10 @@ export class EnforcerDelegate { try { await this.checkIfPolicyModifiable(policy, allowToDeleCSVFilePolicy); - await this.metadataStorage.removePolicyMetadata(policy, rmMetadataTrx); + await this.roleMetadataStorage.removeRoleMetadata( + policy.at(1)!, + rmMetadataTrx, + ); const ok = await this.enforcer.removeGroupingPolicy(...policy); if (!ok) { throw new Error(`Failed to delete policy ${policyToString(policy)}`); @@ -211,7 +216,10 @@ export class EnforcerDelegate { try { for (const policy of policies) { await this.checkIfPolicyModifiable(policy, allowToDeleCSVFilePolicy); - await this.metadataStorage.removePolicyMetadata(policy, rmMetadataTrx); + await this.roleMetadataStorage.removeRoleMetadata( + policy.at(1)!, + rmMetadataTrx, + ); } const ok = await this.enforcer.removeGroupingPolicies(policies); if (!ok) { @@ -279,12 +287,32 @@ export class EnforcerDelegate { return policiesWithoutSource; } + async getGroupPoliciesWithoutSource(): Promise { + const policiesWithoutSource: string[][] = []; + const allPolicies = await this.enforcer.getGroupingPolicy(); + for (const policy of allPolicies) { + const sourcePolicy = await this.roleMetadataStorage.findRoleMetadata( + policy.at(1)!, + ); + if (!sourcePolicy) { + policiesWithoutSource.push(policy); + } + } + return policiesWithoutSource; + } + async migratePreexistingPolicies(enforcer: Enforcer): Promise { const policies = await this.getPoliciesWithoutSource(); + const groupPolicies = await this.getGroupPoliciesWithoutSource(); for (const policy of policies) { await enforcer.removePolicy(...policy); await this.addPolicy(policy, 'legacy'); } + + for (const policy of groupPolicies) { + await enforcer.removeGroupingPolicy(...policy); + await this.addGroupingPolicy(policy, 'legacy'); + } } } diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts index 52fcf96436..ca0fa1ecdf 100644 --- a/plugins/rbac-backend/src/service/permission-policy.ts +++ b/plugins/rbac-backend/src/service/permission-policy.ts @@ -18,7 +18,7 @@ import { FileAdapter, newEnforcer, newModelFromString } from 'casbin'; import { Knex } from 'knex'; import { Logger } from 'winston'; -import { RoleSource, Source } from '@janus-idp/backstage-plugin-rbac-common'; +import { Source } from '@janus-idp/backstage-plugin-rbac-common'; import { ConditionalStorage } from '../database/conditional-storage'; import { RoleMetadataStorage } from '../database/role-metadata'; @@ -27,21 +27,6 @@ import { EnforcerDelegate } from './enforcer-delegate'; import { MODEL } from './permission-model'; import { validateEntityReference } from './policies-validation'; -async function addRoleMetadata( - groupPolicy: string[], - source: RoleSource, - roleMetadataStorage: RoleMetadataStorage, - trx: Knex.Transaction, -) { - const entityRef = groupPolicy[1]; - if (entityRef.startsWith(`role:`)) { - const metadata = await roleMetadataStorage.findRoleMetadata(entityRef); - if (!metadata) { - await roleMetadataStorage.createRoleMetadata({ source }, entityRef, trx); - } - } -} - const legacyPolicies = async (enf: EnforcerDelegate, policy: string[]) => { if ( (await enf.hasPolicy(...policy)) && @@ -165,16 +150,10 @@ const addPredefinedPoliciesAndGroupPolicies = async ( } const delRoleMetaTrx = await knex.transaction(); - try { - for (const roleMeta of rolesToDelete) { - await roleMetadataStorage.removeRoleMetadata(roleMeta, delRoleMetaTrx); - } - await enf.removeGroupingPolicies(groupPoliciesToDelete, true); - delRoleMetaTrx.commit(); - } catch (err) { - delRoleMetaTrx.rollback(); - throw err; + for (const roleMeta of rolesToDelete) { + await roleMetadataStorage.removeRoleMetadata(roleMeta, delRoleMetaTrx); } + await enf.removeGroupingPolicies(groupPoliciesToDelete, true); await enf.removePolicies(policiesToDelete, true); for (const policy of policies) { @@ -200,20 +179,7 @@ const addPredefinedPoliciesAndGroupPolicies = async ( roleMetadataStorage, `csv-file`, ); - const trx = await knex.transaction(); - try { - await addRoleMetadata( - groupPolicy, - 'csv-file', - roleMetadataStorage, - trx, - ); - await enf.addGroupingPolicy(groupPolicy, 'csv-file'); - trx.commit(); - } catch (err) { - trx.rollback(); - throw err; - } + enf.addGroupingPolicy(groupPolicy, 'csv-file'); } } }; diff --git a/plugins/rbac-backend/src/service/policy-builder.ts b/plugins/rbac-backend/src/service/policy-builder.ts index 3874a7384b..21bcc6fcde 100644 --- a/plugins/rbac-backend/src/service/policy-builder.ts +++ b/plugins/rbac-backend/src/service/policy-builder.ts @@ -89,6 +89,7 @@ export class PolicyBuilder { const enforcerDelegate = new EnforcerDelegate( enf, policyMetadataStorage, + roleMetadataStorage, knex, );