diff --git a/plugins/rbac-backend/src/helper.test.ts b/plugins/rbac-backend/src/helper.test.ts index ce28651b60..e0532c77ed 100644 --- a/plugins/rbac-backend/src/helper.test.ts +++ b/plugins/rbac-backend/src/helper.test.ts @@ -90,7 +90,7 @@ describe('helper.ts', () => { expect(mockEnforcerDelegate.removeGroupingPolicy).toHaveBeenCalledWith( ['user:default/admin', roleName], source, - true, + false, ); }); diff --git a/plugins/rbac-backend/src/helper.ts b/plugins/rbac-backend/src/helper.ts index 2c4ff3a681..9f2af96dd2 100644 --- a/plugins/rbac-backend/src/helper.ts +++ b/plugins/rbac-backend/src/helper.ts @@ -35,7 +35,7 @@ export async function removeTheDifference( for (const missingRole of missing) { const role = [missingRole, roleName]; - await enf.removeGroupingPolicy(role, source, true); + await enf.removeGroupingPolicy(role, source, false); } } diff --git a/plugins/rbac-backend/src/service/enforcer-delegate.ts b/plugins/rbac-backend/src/service/enforcer-delegate.ts index 8a67854a77..02dc9c4e36 100644 --- a/plugins/rbac-backend/src/service/enforcer-delegate.ts +++ b/plugins/rbac-backend/src/service/enforcer-delegate.ts @@ -349,7 +349,21 @@ export class EnforcerDelegate { ); await this.policyMetadataStorage.removePolicyMetadata(policy, trx); if (!isUpdate) { - await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); + const roleMetadata = await this.roleMetadataStorage.findRoleMetadata( + roleEntity, + trx, + ); + const groupPolicies = await this.enforcer.getFilteredGroupingPolicy( + 1, + roleEntity, + ); + if ( + roleMetadata && + groupPolicies.length === 0 && + roleEntity !== 'role:default/rbac_admin' + ) { + await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); + } } const ok = await this.enforcer.removeGroupingPolicy(...policy); if (!ok) { @@ -376,20 +390,12 @@ export class EnforcerDelegate { const trx = externalTrx ?? (await this.knex.transaction()); try { for (const policy of policies) { - const roleEntity = policy[1]; await this.checkIfPolicyModifiable( policy, source, trx, allowToDeleteCSVFilePolicy, ); - if (!isUpdate) { - if ( - await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx) - ) { - await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); - } - } await this.policyMetadataStorage.removePolicyMetadata(policy, trx); } @@ -400,6 +406,21 @@ export class EnforcerDelegate { ); } + if (!isUpdate) { + const roleEntity = policies[0][1]; + const roleMetadata = await this.roleMetadataStorage.findRoleMetadata( + roleEntity, + trx, + ); + const groupPolicies = await this.enforcer.getFilteredGroupingPolicy( + 1, + roleEntity, + ); + if (roleMetadata && groupPolicies.length === 0) { + await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); + } + } + if (!externalTrx) { await trx.commit(); } diff --git a/plugins/rbac-backend/src/service/permission-policy.test.ts b/plugins/rbac-backend/src/service/permission-policy.test.ts index 9dc687fda5..19eaa0c2da 100644 --- a/plugins/rbac-backend/src/service/permission-policy.test.ts +++ b/plugins/rbac-backend/src/service/permission-policy.test.ts @@ -449,7 +449,12 @@ describe('RBACPermissionPolicy Tests', () => { expect(await enf.getAllRoles()).toEqual(allEnfRoles); - expect(await enf.getPolicy()).toEqual(allEnfPolicies); + const nonAdminPolicies = (await enf.getPolicy()).filter( + (policy: string[]) => { + return policy[0] !== 'role:default/rbac_admin'; + }, + ); + expect(nonAdminPolicies).toEqual(allEnfPolicies); // policy metadata should to be removed expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes( @@ -513,7 +518,10 @@ describe('RBACPermissionPolicy Tests', () => { expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies); - expect(await enf.getPolicy()).toEqual(allEnfPolicies); + const nonAdminPolicies = (await enf.getPolicy()).filter((p: string[]) => { + return p[0] !== 'role:default/rbac_admin'; + }); + expect(nonAdminPolicies).toEqual(allEnfPolicies); // policy metadata should to be removed expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes( @@ -604,7 +612,12 @@ describe('RBACPermissionPolicy Tests', () => { expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies); - expect(await enf.getPolicy()).toEqual(allEnfPolicies); + const nonAdminPolicies = (await enf.getPolicy()).filter( + (policy: string[]) => { + return policy[0] !== 'role:default/rbac_admin'; + }, + ); + expect(nonAdminPolicies).toEqual(allEnfPolicies); // policy metadata should to be removed expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes( @@ -685,7 +698,12 @@ describe('RBACPermissionPolicy Tests', () => { expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies); - expect(await enf.getPolicy()).toEqual(allEnfPolicies); + const nonAdminPolicies = (await enf.getPolicy()).filter( + (policy: string[]) => { + return policy[0] !== 'role:default/rbac_admin'; + }, + ); + expect(nonAdminPolicies).toEqual(allEnfPolicies); // policy metadata should to be removed expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes( @@ -749,7 +767,12 @@ describe('RBACPermissionPolicy Tests', () => { expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies); - expect(await enf.getPolicy()).toEqual(allEnfPolicies); + const nonAdminPolicies = (await enf.getPolicy()).filter( + (policy: string[]) => { + return policy[0] !== 'role:default/rbac_admin'; + }, + ); + expect(nonAdminPolicies).toEqual(allEnfPolicies); // policy metadata should to be removed expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes( @@ -828,7 +851,12 @@ describe('RBACPermissionPolicy Tests', () => { expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies); - expect(await enf.getPolicy()).toEqual(allEnfPolicies); + const nonAdminPolicies = (await enf.getPolicy()).filter( + (policy: string[]) => { + return policy[0] !== 'role:default/rbac_admin'; + }, + ); + expect(nonAdminPolicies).toEqual(allEnfPolicies); // policy metadata should to be removed expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes( diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts index 785b5aa515..3ed0f8e47f 100644 --- a/plugins/rbac-backend/src/service/permission-policy.ts +++ b/plugins/rbac-backend/src/service/permission-policy.ts @@ -177,17 +177,20 @@ export class RBACPermissionPolicy implements PermissionPolicy { } } - if (adminUsers && adminUsers.length > 0) { - await useAdminsFromConfig( - adminUsers, - enforcerDelegate, - roleMetadataStorage, - knex, - ); - await setAdminPermissions(enforcerDelegate); - } else { + await useAdminsFromConfig( + adminUsers || [], + enforcerDelegate, + roleMetadataStorage, + knex, + ); + await setAdminPermissions(enforcerDelegate); + + if ( + (!adminUsers || adminUsers.length === 0) && + (!superUsers || superUsers.length === 0) + ) { logger.warn( - 'There are no admins configured for the RBAC-backend plugin. The plugin may not work properly.', + 'There are no admins or super admins configured for the RBAC-backend plugin.', ); } diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts index b153a9e3dc..d79793025f 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -127,6 +127,8 @@ const mockEnforcer: Partial = { updatePolicies: jest.fn().mockImplementation(), updateGroupingPolicies: jest.fn().mockImplementation(), + + addOrUpdateGroupingPolicies: jest.fn().mockImplementation(), }; const roleMetadataStorageMock: RoleMetadataStorage = {