From 07200e42d62c51c2ff59e812521ad0c82cb62ea8 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Tue, 4 Jun 2024 02:55:20 -0400 Subject: [PATCH] fix(rbac): fix role list view permission policies column value (#1714) * fix(rbac): fix role list view permission policies column value Signed-off-by: Yi Cai * Update for review comments Signed-off-by: Yi Cai * Fixed lint issues Signed-off-by: Yi Cai * Addressed comments Signed-off-by: Yi Cai * Fixed prettier issue Signed-off-by: Yi Cai * Updated unit tests Signed-off-by: Yi Cai * Addressed review comments Signed-off-by: Yi Cai * Addressed review comments Signed-off-by: Yi Cai --------- Signed-off-by: Yi Cai --- plugins/rbac/src/components/RbacPage.test.tsx | 1 + .../components/RolesList/RolesList.test.tsx | 34 ++++++ .../src/components/RolesList/RolesList.tsx | 34 ++++-- plugins/rbac/src/hooks/useRoles.test.ts | 1 + plugins/rbac/src/hooks/useRoles.ts | 107 +++++++++++++----- 5 files changed, 144 insertions(+), 33 deletions(-) diff --git a/plugins/rbac/src/components/RbacPage.test.tsx b/plugins/rbac/src/components/RbacPage.test.tsx index 3c4bdedd11..eb79c903c5 100644 --- 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 index fcac893a1e..3c217ddc9b 100644 --- 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(); 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(); + 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 index f8fe9e13e1..203a1f94dc 100644 --- a/plugins/rbac/src/components/RolesList/RolesList.tsx +++ b/plugins/rbac/src/components/RolesList/RolesList.tsx @@ -44,6 +44,30 @@ 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 ( <> @@ -51,15 +75,11 @@ export const RolesList = () => { createRoleAllowed={createRoleAllowed} createRoleLoading={createRoleLoading} /> - {(error?.rolesError || error?.policiesError) && ( + {errorWarning.message && (
diff --git a/plugins/rbac/src/hooks/useRoles.test.ts b/plugins/rbac/src/hooks/useRoles.test.ts index bd76d1d94f..bce24d2969 100644 --- 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 index c860990445..c10ae69808 100644 --- 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(''); 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,