fix(rbac): fix role list view permission policies column value (janus…

…-idp#1714) * fix(rbac): fix role list view permission policies column value Signed-off-by: Yi Cai <[email protected]> * Update for review comments Signed-off-by: Yi Cai <[email protected]> * Fixed lint issues Signed-off-by: Yi Cai <[email protected]> * Addressed comments Signed-off-by: Yi Cai <[email protected]> * Fixed prettier issue Signed-off-by: Yi Cai <[email protected]> * Updated unit tests Signed-off-by: Yi Cai <[email protected]> * Addressed review comments Signed-off-by: Yi Cai <[email protected]> * Addressed review comments Signed-off-by: Yi Cai <[email protected]> --------- Signed-off-by: Yi Cai <[email protected]>
04kash · Jun 4, 2024 · 07200e4 · 07200e4
1 parent 87c6053
commit 07200e4
Show file tree

Hide file tree

Showing 5 changed files with 144 additions and 33 deletions.
diff --git a/plugins/rbac/src/components/RbacPage.test.tsx b/plugins/rbac/src/components/RbacPage.test.tsx
@@ -40,6 +40,7 @@ describe('RbacPage', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,

diff --git a/plugins/rbac/src/components/RolesList/RolesList.test.tsx b/plugins/rbac/src/components/RolesList/RolesList.test.tsx
@@ -66,6 +66,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -87,6 +88,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -108,6 +110,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -144,6 +147,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -182,6 +186,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: true,
@@ -203,6 +208,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -224,6 +230,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: true,
@@ -245,6 +252,7 @@ describe('RolesList', () => {
       error: {
         rolesError: '',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -264,6 +272,7 @@ describe('RolesList', () => {
       error: {
         rolesError: 'Something went wrong',
         policiesError: '',
+        roleConditionError: '',
       },
       retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
@@ -273,4 +282,29 @@ describe('RolesList', () => {
     const { queryByText } = await renderInTestApp(<RolesList />);
     expect(queryByText('Something went wrong')).toBeInTheDocument();
   });
+
+  it('should show error message when there is an error fetching the role conditions', async () => {
+    RequirePermissionMock.mockImplementation(props => <>{props.children}</>);
+    mockUsePermission.mockReturnValue({ loading: false, allowed: true });
+    mockUseRoles.mockReturnValue({
+      loading: true,
+      data: [],
+      error: {
+        rolesError: '',
+        policiesError: '',
+        roleConditionError:
+          'Error fetching role conditions for role role:default/xyz, please try again later.',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
+      createRoleAllowed: false,
+      createRoleLoading: false,
+    });
+
+    const { queryByText } = await renderInTestApp(<RolesList />);
+    expect(
+      queryByText(
+        'Error fetching role conditions for role role:default/xyz, please try again later.',
+      ),
+    ).toBeInTheDocument();
+  });
 });
diff --git a/plugins/rbac/src/components/RolesList/RolesList.tsx b/plugins/rbac/src/components/RolesList/RolesList.tsx
@@ -44,22 +44,42 @@ export const RolesList = () => {
     setRoles(searchResults.length);
   };
 
+  const getErrorWarning = () => {
+    const errorTitleBase = 'Something went wrong while fetching the';
+    const errorWarningArr = [
+      { message: error?.rolesError, title: `${errorTitleBase} roles` },
+      {
+        message: error?.policiesError,
+        title: `${errorTitleBase} permission policies`,
+      },
+      {
+        message: error?.roleConditionError,
+        title: `${errorTitleBase} role conditions`,
+      },
+    ];
+
+    return (
+      errorWarningArr.find(({ message }) => message) || {
+        message: '',
+        title: '',
+      }
+    );
+  };
+
+  const errorWarning = getErrorWarning();
+
   return (
     <>
       <SnackbarAlert toastMessage={toastMessage} onAlertClose={onAlertClose} />
       <RolesListToolbar
         createRoleAllowed={createRoleAllowed}
         createRoleLoading={createRoleLoading}
       />
-      {(error?.rolesError || error?.policiesError) && (
+      {errorWarning.message && (
         <div style={{ paddingBottom: '16px' }}>
           <WarningPanel
-            message={error.rolesError || error.policiesError}
-            title={
-              error.rolesError
-                ? 'Something went wrong while fetching the roles'
-                : 'Something went wrong while fetching the permission policies'
-            }
+            message={errorWarning.message}
+            title={errorWarning.title}
             severity="error"
           />
         </div>

diff --git a/plugins/rbac/src/hooks/useRoles.test.ts b/plugins/rbac/src/hooks/useRoles.test.ts
@@ -63,6 +63,7 @@ jest.mock('@backstage/core-plugin-api', () => ({
           effect: 'allow',
         },
       ]),
+    getRoleConditions: jest.fn().mockReturnValue([]),
   }),
 }));
 

diff --git a/plugins/rbac/src/hooks/useRoles.ts b/plugins/rbac/src/hooks/useRoles.ts
@@ -16,6 +16,10 @@ import { rbacApiRef } from '../api/RBACBackendClient';
 import { RolesData } from '../types';
 import { getPermissions } from '../utils/rbac-utils';
 
+type RoleWithConditionalPoliciesCount = Role & {
+  conditionalPoliciesCount: number;
+};
+
 export const useRoles = (
   pollInterval?: number,
 ): {
@@ -26,10 +30,16 @@ export const useRoles = (
   error: {
     rolesError: string;
     policiesError: string;
+    roleConditionError: string;
   };
   retry: { roleRetry: () => void; policiesRetry: () => void };
 } => {
   const rbacApi = useApi(rbacApiRef);
+  const [newRoles, setNewRoles] = React.useState<
+    RoleWithConditionalPoliciesCount[]
+  >([]);
+  const [roleConditionError, setRoleConditionError] =
+    React.useState<string>('');
   const {
     value: roles,
     retry: roleRetry,
@@ -76,39 +86,83 @@ export const useRoles = (
     permission: policyEntityUpdatePermission,
     resourceRef: policyEntityUpdatePermission.resourceType,
   });
+
+  React.useEffect(() => {
+    const fetchAllPermissionPolicies = async () => {
+      if (!Array.isArray(roles)) return;
+      const failedFetchConditionRoles: string[] = [];
+      const conditionPromises = roles.map(async role => {
+        try {
+          const conditionalPolicies = await rbacApi.getRoleConditions(
+            role.name,
+          );
+
+          if ((conditionalPolicies as any as Response)?.statusText) {
+            failedFetchConditionRoles.push(role.name);
+            throw new Error(
+              (conditionalPolicies as any as Response).statusText,
+            );
+          }
+          return {
+            ...role,
+            conditionalPoliciesCount: Array.isArray(conditionalPolicies)
+              ? conditionalPolicies.length
+              : 0,
+          };
+        } catch (error) {
+          setRoleConditionError(
+            `Error fetching role conditions for ${failedFetchConditionRoles.length > 1 ? 'roles' : 'role'} ${failedFetchConditionRoles.join(', ')}, please try again later.`,
+          );
+          return {
+            ...role,
+            conditionalPoliciesCount: 0,
+          };
+        }
+      });
+
+      const updatedRoles = await Promise.all(conditionPromises);
+      setNewRoles(updatedRoles);
+    };
+
+    fetchAllPermissionPolicies();
+  }, [roles, rbacApi]);
+
   const data: RolesData[] = React.useMemo(
     () =>
-      Array.isArray(roles) && roles?.length > 0
-        ? roles.reduce((acc: RolesData[], role: Role) => {
-            const permissions = getPermissions(
-              role.name,
-              policies as RoleBasedPolicy[],
-            );
+      Array.isArray(newRoles) && newRoles?.length > 0
+        ? newRoles.reduce(
+            (acc: RolesData[], role: RoleWithConditionalPoliciesCount) => {
+              const permissions = getPermissions(
+                role.name,
+                policies as RoleBasedPolicy[],
+              );
 
-            return [
-              ...acc,
-              {
-                id: role.name,
-                name: role.name,
-                description: role.metadata?.description ?? '-',
-                members: role.memberReferences,
-                permissions,
-                modifiedBy: '-',
-                lastModified: '-',
-                actionsPermissionResults: {
-                  delete: deletePermissionResult,
-                  edit: {
-                    allowed:
-                      editPermissionResult.allowed && canReadUsersAndGroups,
-                    loading: editPermissionResult.loading,
+              return [
+                ...acc,
+                {
+                  id: role.name,
+                  name: role.name,
+                  description: role.metadata?.description ?? '-',
+                  members: role.memberReferences,
+                  permissions: role.conditionalPoliciesCount + permissions,
+                  modifiedBy: '-',
+                  lastModified: '-',
+                  actionsPermissionResults: {
+                    delete: deletePermissionResult,
+                    edit: {
+                      allowed:
+                        editPermissionResult.allowed && canReadUsersAndGroups,
+                      loading: editPermissionResult.loading,
+                    },
                   },
                 },
-              },
-            ];
-          }, [])
+              ];
+            },
+            [],
+          )
         : [],
     [
-      roles,
+      newRoles,
       policies,
       deletePermissionResult,
       editPermissionResult.allowed,
@@ -138,6 +192,7 @@ export const useRoles = (
         (typeof policies === 'object'
           ? (policies as any as Response)?.statusText
           : '')) as string,
+      roleConditionError,
     },
     createRoleLoading,
     createRoleAllowed,
-Original file line number
+Diff line change
@@ Expand Up / @@ -63,6 +63,7 @@ jest.mock('@backstage/core-plugin-api', () => ({ @@
               effect: 'allow',
             },
           ]),
+        getRoleConditions: jest.fn().mockReturnValue([]),
       }),
     }));
@@ Expand Down @@