feat(rbac): show rules count in overview page (#1845)

janus-idp · Jul 8, 2024 · a10dc36 · a10dc36
1 parent 8c36224
commit a10dc36
Show file tree

Hide file tree

Showing 13 changed files with 123 additions and 36 deletions.
diff --git a/plugins/rbac/src/__fixtures__/mockFormValues.tsx b/plugins/rbac/src/__fixtures__/mockFormValues.tsx
@@ -7,6 +7,7 @@ export const mockFormCurrentValues = {
     {
       permission: 'catalog-entity',
       policies: [{ policy: 'read', effect: 'allow' }],
+      policyString: 'Read',
       isResourced: true,
       plugin: 'catalog',
       conditions: {
@@ -32,6 +33,7 @@ export const mockFormInitialValues = {
       id: 1,
       permission: 'catalog-entity',
       policies: [{ policy: 'read', effect: 'allow' }],
+      policyString: 'Read',
       isResourced: true,
       plugin: 'catalog',
       conditions: {

diff --git a/plugins/rbac/src/components/CreateRole/EditRolePage.test.tsx b/plugins/rbac/src/components/CreateRole/EditRolePage.test.tsx
@@ -86,8 +86,11 @@ beforeEach(() => {
   usePermissionPoliciesMock.mockReturnValue({
     loading: false,
     data: usePermissionPoliciesMockData,
-    conditionsData: [],
-    retry: { policiesRetry: jest.fn(), permissionPoliciesRetry: jest.fn() },
+    retry: {
+      policiesRetry: jest.fn(),
+      permissionPoliciesRetry: jest.fn(),
+      conditionalPoliciesRetry: jest.fn(),
+    },
     error: new Error(''),
   });
 

diff --git a/plugins/rbac/src/components/CreateRole/EditRolePage.tsx b/plugins/rbac/src/components/CreateRole/EditRolePage.tsx
@@ -30,19 +30,17 @@ export const EditRolePage = () => {
     roleName ? `${roleKind}:${roleNamespace}/${roleName}` : '',
   );
 
-  const {
-    data: permissionPolicies,
-    conditionsData,
-    loading: loadingPolicies,
-  } = usePermissionPolicies(`${roleKind}:${roleNamespace}/${roleName}`);
+  const { data, loading: loadingPolicies } = usePermissionPolicies(
+    `${roleKind}:${roleNamespace}/${roleName}`,
+  );
 
   const initialValues: RoleFormValues = {
     name: roleName || '',
     namespace: roleNamespace || 'default',
     kind: roleKind || 'role',
     description: role?.metadata?.description ?? '',
     selectedMembers,
-    permissionPoliciesRows: [...conditionsData, ...permissionPolicies],
+    permissionPoliciesRows: data,
   };
 
   if (loadingMembers || loadingPolicies) {

diff --git a/plugins/rbac/src/components/CreateRole/ReviewStep.tsx b/plugins/rbac/src/components/CreateRole/ReviewStep.tsx
@@ -52,7 +52,7 @@ export const ReviewStep = ({
   isEditing: boolean;
 }) => {
   return (
-    <div style={{ overflow: 'scroll' }}>
+    <div style={{ overflow: 'auto' }}>
       <Typography variant="h6">
         {isEditing ? 'Review and save' : 'Review and create'}
       </Typography>

diff --git a/plugins/rbac/src/components/RoleOverview/PermissionsCard.test.tsx b/plugins/rbac/src/components/RoleOverview/PermissionsCard.test.tsx
@@ -3,6 +3,7 @@ import React from 'react';
 import { usePermission } from '@backstage/plugin-permission-react';
 import { renderInTestApp } from '@backstage/test-utils';
 
+import { mockFormInitialValues } from '../../__fixtures__/mockFormValues';
 import { usePermissionPolicies } from '../../hooks/usePermissionPolicies';
 import { PermissionsData } from '../../types';
 import { PermissionsCard } from './PermissionsCard';
@@ -50,8 +51,11 @@ describe('PermissionsCard', () => {
     mockPermissionPolicies.mockReturnValue({
       loading: false,
       data: usePermissionPoliciesMockData,
-      conditionsData: [],
-      retry: { policiesRetry: jest.fn(), permissionPoliciesRetry: jest.fn() },
+      retry: {
+        policiesRetry: jest.fn(),
+        permissionPoliciesRetry: jest.fn(),
+        conditionalPoliciesRetry: jest.fn(),
+      },
       error: new Error(''),
     });
     const { queryByText } = await renderInTestApp(
@@ -69,8 +73,11 @@ describe('PermissionsCard', () => {
     mockPermissionPolicies.mockReturnValue({
       loading: false,
       data: [],
-      conditionsData: [],
-      retry: { policiesRetry: jest.fn(), permissionPoliciesRetry: jest.fn() },
+      retry: {
+        policiesRetry: jest.fn(),
+        permissionPoliciesRetry: jest.fn(),
+        conditionalPoliciesRetry: jest.fn(),
+      },
       error: new Error(''),
     });
     const { queryByText } = await renderInTestApp(
@@ -87,8 +94,11 @@ describe('PermissionsCard', () => {
     mockPermissionPolicies.mockReturnValue({
       loading: false,
       data: [],
-      conditionsData: [],
-      retry: { policiesRetry: jest.fn(), permissionPoliciesRetry: jest.fn() },
+      retry: {
+        policiesRetry: jest.fn(),
+        permissionPoliciesRetry: jest.fn(),
+        conditionalPoliciesRetry: jest.fn(),
+      },
       error: { message: '404', name: 'Not Found' },
     });
     const { queryByText } = await renderInTestApp(
@@ -110,9 +120,12 @@ describe('PermissionsCard', () => {
     mockPermissionPolicies.mockReturnValue({
       loading: false,
       data: [],
-      conditionsData: [],
       error: new Error(''),
-      retry: { policiesRetry: jest.fn(), permissionPoliciesRetry: jest.fn() },
+      retry: {
+        policiesRetry: jest.fn(),
+        permissionPoliciesRetry: jest.fn(),
+        conditionalPoliciesRetry: jest.fn(),
+      },
     });
     const { getByTestId } = await renderInTestApp(
       <PermissionsCard
@@ -128,9 +141,12 @@ describe('PermissionsCard', () => {
     mockPermissionPolicies.mockReturnValue({
       loading: false,
       data: [],
-      conditionsData: [],
       error: new Error(''),
-      retry: { policiesRetry: jest.fn(), permissionPoliciesRetry: jest.fn() },
+      retry: {
+        policiesRetry: jest.fn(),
+        permissionPoliciesRetry: jest.fn(),
+        conditionalPoliciesRetry: jest.fn(),
+      },
     });
     const { queryByTestId } = await renderInTestApp(
       <PermissionsCard
@@ -140,4 +156,31 @@ describe('PermissionsCard', () => {
     );
     expect(queryByTestId('disable-update-policies')).not.toBeNull();
   });
+
+  it('should show conditions rules count for Conditional permission policies when the data is loaded', async () => {
+    mockUsePermission.mockReturnValue({ loading: false, allowed: true });
+    mockPermissionPolicies.mockReturnValue({
+      loading: false,
+      data: [
+        ...usePermissionPoliciesMockData,
+        ...mockFormInitialValues.permissionPoliciesRows,
+      ],
+      retry: {
+        policiesRetry: jest.fn(),
+        permissionPoliciesRetry: jest.fn(),
+        conditionalPoliciesRetry: jest.fn(),
+      },
+      error: new Error(''),
+    });
+    const { queryByText } = await renderInTestApp(
+      <PermissionsCard
+        entityReference="user:default/debsmita1"
+        canReadUsersAndGroups
+      />,
+    );
+    expect(queryByText('Permission Policies (4)')).not.toBeNull();
+    expect(queryByText('Read, Create, Delete', { exact: true })).not.toBeNull();
+    expect(queryByText('Read', { exact: true })).not.toBeNull();
+    expect(queryByText('1 rule')).not.toBeNull();
+  });
 });
diff --git a/plugins/rbac/src/components/RoleOverview/PermissionsCard.tsx b/plugins/rbac/src/components/RoleOverview/PermissionsCard.tsx
@@ -60,6 +60,10 @@ export const PermissionsCard = ({
 
   let numberOfPolicies = 0;
   (permissions || data)?.forEach(p => {
+    if (p.conditions) {
+      numberOfPolicies++;
+      return;
+    }
     numberOfPolicies =
       numberOfPolicies +
       p.policies.filter(pol => pol.effect === 'allow').length;
@@ -72,6 +76,7 @@ export const PermissionsCard = ({
       onClick: () => {
         retry.permissionPoliciesRetry();
         retry.policiesRetry();
+        retry.conditionalPoliciesRetry();
       },
     },
     {
@@ -103,14 +108,14 @@ export const PermissionsCard = ({
         )}
         <Table
           title={
-            !loading && data?.length
+            !loading && data.length > 0
               ? `Permission Policies (${numberOfPolicies})`
               : 'Permission Policies'
           }
           actions={actions}
           renderSummaryRow={summary => onSearchResultsChange(summary.data)}
           options={{ padding: 'default', search: true, paging: true }}
-          data={data ?? []}
+          data={data}
           columns={columns}
           isLoading={loading}
           emptyContent={

diff --git a/plugins/rbac/src/components/RoleOverview/PermissionsListColumns.tsx b/plugins/rbac/src/components/RoleOverview/PermissionsListColumns.tsx
@@ -1,6 +1,7 @@
 import { TableColumn } from '@backstage/core-components';
 
 import { PermissionsData } from '../../types';
+import { getRulesNumber } from '../../utils/create-role-utils';
 
 export const columns: TableColumn<PermissionsData>[] = [
   {
@@ -30,4 +31,15 @@ export const columns: TableColumn<PermissionsData>[] = [
       return a.policies.length < b.policies.length ? -1 : 1;
     },
   },
+  {
+    title: 'Conditional',
+    field: 'conditions',
+    type: 'string',
+    render: (permissionsData: PermissionsData) => {
+      const totalRules = getRulesNumber(permissionsData.conditions);
+      return totalRules
+        ? `${totalRules} ${totalRules > 1 ? 'rules' : 'rule'}`
+        : '-';
+    },
+  },
 ];
diff --git a/plugins/rbac/src/hooks/usePermissionPolicies.test.ts b/plugins/rbac/src/hooks/usePermissionPolicies.test.ts
@@ -1,5 +1,6 @@
 import { renderHook, waitFor } from '@testing-library/react';
 
+import { mockConditions } from '../__fixtures__/mockConditions';
 import { mockPermissionPolicies } from '../__fixtures__/mockPermissionPolicies';
 import { mockAssociatedPolicies } from '../__fixtures__/mockPolicies';
 import { usePermissionPolicies } from './usePermissionPolicies';
@@ -11,27 +12,30 @@ jest.mock('@backstage/core-plugin-api', () => ({
     .mockReturnValueOnce({
       getAssociatedPolicies: jest.fn().mockReturnValue(mockAssociatedPolicies),
       listPermissions: jest.fn().mockReturnValue(mockPermissionPolicies),
+      getRoleConditions: jest.fn().mockReturnValue(mockConditions),
     })
     .mockReturnValueOnce({
       getAssociatedPolicies: jest.fn().mockReturnValue(mockAssociatedPolicies),
       listPermissions: jest.fn().mockReturnValue([]),
+      getRoleConditions: jest.fn().mockReturnValue([]),
     })
     .mockReturnValue({
       getAssociatedPolicies: jest
         .fn()
         .mockReturnValue({ status: '403', statusText: 'Unauthorized' }),
       listPermissions: jest.fn().mockReturnValue(mockPermissionPolicies),
+      getRoleConditions: jest.fn().mockReturnValue([]),
     }),
 }));
 
 describe('usePermissionPolicies', () => {
-  it('should return permission policies', async () => {
+  it('should return simple and conditional permission policies', async () => {
     const { result } = renderHook(() =>
       usePermissionPolicies('role:default/rbac_admin'),
     );
     await waitFor(() => {
       expect(result.current.loading).toBeFalsy();
-      expect(result.current.data).toHaveLength(3);
+      expect(result.current.data).toHaveLength(5);
     });
   });
 
@@ -45,7 +49,7 @@ describe('usePermissionPolicies', () => {
     });
   });
 
-  it('should return an error then the fetch api call returns an error', async () => {
+  it('should return an error when the fetch api call returns an error', async () => {
     const { result } = renderHook(() =>
       usePermissionPolicies('role:default/rbac_admin'),
     );

diff --git a/plugins/rbac/src/hooks/usePermissionPolicies.ts b/plugins/rbac/src/hooks/usePermissionPolicies.ts
@@ -1,5 +1,5 @@
 import React from 'react';
-import { useAsync, useAsyncRetry, useInterval } from 'react-use';
+import { useAsyncRetry, useInterval } from 'react-use';
 
 import { useApi } from '@backstage/core-plugin-api';
 
@@ -13,6 +13,7 @@ import {
 const getErrorText = (
   policies: any,
   permissionPolicies: any,
+  conditionalPolicies: any,
 ): { name: number; message: string } | undefined => {
   if (!Array.isArray(policies) && (policies as Response)?.statusText) {
     return {
@@ -29,6 +30,16 @@ const getErrorText = (
         (permissionPolicies as Response).statusText
       }`,
     };
+  } else if (
+    !Array.isArray(conditionalPolicies) &&
+    (conditionalPolicies as Response)?.statusText
+  ) {
+    return {
+      name: (conditionalPolicies as Response).status,
+      message: `Error fetching the conditional permission policies. ${
+        (conditionalPolicies as Response).statusText
+      }`,
+    };
   }
   return undefined;
 };
@@ -46,7 +57,11 @@ export const usePermissionPolicies = (
     return await rbacApi.getAssociatedPolicies(entityReference);
   });
 
-  const { value: conditionalPolicies } = useAsync(async () => {
+  const {
+    value: conditionalPolicies,
+    retry: conditionalPoliciesRetry,
+    error: conditionalPoliciesError,
+  } = useAsyncRetry(async () => {
     return await rbacApi.getRoleConditions(entityReference);
   });
 
@@ -61,8 +76,8 @@ export const usePermissionPolicies = (
   const loading =
     !permissionPoliciesError &&
     !policiesError &&
-    !policies &&
-    !permissionPolicies;
+    !conditionalPoliciesError &&
+    (!permissionPolicies || !policies || !conditionalPolicies);
 
   const allPermissionPolicies = React.useMemo(
     () => (Array.isArray(permissionPolicies) ? permissionPolicies : []),
@@ -90,17 +105,18 @@ export const usePermissionPolicies = (
     () => {
       policiesRetry();
       permissionPoliciesRetry();
+      conditionalPoliciesRetry();
     },
     loading ? null : pollInterval || null,
   );
   return {
     loading,
-    data,
-    conditionsData,
-    retry: { policiesRetry, permissionPoliciesRetry },
+    data: [...conditionsData, ...data],
+    retry: { policiesRetry, permissionPoliciesRetry, conditionalPoliciesRetry },
     error:
       policiesError ||
       permissionPoliciesError ||
-      getErrorText(policies, permissionPolicies),
+      conditionalPoliciesError ||
+      getErrorText(policies, permissionPolicies, conditionalPolicies),
   };
 };
diff --git a/plugins/rbac/src/types.ts b/plugins/rbac/src/types.ts
@@ -47,7 +47,7 @@ export type PermissionsData = {
   plugin: string;
   permission: string;
   policies: RowPolicy[];
-  policyString?: string[];
+  policyString?: string[] | string;
   isResourced?: boolean;
   conditions?: ConditionsData;
 };

diff --git a/plugins/rbac/src/utils/rbac-utils.test.ts b/plugins/rbac/src/utils/rbac-utils.test.ts
@@ -383,7 +383,7 @@ describe('getConditionalPermissionsData', () => {
           { policy: 'update', effect: 'deny' },
           { policy: 'delete', effect: 'deny' },
         ],
-        policyString: ['read'],
+        policyString: 'Read',
         conditions: {
           condition: {
             rule: 'HAS_ANNOTATION',