fix(rbac): remove admin metadata, when all admins removed from config (…

…#1314) * fix(rbac): remove admin metadata, when all admins removed from config Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): count super users in warning message about no admins Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): remove role, when it really unused Signed-off-by: Oleksandr Andriienko <[email protected]> --------- Signed-off-by: Oleksandr Andriienko <[email protected]>
janus-idp · Mar 14, 2024 · cc6555e · cc6555e
1 parent c2c9e22
commit cc6555e
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -127,6 +127,8 @@ const mockEnforcer: Partial<EnforcerDelegate> = {
   updatePolicies: jest.fn().mockImplementation(),
 
   updateGroupingPolicies: jest.fn().mockImplementation(),
+
+  addOrUpdateGroupingPolicies: jest.fn().mockImplementation(),
 };
 
 const roleMetadataStorageMock: RoleMetadataStorage = {
-Original file line number
+Diff line change
@@ Expand Up / @@ -90,7 +90,7 @@ describe('helper.ts', () => { @@
           expect(mockEnforcerDelegate.removeGroupingPolicy).toHaveBeenCalledWith(
             ['user:default/admin', roleName],
             source,
-            true,
+            false,
           );
         });
@@ Expand Down @@