From ba4b3e9a9cd88d7c0da560a4030050712224613d Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Thu, 16 Jan 2025 10:51:09 +0200 Subject: [PATCH] fix(rbac): use loadPolicy to keep in sync enforcer for edit operations (#2166) * fix(rbac): use loadPolicy to keep in sync enforcer for edit operations Signed-off-by: Oleksandr Andriienko * fix(rbac): fix permission evaluation when a lot of requests Signed-off-by: Oleksandr Andriienko * fix(rbac): fix error handling Signed-off-by: Oleksandr Andriienko * fix(rbac): remove not needed changes Signed-off-by: Oleksandr Andriienko * fix(rbac): fix up Signed-off-by: Oleksandr Andriienko * fix(rbac): add changeset Signed-off-by: Oleksandr Andriienko * fix(rbac): provide db pool size info to casbin adapter Signed-off-by: Oleksandr Andriienko * fix(rbac): handle code review feedback, improve changeset Signed-off-by: Oleksandr Andriienko --------- Signed-off-by: Oleksandr Andriienko --- .../rbac/.changeset/red-glasses-reply.md | 5 + .../rbac-backend/__fixtures__/test-utils.ts | 7 +- .../rbac/plugins/rbac-backend/package.json | 1 + .../src/audit-log/audit-logger.ts | 7 + .../src/database/casbin-adapter-factory.ts | 1 + .../file-permissions/csv-file-watcher.test.ts | 7 +- .../src/policies/permission-policy.test.ts | 8 +- .../src/policies/permission-policy.ts | 1 + .../src/providers/connect-providers.test.ts | 8 +- .../src/providers/connect-providers.ts | 2 + .../src/service/enforcer-delegate.test.ts | 8 +- .../src/service/enforcer-delegate.ts | 566 +++++++++++------- .../src/service/policy-builder.ts | 12 +- workspaces/rbac/yarn.lock | 12 +- 14 files changed, 419 insertions(+), 226 deletions(-) create mode 100644 workspaces/rbac/.changeset/red-glasses-reply.md diff --git a/workspaces/rbac/.changeset/red-glasses-reply.md b/workspaces/rbac/.changeset/red-glasses-reply.md new file mode 100644 index 0000000000..cd8234fa07 --- /dev/null +++ b/workspaces/rbac/.changeset/red-glasses-reply.md @@ -0,0 +1,5 @@ +--- +'@backstage-community/plugin-rbac-backend': patch +--- + +Use loadPolicy to keep the enforcer in sync for edit operations. It should keep the RBAC plugin in sync when the Backstage instance is scaled to multiple deployment replicas. Reuse the maximum database pool size value from the application configuration in the RBAC Casbin adapter. diff --git a/workspaces/rbac/plugins/rbac-backend/__fixtures__/test-utils.ts b/workspaces/rbac/plugins/rbac-backend/__fixtures__/test-utils.ts index d563ac92eb..22fb159a91 100644 --- a/workspaces/rbac/plugins/rbac-backend/__fixtures__/test-utils.ts +++ b/workspaces/rbac/plugins/rbac-backend/__fixtures__/test-utils.ts @@ -140,7 +140,12 @@ export async function newEnforcerDelegate( await enf.addGroupingPolicies(storedGroupingPolicies); } - return new EnforcerDelegate(enf, roleMetadataStorageMock, mockClientKnex); + return new EnforcerDelegate( + enf, + auditLogger(), + roleMetadataStorageMock, + mockClientKnex, + ); } export async function newPermissionPolicy( diff --git a/workspaces/rbac/plugins/rbac-backend/package.json b/workspaces/rbac/plugins/rbac-backend/package.json index ee1150bd0f..3a07d1ee38 100644 --- a/workspaces/rbac/plugins/rbac-backend/package.json +++ b/workspaces/rbac/plugins/rbac-backend/package.json @@ -64,6 +64,7 @@ "@backstage/types": "1.1.1", "@spotify/prettier-config": "^15.0.0", "@types/express": "4.17.21", + "@types/knex": "^0.16.1", "@types/lodash": "^4.14.151", "@types/node": "18.19.68", "@types/supertest": "2.0.16", diff --git a/workspaces/rbac/plugins/rbac-backend/src/audit-log/audit-logger.ts b/workspaces/rbac/plugins/rbac-backend/src/audit-log/audit-logger.ts index 8672d53ffb..5911bec0ea 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/audit-log/audit-logger.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/audit-log/audit-logger.ts @@ -92,6 +92,10 @@ export type EvaluationAuditInfo = { decision?: PolicyDecision; }; +export const PoliciesData = { + FAILED_TO_FETCH_NEWER_PERMISSIONS: 'FailedToFetchNewerPermissions', +}; + export const ConditionEvents = { CREATE_CONDITION: 'CreateCondition', UPDATE_CONDITION: 'UpdateCondition', @@ -116,6 +120,9 @@ export const RBAC_BACKEND = 'rbac-backend'; // Audit log stage for processing Role-Based Access Control (RBAC) data export const HANDLE_RBAC_DATA_STAGE = 'handleRBACData'; +// Audit log stage to refresh Role-Based Access Control (RBAC) data +export const FETCH_NEWER_PERMISSIONS_STAGE = 'fetchNewerPermissions'; + // Audit log stage for determining access rights based on user permissions and resource information export const EVALUATE_PERMISSION_ACCESS_STAGE = 'evaluatePermissionAccess'; diff --git a/workspaces/rbac/plugins/rbac-backend/src/database/casbin-adapter-factory.ts b/workspaces/rbac/plugins/rbac-backend/src/database/casbin-adapter-factory.ts index 9c59a1e9f9..5008ca2338 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/database/casbin-adapter-factory.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/database/casbin-adapter-factory.ts @@ -54,6 +54,7 @@ export class CasbinDBAdapterFactory { ssl, database: dbName, schema: schema, + poolSize: databaseConfig?.getOptionalNumber('knexConfig.pool.max'), }); } diff --git a/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts b/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts index 0f1b8cafab..d7f68e06da 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts @@ -180,7 +180,12 @@ describe('CSVFileWatcher', () => { const knex = Knex.knex({ client: MockClient }); - enforcerDelegate = new EnforcerDelegate(enf, roleMetadataStorageMock, knex); + enforcerDelegate = new EnforcerDelegate( + enf, + auditLoggerMock, + roleMetadataStorageMock, + knex, + ); auditLoggerMock.auditLog.mockReset(); (roleMetadataStorageMock.updateRoleMetadata as jest.Mock).mockClear(); diff --git a/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.test.ts b/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.test.ts index b3aa0b2040..ca1c639570 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.test.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.test.ts @@ -1750,6 +1750,7 @@ describe('Policy checks for conditional policies', () => { const enfDelegate = new EnforcerDelegate( enf, + auditLoggerMock, roleMetadataStorageMock, mockClientKnex, ); @@ -2173,7 +2174,12 @@ async function newEnforcerDelegate( await enf.addGroupingPolicies(storedGroupingPolicies); } - return new EnforcerDelegate(enf, roleMetadataStorageMock, mockClientKnex); + return new EnforcerDelegate( + enf, + auditLoggerMock, + roleMetadataStorageMock, + mockClientKnex, + ); } async function newPermissionPolicy( diff --git a/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.ts b/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.ts index 381eb716a0..98e54ed65d 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.ts @@ -216,6 +216,7 @@ export class RBACPermissionPolicy implements PermissionPolicy { } const permissionName = request.permission.name; + await this.enforcer.loadPolicy(); const roles = await this.enforcer.getRolesForUser(userEntityRef); if (isResourcePermission(request.permission)) { diff --git a/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.test.ts b/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.test.ts index 1fcfffed33..ada0991af4 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.test.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.test.ts @@ -184,7 +184,12 @@ describe('Connection', () => { const knex = Knex.knex({ client: MockClient }); - enforcerDelegate = new EnforcerDelegate(enf, roleMetadataStorageMock, knex); + enforcerDelegate = new EnforcerDelegate( + enf, + auditLoggerMock, + roleMetadataStorageMock, + knex, + ); await enforcerDelegate.addGroupingPolicy( roleToBeRemoved, @@ -478,6 +483,7 @@ describe('connectRBACProviders', () => { const enforcerDelegate = new EnforcerDelegate( enf, + auditLoggerMock, roleMetadataStorageMock, knex, ); diff --git a/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.ts b/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.ts index 70861aaf24..5a9a711ab2 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.ts @@ -67,6 +67,7 @@ export class Connection implements RBACProviderConnection { const providerRoles = await this.getProviderRoles(); + await this.enforcer.loadPolicy(); // Get the roles for this provider coming from rbac plugin for (const providerRole of providerRoles) { providerRolesforRemoval.push( @@ -95,6 +96,7 @@ export class Connection implements RBACProviderConnection { const providerRoles = await this.getProviderRoles(); + await this.enforcer.loadPolicy(); // Get the roles for this provider coming from rbac plugin for (const providerRole of providerRoles) { providerPermissions.push( diff --git a/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.test.ts b/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.test.ts index c84a90f1ab..ddee443409 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.test.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.test.ts @@ -27,6 +27,7 @@ import { import { BackstageRoleManager } from '../role-manager/role-manager'; import { EnforcerDelegate } from './enforcer-delegate'; import { MODEL } from './permission-model'; +import { auditLoggerMock } from '../../__fixtures__/mock-utils'; // TODO: Move to 'catalogServiceMock' from '@backstage/plugin-catalog-node/testUtils' // once '@backstage/plugin-catalog-node' is upgraded @@ -179,7 +180,12 @@ describe('EnforcerDelegate', () => { await enf.addGroupingPolicies(groupingPolicies); } - return new EnforcerDelegate(enf, roleMetadataStorageMock, knex); + return new EnforcerDelegate( + enf, + auditLoggerMock, + roleMetadataStorageMock, + knex, + ); } describe('hasPolicy', () => { diff --git a/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.ts b/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.ts index 97d976caa5..c11dd79113 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.ts @@ -25,6 +25,11 @@ import { } from '../database/role-metadata'; import { mergeRoleMetadata, policiesToString, policyToString } from '../helper'; import { MODEL } from './permission-model'; +import { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; +import { + FETCH_NEWER_PERMISSIONS_STAGE, + PoliciesData, +} from '../audit-log/audit-logger'; export type RoleEvents = 'roleAdded'; export interface RoleEventEmitter { @@ -38,12 +43,70 @@ type EventMap = { export class EnforcerDelegate implements RoleEventEmitter { private readonly roleEventEmitter = new EventEmitter(); + private loadPolicyPromise: Promise | null = null; + private semaphore: number = 0; + private editOperationsQueue: Promise[] = []; // Queue to track edit operations + constructor( private readonly enforcer: Enforcer, + private readonly auditLogger: AuditLogger, private readonly roleMetadataStorage: RoleMetadataStorage, private readonly knex: Knex, ) {} + async loadPolicy(): Promise { + if (this.loadPolicyPromise) { + // If a load operation is already in progress, return the cached promise + return this.loadPolicyPromise; + } + + // Increment semaphore to block edits during load + this.semaphore++; + + this.loadPolicyPromise = (async () => { + try { + await this.waitForEditOperationsToFinish(); + + await this.enforcer.loadPolicy(); + } catch (err) { + this.auditLogger.auditLog({ + message: 'Failed to load newer policies from database', + eventName: PoliciesData.FAILED_TO_FETCH_NEWER_PERMISSIONS, + stage: FETCH_NEWER_PERMISSIONS_STAGE, + status: 'failed', + errors: [err], + }); + } finally { + this.semaphore--; + this.loadPolicyPromise = null; + } + })(); + + return this.loadPolicyPromise; + } + + private async waitForEditOperationsToFinish(): Promise { + await Promise.all(this.editOperationsQueue); + } + + async execOperation(operation: Promise): Promise { + this.editOperationsQueue.push(operation); + + let result; + try { + result = await operation; + } catch (err) { + throw err; + } finally { + const index = this.editOperationsQueue.indexOf(operation); + if (index !== -1) { + this.editOperationsQueue.splice(index, 1); + } + } + + return result; + } + on(event: RoleEvents, listener: (role: string) => void): this { this.roleEventEmitter.on(event, listener); return this; @@ -174,28 +237,35 @@ export class EnforcerDelegate implements RoleEventEmitter { policies: string[][], externalTrx?: Knex.Transaction, ): Promise { - if (policies.length === 0) { - return; + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; } - const trx = externalTrx || (await this.knex.transaction()); - - try { - const ok = await this.enforcer.addPolicies(policies); - if (!ok) { - throw new Error( - `Failed to store policies ${policiesToString(policies)}`, - ); - } - if (!externalTrx) { - await trx.commit(); + const addPoliciesOperation = (async () => { + if (policies.length === 0) { + return; } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + + const trx = externalTrx || (await this.knex.transaction()); + + try { + const ok = await this.enforcer.addPolicies(policies); + if (!ok) { + throw new Error( + `Failed to store policies ${policiesToString(policies)}`, + ); + } + if (!externalTrx) { + await trx.commit(); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(addPoliciesOperation); } async addGroupingPolicy( @@ -203,50 +273,57 @@ export class EnforcerDelegate implements RoleEventEmitter { roleMetadata: RoleMetadataDao, externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); - const entityRef = roleMetadata.roleEntityRef; - - if (await this.hasGroupingPolicy(...policy)) { - return; + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; } - try { - let currentMetadata; - if (entityRef.startsWith(`role:`)) { - currentMetadata = await this.roleMetadataStorage.findRoleMetadata( - entityRef, - trx, - ); - } - if (currentMetadata) { - await this.roleMetadataStorage.updateRoleMetadata( - mergeRoleMetadata(currentMetadata, roleMetadata), - entityRef, - trx, - ); - } else { - const currentDate: Date = new Date(); - roleMetadata.createdAt = currentDate.toUTCString(); - roleMetadata.lastModified = currentDate.toUTCString(); - await this.roleMetadataStorage.createRoleMetadata(roleMetadata, trx); - } + const addGroupingPolicyOperation = (async () => { + const trx = externalTrx ?? (await this.knex.transaction()); + const entityRef = roleMetadata.roleEntityRef; - const ok = await this.enforcer.addGroupingPolicy(...policy); - if (!ok) { - throw new Error(`failed to create policy ${policyToString(policy)}`); - } - if (!externalTrx) { - await trx.commit(); - } - if (!currentMetadata) { - this.roleEventEmitter.emit('roleAdded', roleMetadata.roleEntityRef); + if (await this.hasGroupingPolicy(...policy)) { + return; } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + try { + let currentMetadata; + if (entityRef.startsWith(`role:`)) { + currentMetadata = await this.roleMetadataStorage.findRoleMetadata( + entityRef, + trx, + ); + } + + if (currentMetadata) { + await this.roleMetadataStorage.updateRoleMetadata( + mergeRoleMetadata(currentMetadata, roleMetadata), + entityRef, + trx, + ); + } else { + const currentDate: Date = new Date(); + roleMetadata.createdAt = currentDate.toUTCString(); + roleMetadata.lastModified = currentDate.toUTCString(); + await this.roleMetadataStorage.createRoleMetadata(roleMetadata, trx); + } + + const ok = await this.enforcer.addGroupingPolicy(...policy); + if (!ok) { + throw new Error(`failed to create policy ${policyToString(policy)}`); + } + if (!externalTrx) { + await trx.commit(); + } + if (!currentMetadata) { + this.roleEventEmitter.emit('roleAdded', roleMetadata.roleEntityRef); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(addGroupingPolicyOperation); } async addGroupingPolicies( @@ -254,50 +331,57 @@ export class EnforcerDelegate implements RoleEventEmitter { roleMetadata: RoleMetadataDao, externalTrx?: Knex.Transaction, ): Promise { - if (policies.length === 0) { - return; + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; } - const trx = externalTrx ?? (await this.knex.transaction()); - - try { - const currentRoleMetadata = - await this.roleMetadataStorage.findRoleMetadata( - roleMetadata.roleEntityRef, - trx, - ); - if (currentRoleMetadata) { - await this.roleMetadataStorage.updateRoleMetadata( - mergeRoleMetadata(currentRoleMetadata, roleMetadata), - roleMetadata.roleEntityRef, - trx, - ); - } else { - const currentDate: Date = new Date(); - roleMetadata.createdAt = currentDate.toUTCString(); - roleMetadata.lastModified = currentDate.toUTCString(); - await this.roleMetadataStorage.createRoleMetadata(roleMetadata, trx); + const addGroupingPoliciesOperation = (async () => { + if (policies.length === 0) { + return; } - const ok = await this.enforcer.addGroupingPolicies(policies); - if (!ok) { - throw new Error( - `Failed to store policies ${policiesToString(policies)}`, - ); - } + const trx = externalTrx ?? (await this.knex.transaction()); - if (!externalTrx) { - await trx.commit(); - } - if (!currentRoleMetadata) { - this.roleEventEmitter.emit('roleAdded', roleMetadata.roleEntityRef); - } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + try { + const currentRoleMetadata = + await this.roleMetadataStorage.findRoleMetadata( + roleMetadata.roleEntityRef, + trx, + ); + if (currentRoleMetadata) { + await this.roleMetadataStorage.updateRoleMetadata( + mergeRoleMetadata(currentRoleMetadata, roleMetadata), + roleMetadata.roleEntityRef, + trx, + ); + } else { + const currentDate: Date = new Date(); + roleMetadata.createdAt = currentDate.toUTCString(); + roleMetadata.lastModified = currentDate.toUTCString(); + await this.roleMetadataStorage.createRoleMetadata(roleMetadata, trx); + } + + const ok = await this.enforcer.addGroupingPolicies(policies); + if (!ok) { + throw new Error( + `Failed to store policies ${policiesToString(policies)}`, + ); + } + + if (!externalTrx) { + await trx.commit(); + } + if (!currentRoleMetadata) { + this.roleEventEmitter.emit('roleAdded', roleMetadata.roleEntityRef); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(addGroupingPoliciesOperation); } async updateGroupingPolicies( @@ -343,47 +427,61 @@ export class EnforcerDelegate implements RoleEventEmitter { } async removePolicy(policy: string[], externalTrx?: Knex.Transaction) { - const trx = externalTrx ?? (await this.knex.transaction()); + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; + } - try { - const ok = await this.enforcer.removePolicy(...policy); - if (!ok) { - throw new Error(`fail to delete policy ${policy}`); - } - if (!externalTrx) { - await trx.commit(); - } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + const removePolicyOperation = (async () => { + const trx = externalTrx ?? (await this.knex.transaction()); + + try { + const ok = await this.enforcer.removePolicy(...policy); + if (!ok) { + throw new Error(`fail to delete policy ${policy}`); + } + if (!externalTrx) { + await trx.commit(); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(removePolicyOperation); } async removePolicies( policies: string[][], externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; + } - try { - const ok = await this.enforcer.removePolicies(policies); - if (!ok) { - throw new Error( - `Failed to delete policies ${policiesToString(policies)}`, - ); - } + const removePoliciesOperation = (async () => { + const trx = externalTrx ?? (await this.knex.transaction()); - if (!externalTrx) { - await trx.commit(); - } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + try { + const ok = await this.enforcer.removePolicies(policies); + if (!ok) { + throw new Error( + `Failed to delete policies ${policiesToString(policies)}`, + ); + } + + if (!externalTrx) { + await trx.commit(); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(removePoliciesOperation); } async removeGroupingPolicy( @@ -392,46 +490,53 @@ export class EnforcerDelegate implements RoleEventEmitter { isUpdate?: boolean, externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); - const roleEntity = policy[1]; + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; + } - try { - const ok = await this.enforcer.removeGroupingPolicy(...policy); - if (!ok) { - throw new Error(`Failed to delete policy ${policyToString(policy)}`); - } + const removeGroupingPolicyOperation = (async () => { + const trx = externalTrx ?? (await this.knex.transaction()); + const roleEntity = policy[1]; - if (!isUpdate) { - const currentRoleMetadata = - await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx); - const remainingGroupPolicies = await this.getFilteredGroupingPolicy( - 1, - roleEntity, - ); - if ( - currentRoleMetadata && - remainingGroupPolicies.length === 0 && - roleEntity !== ADMIN_ROLE_NAME - ) { - await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); - } else if (currentRoleMetadata) { - await this.roleMetadataStorage.updateRoleMetadata( - mergeRoleMetadata(currentRoleMetadata, roleMetadata), + try { + const ok = await this.enforcer.removeGroupingPolicy(...policy); + if (!ok) { + throw new Error(`Failed to delete policy ${policyToString(policy)}`); + } + + if (!isUpdate) { + const currentRoleMetadata = + await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx); + const remainingGroupPolicies = await this.getFilteredGroupingPolicy( + 1, roleEntity, - trx, ); + if ( + currentRoleMetadata && + remainingGroupPolicies.length === 0 && + roleEntity !== ADMIN_ROLE_NAME + ) { + await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); + } else if (currentRoleMetadata) { + await this.roleMetadataStorage.updateRoleMetadata( + mergeRoleMetadata(currentRoleMetadata, roleMetadata), + roleEntity, + trx, + ); + } } - } - if (!externalTrx) { - await trx.commit(); - } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + if (!externalTrx) { + await trx.commit(); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(removeGroupingPolicyOperation); } async removeGroupingPolicies( @@ -440,48 +545,56 @@ export class EnforcerDelegate implements RoleEventEmitter { isUpdate?: boolean, externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; + } - const roleEntity = roleMetadata.roleEntityRef; - try { - const ok = await this.enforcer.removeGroupingPolicies(policies); - if (!ok) { - throw new Error( - `Failed to delete grouping policies: ${policiesToString(policies)}`, - ); - } + const removeGroupingPolicyOperation = (async () => { + const trx = externalTrx ?? (await this.knex.transaction()); + const roleEntity = roleMetadata.roleEntityRef; - if (!isUpdate) { - const currentRoleMetadata = - await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx); - const remainingGroupPolicies = await this.getFilteredGroupingPolicy( - 1, - roleEntity, - ); - if ( - currentRoleMetadata && - remainingGroupPolicies.length === 0 && - roleEntity !== ADMIN_ROLE_NAME - ) { - await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); - } else if (currentRoleMetadata) { - await this.roleMetadataStorage.updateRoleMetadata( - mergeRoleMetadata(currentRoleMetadata, roleMetadata), + try { + const ok = await this.enforcer.removeGroupingPolicies(policies); + if (!ok) { + throw new Error( + `Failed to delete grouping policies: ${policiesToString(policies)}`, + ); + } + + if (!isUpdate) { + const currentRoleMetadata = + await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx); + const remainingGroupPolicies = await this.getFilteredGroupingPolicy( + 1, roleEntity, - trx, ); + + if ( + currentRoleMetadata && + remainingGroupPolicies.length === 0 && + roleEntity !== ADMIN_ROLE_NAME + ) { + await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); + } else if (currentRoleMetadata) { + await this.roleMetadataStorage.updateRoleMetadata( + mergeRoleMetadata(currentRoleMetadata, roleMetadata), + roleEntity, + trx, + ); + } } - } - if (!externalTrx) { - await trx.commit(); - } - } catch (err) { - if (!externalTrx) { - await trx.rollback(err); + if (!externalTrx) { + await trx.commit(); + } + } catch (err) { + if (!externalTrx) { + await trx.rollback(err); + } + throw err; } - throw err; - } + })(); + await this.execOperation(removeGroupingPolicyOperation); } /** @@ -498,7 +611,8 @@ export class EnforcerDelegate implements RoleEventEmitter { * The temporary enforcer has lazy loading of the permission policies enabled to reduce the amount * of time it takes to initialize the temporary enforcer. * The justification for lazy loading is because permission policies are already present in the - * role manager / database and it will be filtered and loaded whenever `loadFilteredPolicy` is called. + * role manager / database and it will be filtered and loaded whenever `getFilteredPolicy` is called + * and permissions / roles are applied to the temp enforcer * @param entityRef The user to enforce * @param resourceType The resource type / name of the permission policy * @param action The action of the permission policy @@ -512,35 +626,59 @@ export class EnforcerDelegate implements RoleEventEmitter { action: string, roles: string[], ): Promise { - const filter = []; - if (roles.length > 0) { - roles.forEach(role => { - filter.push({ ptype: 'p', v0: role, v1: resourceType, v2: action }); - }); - } else { - filter.push({ ptype: 'p', v1: resourceType, v2: action }); + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; } - const adapt = this.enforcer.getAdapter(); - const roleManager = this.enforcer.getRoleManager(); - const tempEnforcer = new Enforcer(); - await tempEnforcer.initWithModelAndAdapter( - newModelFromString(MODEL), - adapt, - true, - ); - tempEnforcer.setRoleManager(roleManager); + const evaluatePermissionOperation = (async () => { + const filter = []; + if (roles.length > 0) { + roles.forEach(role => { + filter.push({ ptype: 'p', v0: role, v1: resourceType, v2: action }); + }); + } else { + filter.push({ ptype: 'p', v1: resourceType, v2: action }); + } + + const adapt = this.enforcer.getAdapter(); + const roleManager = this.enforcer.getRoleManager(); + const tempEnforcer = new Enforcer(); + await tempEnforcer.initWithModelAndAdapter( + newModelFromString(MODEL), + adapt, + true, + ); + tempEnforcer.setRoleManager(roleManager); + + await tempEnforcer.loadFilteredPolicy(filter); - await tempEnforcer.loadFilteredPolicy(filter); + return await tempEnforcer.enforce(entityRef, resourceType, action); + })(); - return await tempEnforcer.enforce(entityRef, resourceType, action); + return await this.execOperation(evaluatePermissionOperation); } async getImplicitPermissionsForUser(user: string): Promise { - return this.enforcer.getImplicitPermissionsForUser(user); + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; + } + + const getPermissionsForUserOperation = (async () => { + return this.enforcer.getImplicitPermissionsForUser(user); + })(); + + return await this.execOperation(getPermissionsForUserOperation); } async getAllRoles(): Promise { - return this.enforcer.getAllRoles(); + if (this.loadPolicyPromise) { + await this.loadPolicyPromise; + } + + const getRolesOperation = (async () => { + return this.enforcer.getAllRoles(); + })(); + + return await this.execOperation(getRolesOperation); } } diff --git a/workspaces/rbac/plugins/rbac-backend/src/service/policy-builder.ts b/workspaces/rbac/plugins/rbac-backend/src/service/policy-builder.ts index 099a9a5119..272deb7721 100644 --- a/workspaces/rbac/plugins/rbac-backend/src/service/policy-builder.ts +++ b/workspaces/rbac/plugins/rbac-backend/src/service/policy-builder.ts @@ -125,17 +125,17 @@ export class PolicyBuilder { const conditionStorage = new DataBaseConditionalStorage(databaseClient); const roleMetadataStorage = new DataBaseRoleMetadataStorage(databaseClient); - const enforcerDelegate = new EnforcerDelegate( - enf, - roleMetadataStorage, - databaseClient, - ); - const defAuditLog = new DefaultAuditLogger({ logger: env.logger, authService: env.auth, httpAuthService: env.httpAuth, }); + const enforcerDelegate = new EnforcerDelegate( + enf, + defAuditLog, + roleMetadataStorage, + databaseClient, + ); if (rbacProviders) { await connectRBACProviders( diff --git a/workspaces/rbac/yarn.lock b/workspaces/rbac/yarn.lock index 056abc421e..e94de964a5 100644 --- a/workspaces/rbac/yarn.lock +++ b/workspaces/rbac/yarn.lock @@ -2696,6 +2696,7 @@ __metadata: "@janus-idp/backstage-plugin-audit-log-node": ^1.7.1 "@spotify/prettier-config": ^15.0.0 "@types/express": 4.17.21 + "@types/knex": ^0.16.1 "@types/lodash": ^4.14.151 "@types/node": 18.19.68 "@types/supertest": 2.0.16 @@ -13094,6 +13095,15 @@ __metadata: languageName: node linkType: hard +"@types/knex@npm:^0.16.1": + version: 0.16.1 + resolution: "@types/knex@npm:0.16.1" + dependencies: + knex: "*" + checksum: daf16bef279e68a11c74ad5d351237c6667ea9921ebe73e613d24b26acc25ba86b40fdc9d8adddfe1a0d234f20336adde6e4a2d062ae7ebe74a2c288ac6ccdc5 + languageName: node + linkType: hard + "@types/lodash@npm:^4.14.151": version: 4.17.13 resolution: "@types/lodash@npm:4.17.13" @@ -24005,7 +24015,7 @@ __metadata: languageName: node linkType: hard -"knex@npm:3, knex@npm:^3.0.0": +"knex@npm:*, knex@npm:3, knex@npm:^3.0.0": version: 3.1.0 resolution: "knex@npm:3.1.0" dependencies: