fix(rbac): correct plugin ID matching to permission policy (#1795)

* fix(rbac): correct plugin ID matching to permission policy Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): refactor types for plugin policies list Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): fix broken getAllPolicies method Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): update apis.md file and fix import Signed-off-by: Oleksandr Andriienko <[email protected]> --------- Signed-off-by: Oleksandr Andriienko <[email protected]>
janus-idp · Jun 28, 2024 · 6dc4b1c · 6dc4b1c
1 parent 23a143d
commit 6dc4b1c
Show file tree

Hide file tree

Showing 12 changed files with 167 additions and 127 deletions.
diff --git a/plugins/rbac-backend/docs/apis.md b/plugins/rbac-backend/docs/apis.md
@@ -461,40 +461,36 @@ Returns:
 [
   {
     "pluginId": "catalog",
-    "policies": [
+      "policies": [
       {
-        "isResourced": true,
-        "permission": "catalog-entity",
-        "policy": "read"
+        "name": "catalog.entity.read",
+        "policy": "read",
+        "resourceType": "catalog-entity"
       },
       {
-        "isResourced": false,
-        "permission": "catalog.entity.create",
+        "name": "catalog.entity.create",
         "policy": "create"
       },
       {
-        "isResourced": true,
-        "permission": "catalog-entity",
-        "policy": "delete"
+        "name": "catalog.entity.delete",
+        "policy": "delete",
+        "resourceType": "catalog-entity"
       },
       {
-        "isResourced": true,
-        "permission": "catalog-entity",
-        "policy": "update"
+        "name": "catalog.entity.refresh",
+        "policy": "update",
+        "resourceType": "catalog-entity"
       },
       {
-        "isResourced": false,
-        "permission": "catalog.location.read",
+        "name": "catalog.location.read",
         "policy": "read"
       },
       {
-        "isResourced": false,
-        "permission": "catalog.location.create",
+        "name": "catalog.location.create",
         "policy": "create"
       },
       {
-        "isResourced": false,
-        "permission": "catalog.location.delete",
+        "name": "catalog.location.delete",
         "policy": "delete"
       }
     ]

diff --git a/plugins/rbac-backend/src/audit-log/audit-logger.ts b/plugins/rbac-backend/src/audit-log/audit-logger.ts
@@ -90,7 +90,6 @@ export const ConditionEvents = {
 };
 
 export type ConditionAuditInfo = {
-  conditionId?: number;
   condition: RoleConditionalPolicyDecision<PermissionAction>;
 };
 

diff --git a/plugins/rbac-backend/src/service/plugin-endpoint.test.ts b/plugins/rbac-backend/src/service/plugin-endpoint.test.ts
@@ -91,8 +91,8 @@ describe('plugin-endpoint', () => {
       expect(policiesMetadata[0].pluginId).toEqual('permission');
       expect(policiesMetadata[0].policies).toEqual([
         {
-          isResourced: true,
-          permission: 'policy-entity',
+          name: 'policy.entity.read',
+          resourceType: 'policy-entity',
           policy: 'read',
         },
       ]);
@@ -118,8 +118,7 @@ describe('plugin-endpoint', () => {
       expect(policiesMetadata[0].pluginId).toEqual('permission');
       expect(policiesMetadata[0].policies).toEqual([
         {
-          isResourced: false,
-          permission: 'catalog.entity.create',
+          name: 'catalog.entity.create',
           policy: 'create',
         },
       ]);
@@ -158,8 +157,8 @@ describe('plugin-endpoint', () => {
       expect(policiesMetadata[0].pluginId).toEqual('permission');
       expect(policiesMetadata[0].policies).toEqual([
         {
-          isResourced: true,
-          permission: 'policy-entity',
+          name: 'policy.entity.read',
+          resourceType: 'policy-entity',
           policy: 'read',
         },
       ]);
@@ -200,8 +199,8 @@ describe('plugin-endpoint', () => {
       expect(policiesMetadata[0].pluginId).toEqual('permission');
       expect(policiesMetadata[0].policies).toEqual([
         {
-          isResourced: true,
-          permission: 'policy-entity',
+          name: 'policy.entity.read',
+          resourceType: 'policy-entity',
           policy: 'read',
         },
       ]);
@@ -242,8 +241,8 @@ describe('plugin-endpoint', () => {
       expect(policiesMetadata[0].pluginId).toEqual('permission');
       expect(policiesMetadata[0].policies).toEqual([
         {
-          isResourced: true,
-          permission: 'policy-entity',
+          name: 'policy.entity.read',
+          resourceType: 'policy-entity',
           policy: 'read',
         },
       ]);

diff --git a/plugins/rbac-backend/src/service/plugin-endpoints.ts b/plugins/rbac-backend/src/service/plugin-endpoints.ts
@@ -20,7 +20,10 @@ import {
   MetadataResponseSerializedRule,
 } from '@backstage/plugin-permission-node';
 
-import { Policy } from '@janus-idp/backstage-plugin-rbac-common';
+import {
+  PluginPermissionMetaData,
+  PolicyDetails,
+} from '@janus-idp/backstage-plugin-rbac-common';
 import { PluginIdProvider } from '@janus-idp/backstage-plugin-rbac-node';
 
 type PluginMetadataResponse = {
@@ -33,11 +36,6 @@ export type PluginMetadataResponseSerializedRule = {
   rules: MetadataResponseSerializedRule[];
 };
 
-export type PluginPermissionMetaData = {
-  pluginId: string;
-  policies: Policy[];
-};
-
 export class PluginPermissionMetadataCollector {
   private readonly pluginIds: string[];
   private urlReader: UrlReaderService;
@@ -147,20 +145,21 @@ export class PluginPermissionMetadataCollector {
   }
 }
 
-function permissionsToCasbinPolicies(permissions: Permission[]): Policy[] {
-  const policies: Policy[] = [];
+function permissionsToCasbinPolicies(
+  permissions: Permission[],
+): PolicyDetails[] {
+  const policies: PolicyDetails[] = [];
   for (const permission of permissions) {
     if (isResourcePermission(permission)) {
       policies.push({
-        permission: permission.resourceType,
+        resourceType: permission.resourceType,
+        name: permission.name,
         policy: permission.attributes.action || 'use',
-        isResourced: true,
       });
     } else {
       policies.push({
-        permission: permission.name,
+        name: permission.name,
         policy: permission.attributes.action || 'use',
-        isResourced: false,
       });
     }
   }

diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts
@@ -15,6 +15,7 @@ import {
   PermissionAction,
   PermissionInfo,
   PermissionPolicyMetadata,
+  PluginPermissionMetaData,
   policyEntityCreatePermission,
   policyEntityDeletePermission,
   policyEntityReadPermission,
@@ -32,7 +33,6 @@ import { EnforcerDelegate } from './enforcer-delegate';
 import { RBACPermissionPolicy } from './permission-policy';
 import {
   PluginMetadataResponseSerializedRule,
-  PluginPermissionMetaData,
   PluginPermissionMetadataCollector,
 } from './plugin-endpoints';
 import { PoliciesServer } from './policies-rest-api';
@@ -3440,7 +3440,8 @@ describe('REST policies api', () => {
           pluginId: 'permissions',
           policies: [
             {
-              permission: 'policy-entity',
+              name: 'catalog.entity.read',
+              resourceType: 'policy-entity',
               policy: 'read',
             },
           ],

diff --git a/plugins/rbac-common/src/types.ts b/plugins/rbac-common/src/types.ts
@@ -25,14 +25,13 @@ export type RoleMetadata = {
 
 export type Policy = {
   permission?: string;
-  isResourced?: boolean;
   policy?: string;
-  effect?: string;
-  metadata?: PermissionPolicyMetadata;
 };
 
 export type RoleBasedPolicy = Policy & {
   entityReference?: string;
+  effect?: string;
+  metadata?: PermissionPolicyMetadata;
 };
 
 export type Role = {
@@ -46,9 +45,27 @@ export type UpdatePolicy = {
   newPolicy: Policy;
 };
 
-export type PermissionPolicy = {
-  pluginId?: string;
-  policies?: Policy[];
+export type NamedPolicy = {
+  name: string;
+
+  policy: string;
+};
+
+export type ResourcedPolicy = NamedPolicy & {
+  resourceType: string;
+};
+
+export type PolicyDetails = NamedPolicy | ResourcedPolicy;
+
+export function isResourcedPolicy(
+  policy: PolicyDetails,
+): policy is ResourcedPolicy {
+  return 'resourceType' in policy;
+}
+
+export type PluginPermissionMetaData = {
+  pluginId: string;
+  policies: PolicyDetails[];
 };
 
 export type NonEmptyArray<T> = [T, ...T[]];

diff --git a/plugins/rbac/dev/index.tsx b/plugins/rbac/dev/index.tsx
@@ -13,7 +13,7 @@ import { createDevAppThemes } from '@redhat-developer/red-hat-developer-hub-them
 
 import {
   PermissionAction,
-  PermissionPolicy,
+  PluginPermissionMetaData,
   Role,
   RoleBasedPolicy,
   RoleConditionalPolicyDecision,
@@ -82,7 +82,7 @@ class MockRBACApi implements RBACAPI {
     return mockMembers;
   }
 
-  async listPermissions(): Promise<PermissionPolicy[]> {
+  async listPermissions(): Promise<PluginPermissionMetaData[]> {
     return mockPermissionPolicies;
   }
 

diff --git a/plugins/rbac/src/__fixtures__/mockPermissionPolicies.ts b/plugins/rbac/src/__fixtures__/mockPermissionPolicies.ts
@@ -1,38 +1,38 @@
-import { PermissionPolicy } from '@janus-idp/backstage-plugin-rbac-common';
+import { PluginPermissionMetaData } from '@janus-idp/backstage-plugin-rbac-common';
 
-export const mockPermissionPolicies: PermissionPolicy[] = [
+export const mockPermissionPolicies: PluginPermissionMetaData[] = [
   {
     pluginId: 'catalog',
     policies: [
       {
-        permission: 'catalog-entity',
+        resourceType: 'catalog-entity',
+        name: 'catalog.entity.read',
         policy: 'read',
-        isResourced: true,
       },
       {
-        permission: 'catalog.entity.create',
+        name: 'catalog.entity.create',
         policy: 'create',
       },
       {
-        permission: 'catalog-entity',
+        resourceType: 'catalog-entity',
+        name: 'catalog.entity.delete',
         policy: 'delete',
-        isResourced: true,
       },
       {
-        permission: 'catalog-entity',
+        resourceType: 'catalog-entity',
+        name: 'catalog.entity.update',
         policy: 'update',
-        isResourced: true,
       },
       {
-        permission: 'catalog.location.read',
+        name: 'catalog.location.read',
         policy: 'read',
       },
       {
-        permission: 'catalog.location.create',
+        name: 'catalog.location.create',
         policy: 'create',
       },
       {
-        permission: 'catalog.location.delete',
+        name: 'catalog.location.delete',
         policy: 'delete',
       },
     ],
@@ -41,39 +41,39 @@ export const mockPermissionPolicies: PermissionPolicy[] = [
     pluginId: 'scaffolder',
     policies: [
       {
-        permission: 'scaffolder-template',
+        resourceType: 'scaffolder-template',
+        name: 'scaffolder.template.read',
         policy: 'read',
-        isResourced: true,
       },
       {
-        permission: 'scaffolder-template',
+        resourceType: 'scaffolder-template',
+        name: 'scaffolder.template.read',
         policy: 'read',
-        isResourced: true,
       },
       {
-        permission: 'scaffolder-action',
+        resourceType: 'scaffolder-action',
+        name: 'scaffolder.action.use',
         policy: 'use',
-        isResourced: true,
       },
     ],
   },
   {
     pluginId: 'permission',
     policies: [
       {
-        permission: 'policy-entity',
+        name: 'policy-entity',
         policy: 'read',
       },
       {
-        permission: 'policy-entity',
+        name: 'policy-entity',
         policy: 'create',
       },
       {
-        permission: 'policy-entity',
+        name: 'policy-entity',
         policy: 'delete',
       },
       {
-        permission: 'policy-entity',
+        name: 'policy-entity',
         policy: 'update',
       },
     ],

diff --git a/plugins/rbac/src/api/RBACBackendClient.ts b/plugins/rbac/src/api/RBACBackendClient.ts
@@ -6,7 +6,7 @@ import {
 
 import {
   PermissionAction,
-  PermissionPolicy,
+  PluginPermissionMetaData,
   Role,
   RoleBasedPolicy,
   RoleConditionalPolicyDecision,
@@ -31,7 +31,7 @@ export type RBACAPI = {
   deleteRole: (role: string) => Promise<Response>;
   getRole: (role: string) => Promise<Role[] | Response>;
   getMembers: () => Promise<MemberEntity[] | Response>;
-  listPermissions: () => Promise<PermissionPolicy[] | Response>;
+  listPermissions: () => Promise<PluginPermissionMetaData[] | Response>;
   createRole: (role: Role) => Promise<RoleError | Response>;
   updateRole: (oldRole: Role, newRole: Role) => Promise<RoleError | Response>;
   updatePolicies: (
-Original file line number
+Diff line change
@@ Expand Up / @@ -90,7 +90,6 @@ export const ConditionEvents = { @@
     };
     export type ConditionAuditInfo = {
-      conditionId?: number;
       condition: RoleConditionalPolicyDecision<PermissionAction>;
     };
@@ Expand Down @@