fix(rbac): fix the roles table to also watch policies (#1057)

debsmita1 · web-flow · commit ead78e2e96e2 · 2024-01-23T09:06:05.000Z
diff --git a/plugins/rbac/src/api/RBACBackendClient.ts b/plugins/rbac/src/api/RBACBackendClient.ts
@@ -16,7 +16,7 @@ import { getKindNamespaceName } from '../utils/rbac-utils';
 // @public
 export type RBACAPI = {
   getUserAuthorization: () => Promise<{ status: string }>;
-  getRoles: () => Promise<Role[]>;
+  getRoles: () => Promise<Role[] | Response>;
   getPolicies: () => Promise<RoleBasedPolicy[] | Response>;
   getAssociatedPolicies: (
     entityReference: string,
@@ -78,6 +78,11 @@ export class RBACBackendClient implements RBACAPI {
         ...(idToken && { Authorization: `Bearer ${idToken}` }),
       },
     });
+
+    if (jsonResponse.status !== 200 && jsonResponse.status !== 204) {
+      return jsonResponse;
+    }
+
     return jsonResponse.json();
   }
 
diff --git a/plugins/rbac/src/components/RbacPage.test.tsx b/plugins/rbac/src/components/RbacPage.test.tsx
@@ -37,7 +37,11 @@ describe('RbacPage', () => {
     mockUseRoles.mockReturnValue({
       loading: true,
       data: [],
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
diff --git a/plugins/rbac/src/components/RolesList/RolesList.test.tsx b/plugins/rbac/src/components/RolesList/RolesList.test.tsx
@@ -63,7 +63,11 @@ describe('RolesList', () => {
     mockUseRoles.mockReturnValue({
       loading: false,
       data: useRolesMockData,
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
@@ -80,12 +84,17 @@ describe('RolesList', () => {
     mockUseRoles.mockReturnValue({
       loading: false,
       data: [],
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
-    const { getByTestId } = await renderInTestApp(<RolesList />);
+    const { getByTestId, queryByText } = await renderInTestApp(<RolesList />);
     expect(getByTestId('roles-table-empty')).not.toBeNull();
+    expect(queryByText('Something went wrong')).not.toBeInTheDocument();
   });
 
   it('should show delete icon if user is authorized to delete roles', async () => {
@@ -96,7 +105,11 @@ describe('RolesList', () => {
     mockUseRoles.mockReturnValue({
       loading: false,
       data: useRolesMockData,
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
@@ -128,7 +141,11 @@ describe('RolesList', () => {
           },
         },
       ],
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
@@ -160,7 +177,11 @@ describe('RolesList', () => {
           },
         },
       ],
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: true,
       createRoleLoading: false,
     });
@@ -175,7 +196,11 @@ describe('RolesList', () => {
     mockUseRoles.mockReturnValue({
       loading: false,
       data: useRolesMockData,
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
@@ -192,7 +217,11 @@ describe('RolesList', () => {
     mockUseRoles.mockReturnValue({
       loading: false,
       data: useRolesMockData,
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: true,
       createRoleLoading: false,
     });
@@ -209,12 +238,35 @@ describe('RolesList', () => {
     mockUseRoles.mockReturnValue({
       loading: false,
       data: useRolesMockData,
-      retry: jest.fn(),
+      error: {
+        rolesError: '',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
       createRoleAllowed: false,
       createRoleLoading: false,
     });
     const { getByTestId } = await renderInTestApp(<RolesList />);
 
     expect(getByTestId('create-role-warning')).not.toBeNull();
   });
+
+  it('should show error message when there is an error fetching the roles', async () => {
+    RequirePermissionMock.mockImplementation(props => <>{props.children}</>);
+    mockUsePermission.mockReturnValue({ loading: false, allowed: true });
+    mockUseRoles.mockReturnValue({
+      loading: true,
+      data: [],
+      error: {
+        rolesError: 'Something went wrong',
+        policiesError: '',
+      },
+      retry: { roleRetry: jest.fn(), policiesRetry: jest.fn() },
+      createRoleAllowed: false,
+      createRoleLoading: false,
+    });
+
+    const { queryByText } = await renderInTestApp(<RolesList />);
+    expect(queryByText('Something went wrong')).toBeInTheDocument();
+  });
 });
diff --git a/plugins/rbac/src/components/RolesList/RolesList.tsx b/plugins/rbac/src/components/RolesList/RolesList.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 
-import { Table } from '@backstage/core-components';
+import { Table, WarningPanel } from '@backstage/core-components';
 
 import { makeStyles } from '@material-ui/core';
 
@@ -27,12 +27,13 @@ export const RolesList = () => {
 
   const [roles, setRoles] = React.useState<number | undefined>();
   const classes = useStyles();
-  const { loading, data, retry, createRoleAllowed, createRoleLoading } =
+  const { loading, data, retry, createRoleAllowed, createRoleLoading, error } =
     useRoles();
 
   const closeDialog = () => {
     setOpenDialog(false);
-    retry();
+    retry.roleRetry();
+    retry.policiesRetry();
   };
 
   const onAlertClose = () => {
@@ -49,6 +50,19 @@ export const RolesList = () => {
         createRoleAllowed={createRoleAllowed}
         createRoleLoading={createRoleLoading}
       />
+      {(error?.rolesError || error?.policiesError) && (
+        <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'
+            }
+            severity="error"
+          />
+        </div>
+      )}
       <Table
         title={
           !loading && data?.length
diff --git a/plugins/rbac/src/hooks/useRoles.ts b/plugins/rbac/src/hooks/useRoles.ts
@@ -24,19 +24,24 @@ export const useRoles = (
   data: RolesData[];
   createRoleLoading: boolean;
   createRoleAllowed: boolean;
-  retry: () => void;
+  error: {
+    rolesError: string;
+    policiesError: string;
+  };
+  retry: { roleRetry: () => void; policiesRetry: () => void };
 } => {
   const rbacApi = useApi(rbacApiRef);
   const {
-    loading: rolesLoading,
     value: roles,
     retry: roleRetry,
+    error: rolesError,
   } = useAsyncRetry(async () => await rbacApi.getRoles());
 
-  const { loading: policiesLoading, value: policies } = useAsyncRetry(
-    async () => await rbacApi.getPolicies(),
-    [],
-  );
+  const {
+    value: policies,
+    retry: policiesRetry,
+    error: policiesError,
+  } = useAsyncRetry(async () => await rbacApi.getPolicies(), []);
 
   const deletePermissionResult = usePermission({
     permission: policyEntityDeletePermission,
@@ -67,7 +72,7 @@ export const useRoles = (
   });
   const data: RolesData[] = React.useMemo(
     () =>
-      roles && roles?.length > 0
+      Array.isArray(roles) && roles?.length > 0
         ? roles.reduce((acc: RolesData[], role: Role) => {
             const permissions = getPermissions(
               role.name,
@@ -107,19 +112,31 @@ export const useRoles = (
       catalogEntityReadPermissionResult,
     ],
   );
-  const loading = rolesLoading && policiesLoading;
+  const loading = !rolesError && !policiesError && !roles && !policies;
+
   useInterval(
     () => {
       roleRetry();
+      policiesRetry();
     },
     loading ? null : pollInterval || 10000,
   );
 
   return {
     loading,
     data,
+    error: {
+      rolesError: (rolesError?.message ||
+        (typeof roles === 'object'
+          ? (roles as any as Response)?.statusText
+          : '')) as string,
+      policiesError: (policiesError?.message ||
+        (typeof policies === 'object'
+          ? (policies as any as Response)?.statusText
+          : '')) as string,
+    },
     createRoleLoading,
     createRoleAllowed,
-    retry: roleRetry,
+    retry: { roleRetry, policiesRetry },
   };
 };
