diff --git a/src/__data__/role-data.ts b/src/__data__/role-data.ts new file mode 100644 index 00000000..8ae0fde5 --- /dev/null +++ b/src/__data__/role-data.ts @@ -0,0 +1,27 @@ +export const defaultKonfluxRoleMap = { + 'konflux-admin-user-actions': 'admin', + 'konflux-contributor-user-actions': 'contributor', + 'konflux-maintainer-user-actions': 'maintainer', +}; + +export const mockConfigMap = { + kind: 'ConfigMap', + apiVersion: 'v1', + metadata: { + name: 'konflux-public-info', + namespace: 'konflux-info', + }, + data: { + 'info.json': + '{\n "environment": "staging",\n "integrations": {\n "github": {\n "application_url": "https://github.com/apps/konflux-staging"\n },\n "sbom_server": {\n "url": "https://atlas.stage.devshift.net/sbom/content/\u003cPLACEHOLDER\u003e"\n },\n "image_controller": {\n "enabled": true,\n "notifications": [\n {\n "title": "SBOM-event-to-Bombino",\n "event": "repo_push",\n "method": "webhook",\n "config": {\n "url": "https://bombino.preprod.api.redhat.com/v1/sbom/quay/push"\n }\n }\n ]\n }\n },\n "rbac": [\n {\n "displayName": "admin",\n "description": "Full access to Konflux resources including secrets",\n "roleRef": {\n "apiGroup": "rbac.authorization.k8s.io",\n "kind": "ClusterRole",\n "name": "konflux-admin-user-actions"\n }\n },\n {\n "displayName": "maintainer",\n "description": "Partial access to Konflux resources without access to secrets",\n "roleRef": {\n "apiGroup": "rbac.authorization.k8s.io",\n "kind": "ClusterRole",\n "name": "konflux-maintainer-user-actions"\n }\n },\n {\n "displayName": "contributor",\n "description": "View access to Konflux resources without access to secrets",\n "roleRef": {\n "apiGroup": "rbac.authorization.k8s.io",\n "kind": "ClusterRole",\n "name": "konflux-contributor-user-actions"\n }\n }\n ]\n}\n', + }, +}; + +export const invalidMockConfigMap = { + kind: 'ConfigMap', + apiVersion: 'v1', + metadata: { + name: 'konflux-public-info', + namespace: 'konflux-info', + }, +}; diff --git a/src/__data__/rolebinding-data.ts b/src/__data__/rolebinding-data.ts new file mode 100644 index 00000000..3cd20473 --- /dev/null +++ b/src/__data__/rolebinding-data.ts @@ -0,0 +1,77 @@ +import { RoleBinding } from '../types'; + +export const mockRoleBinding: RoleBinding = { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'RoleBinding', + metadata: { name: 'metadata-name', namespace: 'test-ns' }, + subjects: [ + { apiGroup: 'rbac.authorization.k8s.io', name: 'user1', kind: 'User', namespace: 'test-ns' }, + ], + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'konflux-contributor-user-actions', + }, +}; + +export const mockRoleBindings: RoleBinding[] = [ + { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'RoleBinding', + metadata: { name: 'konflux-contributor-user1-actions-user', namespace: 'test-ns' }, + subjects: [ + { apiGroup: 'rbac.authorization.k8s.io', name: 'user1', kind: 'User', namespace: 'test-ns' }, + ], + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'konflux-contributor-user-actions', + }, + }, + { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'RoleBinding', + metadata: { name: 'konflux-maintainer-user2-actions-user', namespace: 'test-ns' }, + subjects: [ + { apiGroup: 'rbac.authorization.k8s.io', name: 'user2', kind: 'User', namespace: 'test-ns' }, + ], + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'konflux-maintainer-user-actions', + }, + }, + { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'RoleBinding', + metadata: { name: 'konflux-maintainer-user3-actions-user', namespace: 'test-ns' }, + subjects: [ + { apiGroup: 'rbac.authorization.k8s.io', name: 'user3', kind: 'User', namespace: 'test-ns' }, + ], + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'konflux-maintainer-user-actions', + }, + }, + { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'RoleBinding', + metadata: { name: 'konflux-maintainer-user4-actions-user', namespace: 'test-ns' }, + subjects: [ + { apiGroup: 'rbac.authorization.k8s.io', name: 'user4', kind: 'User', namespace: 'test-ns' }, + ], + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'konflux-admin-user-actions', + }, + }, +]; + +export const mockRoleBindingWithoutUser: RoleBinding = { + ...mockRoleBinding, + subjects: mockRoleBinding.subjects.map((subject) => + subject.kind === 'User' ? { ...subject, kind: 'Group' } : subject, + ), +}; diff --git a/src/components/UserAccess/RBListHeader.tsx b/src/components/UserAccess/RBListHeader.tsx new file mode 100644 index 00000000..920dfb59 --- /dev/null +++ b/src/components/UserAccess/RBListHeader.tsx @@ -0,0 +1,23 @@ +export const rbTableColumnClasses = { + username: 'pf-m-width-25', + role: 'pf-m-width-20', + status: 'pf-m-hidden pf-m-visible-on-md pf-m-width-20', + kebab: 'pf-v5-c-table__action', +}; + +export const RBListHeader = () => [ + { + title: 'Username', + props: { className: rbTableColumnClasses.username }, + }, + { + title: 'Role', + props: { className: rbTableColumnClasses.role }, + }, + { + title: ' ', + props: { + className: rbTableColumnClasses.kebab, + }, + }, +]; diff --git a/src/components/UserAccess/RBListRow.tsx b/src/components/UserAccess/RBListRow.tsx new file mode 100644 index 00000000..c557a7cb --- /dev/null +++ b/src/components/UserAccess/RBListRow.tsx @@ -0,0 +1,33 @@ +import * as React from 'react'; +import { Bullseye, Spinner } from '@patternfly/react-core'; +import { useRoleMap } from '../../hooks/useRole'; +import { RowFunctionArgs, TableData } from '../../shared'; +import ActionMenu from '../../shared/components/action-menu/ActionMenu'; +import { RoleBinding } from '../../types'; +import { rbTableColumnClasses } from './RBListHeader'; +import { useRBActions } from './user-access-actions'; + +export const RBListRow: React.FC>> = ({ + obj, +}) => { + const actions = useRBActions(obj); + const [roleMap, roleMapLoading] = useRoleMap(); + + if (roleMapLoading) { + return ( + + + + ); + } + + return ( + <> + {obj.subjects[0].name} + {roleMap[obj.roleRef.name]} + + + + + ); +}; diff --git a/src/components/UserAccess/RevokeAccessModal.tsx b/src/components/UserAccess/RevokeAccessModal.tsx index 59350dde..6c9d1392 100644 --- a/src/components/UserAccess/RevokeAccessModal.tsx +++ b/src/components/UserAccess/RevokeAccessModal.tsx @@ -13,18 +13,18 @@ import { ModalVariant, } from '@patternfly/react-core'; import { K8sQueryDeleteResource } from '../../k8s'; -import { SpaceBindingRequestModel } from '../../models'; -import { WorkspaceBinding } from '../../types'; +import { RoleBindingModel } from '../../models'; +import { RoleBinding } from '../../types'; import { RawComponentProps } from '../modal/createModalLauncher'; import { invalidateWorkspaceQuery } from '../Workspace/utils'; type Props = RawComponentProps & { - sbr: WorkspaceBinding['bindingRequest']; + rb: RoleBinding; username: string; }; export const RevokeAccessModal: React.FC> = ({ - sbr, + rb, username, onClose, modalProps, @@ -38,10 +38,10 @@ export const RevokeAccessModal: React.FC> = ({ setError(null); try { await K8sQueryDeleteResource({ - model: SpaceBindingRequestModel, + model: RoleBindingModel, queryOptions: { - name: sbr.name, - ns: sbr.namespace, + name: rb.metadata.name, + ns: rb.metadata.namespace, }, }); await invalidateWorkspaceQuery(); @@ -50,7 +50,7 @@ export const RevokeAccessModal: React.FC> = ({ setError((err as { message: string }).message || (err.toString() as string)); } }, - [onClose, sbr], + [onClose, rb], ); return ( diff --git a/src/components/UserAccess/SBRListHeader.tsx b/src/components/UserAccess/SBRListHeader.tsx deleted file mode 100644 index 7eed705f..00000000 --- a/src/components/UserAccess/SBRListHeader.tsx +++ /dev/null @@ -1,27 +0,0 @@ -export const sbrTableColumnClasses = { - username: 'pf-m-width-25', - role: 'pf-m-width-20', - status: 'pf-m-hidden pf-m-visible-on-md pf-m-width-20', - kebab: 'pf-v5-c-table__action', -}; - -export const SBRListHeader = () => [ - { - title: 'Username', - props: { className: sbrTableColumnClasses.username }, - }, - { - title: 'Role', - props: { className: sbrTableColumnClasses.role }, - }, - { - title: 'Status', - props: { className: sbrTableColumnClasses.status }, - }, - { - title: ' ', - props: { - className: sbrTableColumnClasses.kebab, - }, - }, -]; diff --git a/src/components/UserAccess/SBRListRow.tsx b/src/components/UserAccess/SBRListRow.tsx deleted file mode 100644 index 8ce52f3c..00000000 --- a/src/components/UserAccess/SBRListRow.tsx +++ /dev/null @@ -1,26 +0,0 @@ -import * as React from 'react'; -import { RowFunctionArgs, TableData } from '../../shared'; -import ActionMenu from '../../shared/components/action-menu/ActionMenu'; -import { WorkspaceBinding } from '../../types'; -import { sbrTableColumnClasses } from './SBRListHeader'; -import { SBRStatusLabel } from './SBRStatusLabel'; -import { useSBRActions } from './user-access-actions'; - -export const SBRListRow: React.FC>> = ({ - obj, -}) => { - const actions = useSBRActions(obj); - - return ( - <> - {obj.masterUserRecord} - {obj.role} - - {obj.bindingRequest ? : '-'} - - - - - - ); -}; diff --git a/src/components/UserAccess/SBRStatusLabel.tsx b/src/components/UserAccess/SBRStatusLabel.tsx deleted file mode 100644 index d551ee13..00000000 --- a/src/components/UserAccess/SBRStatusLabel.tsx +++ /dev/null @@ -1,27 +0,0 @@ -import * as React from 'react'; -import { Label, Tooltip } from '@patternfly/react-core'; -import { useSpaceBindingRequest } from '../../hooks/useSpaceBindingRequests'; -import { WorkspaceBinding } from '../../types'; -import { useWorkspaceInfo } from '../Workspace/useWorkspaceInfo'; - -export const SBRStatusLabel: React.FC< - React.PropsWithChildren<{ sbr: WorkspaceBinding['bindingRequest'] }> -> = ({ sbr }) => { - const { workspace } = useWorkspaceInfo(); - const [binding, loaded] = useSpaceBindingRequest(sbr.namespace, workspace, sbr.name); - const status = binding?.status?.conditions?.[0]; - - if (!loaded || !status) { - return <>-; - } - - if (status.reason === 'Provisioned') { - return ; - } - - return ( - - - - ); -}; diff --git a/src/components/UserAccess/UserAccessForm/EditAccessPage.tsx b/src/components/UserAccess/UserAccessForm/EditAccessPage.tsx index f7d00c9b..eb2c364a 100644 --- a/src/components/UserAccess/UserAccessForm/EditAccessPage.tsx +++ b/src/components/UserAccess/UserAccessForm/EditAccessPage.tsx @@ -3,9 +3,9 @@ import { useParams } from 'react-router-dom'; import { Bullseye, Spinner } from '@patternfly/react-core'; import { FULL_APPLICATION_TITLE } from '../../../consts/labels'; import { useDocumentTitle } from '../../../hooks/useDocumentTitle'; -import { useSpaceBindingRequest } from '../../../hooks/useSpaceBindingRequests'; +import { useRoleBindings } from '../../../hooks/useRoleBindings'; import { HttpError } from '../../../k8s/error'; -import { SpaceBindingRequestModel } from '../../../models'; +import { RoleBindingModel } from '../../../models'; import { RouterParams } from '../../../routes/utils'; import ErrorEmptyState from '../../../shared/components/empty-state/ErrorEmptyState'; import { AccessReviewResources } from '../../../types'; @@ -15,35 +15,30 @@ import { UserAccessFormPage } from './UserAccessFormPage'; const EditAccessPage: React.FC> = () => { const { bindingName } = useParams(); - const { workspace, workspaceResource } = useWorkspaceInfo(); - const binding = workspaceResource.status?.bindings?.find( - (b) => b.masterUserRecord === bindingName, - )?.bindingRequest; - - const accessReviewResources: AccessReviewResources = binding - ? [{ model: SpaceBindingRequestModel, verb: 'update' }] - : [{ model: SpaceBindingRequestModel, verb: 'create' }]; + const { namespace } = useWorkspaceInfo(); + const [roleBindings, loading, error] = useRoleBindings(namespace); + const binding = roleBindings.find((roleBinding) => + roleBinding.subjects.some((subject) => subject.name === bindingName), + ); - useDocumentTitle(`Edit access to workspace, ${workspace} | ${FULL_APPLICATION_TITLE}`); + useDocumentTitle(`Edit access to workspace, ${namespace} | ${FULL_APPLICATION_TITLE}`); - const [existingSBR, loaded, loadErr] = useSpaceBindingRequest( - binding.namespace, - workspace, - binding.name, - ); + const accessReviewResources: AccessReviewResources = binding + ? [{ model: RoleBindingModel, verb: 'update' }] + : [{ model: RoleBindingModel, verb: 'create' }]; - if (binding && loadErr) { - const httpError = HttpError.fromCode((loadErr as { code: number }).code); + if (error) { + const httpError = HttpError.fromCode((error as { code: number }).code); return ( ); } - if (binding && !loaded) { + if (loading) { return ( @@ -53,9 +48,8 @@ const EditAccessPage: React.FC> = () => { return ( - + ); }; - export default EditAccessPage; diff --git a/src/components/UserAccess/UserAccessForm/PermissionsTable.tsx b/src/components/UserAccess/UserAccessForm/PermissionsTable.tsx index 87ab7b54..8f781b8b 100644 --- a/src/components/UserAccess/UserAccessForm/PermissionsTable.tsx +++ b/src/components/UserAccess/UserAccessForm/PermissionsTable.tsx @@ -1,6 +1,6 @@ import React from 'react'; import { Table, Tbody, Td, Th, Thead, Tr } from '@patternfly/react-table'; -import { WorkspaceRole } from '../../../types'; +import { NamespaceRole } from '../../../types'; import './PermissionsTable.scss'; @@ -13,7 +13,7 @@ enum Permission { } // roles and permissions ADR: https://github.com/redhat-appstudio/book/blob/main/ADR/0011-roles-and-permissions.md -const permissions: Record> = { +const permissions: Record> = { contributor: { Application: [Permission.Read], Component: [Permission.Read], @@ -75,7 +75,7 @@ const permissions: Record> = { }, }; -export const PermissionsTable = React.memo<{ role: WorkspaceRole }>(({ role }) => { +export const PermissionsTable = React.memo<{ role: NamespaceRole }>(({ role }) => { const rolePermissions = permissions[role]; return ( diff --git a/src/components/UserAccess/UserAccessForm/RoleSection.tsx b/src/components/UserAccess/UserAccessForm/RoleSection.tsx index ac91f6f6..34180600 100644 --- a/src/components/UserAccess/UserAccessForm/RoleSection.tsx +++ b/src/components/UserAccess/UserAccessForm/RoleSection.tsx @@ -1,21 +1,30 @@ import React from 'react'; -import { ExpandableSection, FormSection } from '@patternfly/react-core'; +import { Bullseye, ExpandableSection, FormSection, Spinner } from '@patternfly/react-core'; import { useField } from 'formik'; import HelpPopover from '../../../components/HelpPopover'; +import { useRoleMap } from '../../../hooks/useRole'; import DropdownField from '../../../shared/components/formik-fields/DropdownField'; -import { WorkspaceRole } from '../../../types'; +import { NamespaceRole } from '../../../types'; import { PermissionsTable } from './PermissionsTable'; - import './RoleSection.scss'; -const dropdownItems = [ - { key: 'contributor', value: 'contributor' }, - { key: 'maintainer', value: 'maintainer' }, - { key: 'admin', value: 'admin' }, -]; - export const RoleSection: React.FC> = () => { - const [{ value: role }] = useField('role'); + const [{ value: role }] = useField('role'); + const [roleMap, roleMapLoading] = useRoleMap(); + + if (roleMapLoading) { + return ( + + + + ); + } + const dropdownItems = [ + ...Object.entries(roleMap).map(([key, value]) => ({ + key, + value, + })), + ]; return ( <> diff --git a/src/components/UserAccess/UserAccessForm/UserAccessFormPage.tsx b/src/components/UserAccess/UserAccessForm/UserAccessFormPage.tsx index 60f5a7bd..5ef07c50 100644 --- a/src/components/UserAccess/UserAccessForm/UserAccessFormPage.tsx +++ b/src/components/UserAccess/UserAccessForm/UserAccessFormPage.tsx @@ -1,25 +1,27 @@ import React from 'react'; import { useNavigate } from 'react-router-dom'; +import { Bullseye, Spinner } from '@patternfly/react-core'; import { Formik, FormikHelpers } from 'formik'; -import { SpaceBindingRequest } from '../../../types'; +import { useRoleMap } from '../../../hooks/useRole'; +import { NamespaceRole, RoleBinding } from '../../../types'; import { TrackEvents, useTrackEvent } from '../../../utils/analytics'; import { useWorkspaceInfo } from '../../Workspace/useWorkspaceInfo'; import { invalidateWorkspaceQuery } from '../../Workspace/utils'; -import { createSBRs, editSBR, userAccessFormSchema, UserAccessFormValues } from './form-utils'; +import { createRBs, editRB, userAccessFormSchema, UserAccessFormValues } from './form-utils'; import { UserAccessForm } from './UserAccessForm'; - type Props = { - existingSbr?: SpaceBindingRequest; + existingRb?: RoleBinding; username?: string; edit?: boolean; }; export const UserAccessFormPage: React.FC> = ({ - existingSbr, + existingRb, edit, username, }) => { const { workspace, namespace } = useWorkspaceInfo(); + const [roleMap, roleMapLoading] = useRoleMap(); const track = useTrackEvent(); const navigate = useNavigate(); @@ -39,16 +41,14 @@ export const UserAccessFormPage: React.FC> = ({ workspace, }); try { - await (existingSbr - ? editSBR(values, existingSbr, true) - : createSBRs(values, namespace, true)); - await (existingSbr ? editSBR(values, existingSbr) : createSBRs(values, namespace)); + await (existingRb ? editRB(values, existingRb, true) : createRBs(values, namespace, true)); + await (existingRb ? editRB(values, existingRb) : createRBs(values, namespace)); track(edit ? 'Access Edited' : 'Access Created', { usernames: values.usernames, workspace, }); await invalidateWorkspaceQuery(); - navigate(`/workspace/${workspace}/access`); + navigate(`/workspaces/${workspace}/access`); } catch (error) { // eslint-disable-next-line no-console console.warn('Error while submitting access form:', error); @@ -69,12 +69,21 @@ export const UserAccessFormPage: React.FC> = ({ }), workspace, }); - navigate(`/workspace/${workspace}/access`); + navigate(`/workspaces/${workspace}/access`); }; + if (roleMapLoading) { + return ( + + + + ); + } + const initialValues: UserAccessFormValues = { - usernames: existingSbr ? [existingSbr.spec.masterUserRecord] : edit ? [username] : [], - role: existingSbr?.spec?.spaceRole, + usernames: existingRb ? existingRb.subjects.map((sub) => sub.name) : edit ? [username] : [], + role: roleMap[existingRb?.roleRef.name] as NamespaceRole, + roleMap, }; return ( diff --git a/src/components/UserAccess/UserAccessForm/UsernameSection.tsx b/src/components/UserAccess/UserAccessForm/UsernameSection.tsx index 7a192dac..cf30b494 100644 --- a/src/components/UserAccess/UserAccessForm/UsernameSection.tsx +++ b/src/components/UserAccess/UserAccessForm/UsernameSection.tsx @@ -32,11 +32,6 @@ export const UsernameSection: React.FC> = ({ disa setValidating(true); setError(''); - if (!username) { - setValidating(false); - return; - } - try { await konfluxUsernameYupValidation.validate(username); setValidating(false); diff --git a/src/components/UserAccess/UserAccessForm/__tests__/EditAccessPage.spec.tsx b/src/components/UserAccess/UserAccessForm/__tests__/EditAccessPage.spec.tsx new file mode 100644 index 00000000..008a2e2f --- /dev/null +++ b/src/components/UserAccess/UserAccessForm/__tests__/EditAccessPage.spec.tsx @@ -0,0 +1,81 @@ +import { useParams } from 'react-router-dom'; +import { render, screen, waitFor } from '@testing-library/react'; +import { mockRoleBinding } from '../../../../__data__/rolebinding-data'; +import { useRoleBindings } from '../../../../hooks/useRoleBindings'; +import { useWorkspaceInfo } from '../../../Workspace/useWorkspaceInfo'; +import EditAccessPage from '../EditAccessPage'; + +jest.mock('react-router-dom', () => ({ + useParams: jest.fn(), + useNavigate: jest.fn().mockReturnValue(jest.fn()), +})); + +jest.mock('../../../Workspace/useWorkspaceInfo', () => ({ + useWorkspaceInfo: jest.fn(), +})); + +jest.mock('../../../../hooks/useRoleBindings', () => ({ + useRoleBindings: jest.fn(), +})); + +jest.mock('../../../../utils/rbac', () => ({ + useAccessReviewForModels: jest.fn(), +})); + +jest.mock('../../../PageAccess/PageAccessCheck', () => ({ + __esModule: true, + default: jest.fn(({ children }) => <>{children}), +})); + +jest.mock('../UserAccessFormPage', () => ({ + UserAccessFormPage: jest.fn(() =>
Mocked User Access Form Page
), +})); + +describe('EditAccessPage', () => { + const mockUseParams = useParams as jest.Mock; + const mockUseWorkspaceInfo = useWorkspaceInfo as jest.Mock; + const mockUseRoleBindings = useRoleBindings as jest.Mock; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should render spinner while data is loading', () => { + mockUseParams.mockReturnValue({ bindingName: 'user1' }); + mockUseWorkspaceInfo.mockReturnValue({ namespace: 'test-namespace' }); + mockUseRoleBindings.mockReturnValue([[], true, null]); // Simulate loading state + + render(); + + expect(screen.getByRole('progressbar')).toBeInTheDocument(); + }); + + it('should render error state when error occurs', async () => { + mockUseParams.mockReturnValue({ bindingName: 'user1' }); + mockUseWorkspaceInfo.mockReturnValue({ namespace: 'test-namespace' }); + mockUseRoleBindings.mockReturnValue([[], false, { code: 500 }]); // Simulate error + + render(); + + // Wait for async error display + await waitFor(() => { + expect(screen.getByText('Unable to load role binding user1')).toBeInTheDocument(); + // Adjust error message as per your HttpError handling + expect(screen.getByText('Internal Server Error')).toBeInTheDocument(); + }); + }); + + it('should render the form when data is loaded', async () => { + mockUseParams.mockReturnValue({ bindingName: 'user1' }); + mockUseWorkspaceInfo.mockReturnValue({ namespace: 'test-ws' }); + // Simulate loaded state with mock data + mockUseRoleBindings.mockReturnValue([[mockRoleBinding], false, null]); + + render(); + + // Ensure that UserAccessFormPage is rendered when data is loaded + await waitFor(() => + expect(screen.getByText('Mocked User Access Form Page')).toBeInTheDocument(), + ); + }); +}); diff --git a/src/components/UserAccess/UserAccessForm/__tests__/RoleSection.spec.tsx b/src/components/UserAccess/UserAccessForm/__tests__/RoleSection.spec.tsx index a648c151..124e1639 100644 --- a/src/components/UserAccess/UserAccessForm/__tests__/RoleSection.spec.tsx +++ b/src/components/UserAccess/UserAccessForm/__tests__/RoleSection.spec.tsx @@ -1,8 +1,19 @@ -import { screen, fireEvent } from '@testing-library/react'; +import { screen, fireEvent, act, waitFor } from '@testing-library/react'; +import { defaultKonfluxRoleMap } from '../../../../__data__/role-data'; +import { useRoleMap } from '../../../../hooks/useRole'; import { formikRenderer } from '../../../../utils/test-utils'; import { RoleSection } from '../RoleSection'; +jest.mock('../../../../hooks/useRole', () => ({ + useRoleMap: jest.fn(), +})); + describe('RoleSection', () => { + beforeEach(() => { + const mockUseRoleMap = useRoleMap as jest.Mock; + mockUseRoleMap.mockReturnValue([defaultKonfluxRoleMap, false, null]); + }); + it('should not render permissions if role is not selected', () => { formikRenderer(, { role: '' }); expect(screen.getByText('Select role')).toBeVisible(); @@ -12,17 +23,31 @@ describe('RoleSection', () => { expect(screen.queryByText('Show list of permissions')).toBeNull(); }); - it('should render permissions if role is selected', () => { + it('should render permissions if role is selected', async () => { formikRenderer(, { role: '' }); - fireEvent.click(screen.getByText('Select role')); - expect(screen.getByText('contributor')).toBeVisible(); - expect(screen.getByText('maintainer')).toBeVisible(); - expect(screen.getByText('admin')).toBeVisible(); - fireEvent.click(screen.getByText('maintainer')); - expect(screen.getByText('Show list of permissions for the maintainer')).toBeVisible(); + act(() => { + fireEvent.click(screen.getByText('Select role')); + }); + await waitFor(() => { + expect(screen.getByText('contributor')).toBeVisible(); + expect(screen.getByText('maintainer')).toBeVisible(); + expect(screen.getByText('admin')).toBeVisible(); + }); + act(() => { + fireEvent.click(screen.getByText('maintainer')); + }); + await waitFor(() => { + fireEvent.click(screen.getByText('maintainer')); + expect(screen.getByText('Show list of permissions for the maintainer')).toBeVisible(); + }); - fireEvent.click(screen.getByText('maintainer')); - fireEvent.click(screen.getByText('admin')); - expect(screen.getByText('Show list of permissions for the admin')).toBeVisible(); + act(() => { + fireEvent.click(screen.getByText('contributor')); + fireEvent.click(screen.getByText('admin')); + }); + await waitFor(() => { + fireEvent.click(screen.getByText('admin')); + expect(screen.getByText('Show list of permissions for the admin')).toBeVisible(); + }); }); }); diff --git a/src/components/UserAccess/UserAccessForm/__tests__/UserAccessForm.spec.tsx b/src/components/UserAccess/UserAccessForm/__tests__/UserAccessForm.spec.tsx index b5498195..28fa5a27 100644 --- a/src/components/UserAccess/UserAccessForm/__tests__/UserAccessForm.spec.tsx +++ b/src/components/UserAccess/UserAccessForm/__tests__/UserAccessForm.spec.tsx @@ -1,9 +1,15 @@ import { screen } from '@testing-library/react'; import { FormikProps } from 'formik'; +import { defaultKonfluxRoleMap } from '../../../../__data__/role-data'; +import { useRoleMap } from '../../../../hooks/useRole'; import { createUseWorkspaceInfoMock, formikRenderer } from '../../../../utils/test-utils'; import { UserAccessFormValues } from '../form-utils'; import { UserAccessForm } from '../UserAccessForm'; +jest.mock('../../../../hooks/useRole', () => ({ + useRoleMap: jest.fn(), +})); + jest.mock('../../../../utils/breadcrumb-utils', () => ({ useWorkspaceBreadcrumbs: jest.fn(() => []), })); @@ -21,6 +27,10 @@ jest.mock('react-router-dom', () => { }); describe('UserAccessForm', () => { + beforeEach(() => { + const mockUseRoleMap = useRoleMap as jest.Mock; + mockUseRoleMap.mockReturnValue([defaultKonfluxRoleMap, false, null]); + }); createUseWorkspaceInfoMock({ workspace: 'test-ws' }); it('should show create form', () => { diff --git a/src/components/UserAccess/UserAccessForm/__tests__/UserAccessFormPage.spec.tsx b/src/components/UserAccess/UserAccessForm/__tests__/UserAccessFormPage.spec.tsx index b7d12775..9f9e4e82 100644 --- a/src/components/UserAccess/UserAccessForm/__tests__/UserAccessFormPage.spec.tsx +++ b/src/components/UserAccess/UserAccessForm/__tests__/UserAccessFormPage.spec.tsx @@ -1,8 +1,11 @@ import { act, fireEvent, screen, waitFor } from '@testing-library/react'; import identity from 'lodash-es/identity'; -import { SpaceBindingRequest, Workspace } from '../../../../types'; -import { namespaceRenderer } from '../../../../utils/test-utils'; -import { createSBRs, editSBR } from '../form-utils'; +import { defaultKonfluxRoleMap } from '../../../../__data__/role-data'; +import { mockRoleBinding } from '../../../../__data__/rolebinding-data'; +import { useRoleMap } from '../../../../hooks/useRole'; +import { Workspace } from '../../../../types'; +import { createK8sWatchResourceMock, namespaceRenderer } from '../../../../utils/test-utils'; +import { createRBs, editRB } from '../form-utils'; import { UserAccessFormPage } from '../UserAccessFormPage'; jest.mock('react-i18next', () => ({ @@ -29,23 +32,29 @@ jest.mock('react-router-dom', () => ({ jest.mock('../form-utils', () => ({ ...jest.requireActual('../form-utils'), - createSBRs: jest.fn(), - editSBR: jest.fn(), + createRBs: jest.fn(), + editRB: jest.fn(), })); -const createSBRsMock = createSBRs as jest.Mock; -const editSBRsMock = editSBR as jest.Mock; +jest.mock('../../../../hooks/useRole', () => ({ + useRoleMap: jest.fn(), +})); + +const watchMock = createK8sWatchResourceMock(); +const createRBsMock = createRBs as jest.Mock; +const editRBMock = editRB as jest.Mock; describe('UserAccessFormPage', () => { - // beforeEach(jest.useFakeTimers); - afterEach(jest.clearAllMocks); + const mockUseRoleMap = useRoleMap as jest.Mock; + beforeEach(() => { + mockUseRoleMap.mockReturnValue([defaultKonfluxRoleMap, false, null]); + watchMock.mockReturnValue([[], true]); + }); - // afterEach(() => { - // jest.useRealTimers(); - // }); + afterEach(jest.clearAllMocks); it('should create resources on submit', async () => { - createSBRsMock.mockResolvedValue({}); + createRBsMock.mockResolvedValue({}); namespaceRenderer(, 'test-ns', { workspace: 'test-ws', workspaceResource: {} as Workspace, @@ -57,14 +66,14 @@ describe('UserAccessFormPage', () => { await act(() => fireEvent.click(screen.getByText('maintainer'))); await waitFor(() => expect(screen.getByRole('button', { name: 'Grant access' })).toBeEnabled()); await act(() => fireEvent.click(screen.getByRole('button', { name: 'Grant access' }))); - expect(createSBRsMock).toHaveBeenCalledTimes(2); - expect(createSBRsMock).toHaveBeenCalledWith( - { role: 'maintainer', usernames: ['user1'] }, + expect(createRBsMock).toHaveBeenCalledTimes(2); + expect(createRBsMock).toHaveBeenCalledWith( + { role: 'maintainer', usernames: ['user1'], roleMap: defaultKonfluxRoleMap }, 'test-ns', ); }); - it('should create resources for edit when existing sbr is not available', async () => { + it('should create resources for edit when existing rb is not available', async () => { namespaceRenderer(, 'test-ns', { workspace: 'test-ws', workspaceResource: {} as Workspace, @@ -75,43 +84,25 @@ describe('UserAccessFormPage', () => { await act(() => fireEvent.click(screen.getByText('maintainer'))); await waitFor(() => expect(screen.getByRole('button', { name: 'Save changes' })).toBeEnabled()); await act(() => fireEvent.click(screen.getByRole('button', { name: 'Save changes' }))); - expect(createSBRsMock).toHaveBeenCalledTimes(2); - expect(createSBRsMock).toHaveBeenCalledWith( - { role: 'maintainer', usernames: ['myuser'] }, + expect(createRBsMock).toHaveBeenCalledTimes(2); + expect(createRBsMock).toHaveBeenCalledWith( + { role: 'maintainer', usernames: ['myuser'], roleMap: defaultKonfluxRoleMap }, 'test-ns', ); }); - it('should update resources when existing sbr is provided', async () => { - editSBRsMock.mockResolvedValue({}); - const mockSBR: SpaceBindingRequest = { - apiVersion: 'appstudio.redhat.com/v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { - name: 'test-sbr', - }, - spec: { - masterUserRecord: 'user1', - spaceRole: 'contributor', - }, - }; - namespaceRenderer( - , - 'test-ns', - { - workspace: 'test-ws', - workspaceResource: {} as Workspace, - }, - ); + it('should update resources when existing rb is provided', async () => { + namespaceRenderer(, 'test-ns'); + expect(screen.getByText('Edit access to workspace, test-ws')).toBeVisible(); expect(screen.getByRole('searchbox')).toBeDisabled(); await act(() => fireEvent.click(screen.getByText('contributor'))); await act(() => fireEvent.click(screen.getByText('maintainer'))); await waitFor(() => expect(screen.getByRole('button', { name: 'Save changes' })).toBeEnabled()); await act(() => fireEvent.click(screen.getByRole('button', { name: 'Save changes' }))); - expect(editSBRsMock).toHaveBeenCalledTimes(2); - expect(editSBRsMock).toHaveBeenCalledWith( - { role: 'maintainer', usernames: ['user1'] }, - mockSBR, + expect(editRBMock).toHaveBeenCalledTimes(2); + expect(editRBMock).toHaveBeenCalledWith( + { role: 'maintainer', usernames: ['user1'], roleMap: defaultKonfluxRoleMap }, + mockRoleBinding, ); }); }); diff --git a/src/components/UserAccess/UserAccessForm/__tests__/form-utils.spec.ts b/src/components/UserAccess/UserAccessForm/__tests__/form-utils.spec.ts index a780776d..b0c6dce5 100644 --- a/src/components/UserAccess/UserAccessForm/__tests__/form-utils.spec.ts +++ b/src/components/UserAccess/UserAccessForm/__tests__/form-utils.spec.ts @@ -1,54 +1,138 @@ import '@testing-library/jest-dom'; +import { defaultKonfluxRoleMap } from '../../../../__data__/role-data'; +import { mockRoleBinding } from '../../../../__data__/rolebinding-data'; +import { K8sQueryDeleteResource } from '../../../../k8s'; +import { NamespaceRole } from '../../../../types'; import { createK8sUtilMock } from '../../../../utils/test-utils'; -import { createSBRs, editSBR } from '../form-utils'; +import { createRBs, editRB, deleteRB } from '../form-utils'; const k8sCreateMock = createK8sUtilMock('K8sQueryCreateResource'); -const k8sPatchMock = createK8sUtilMock('K8sQueryPatchResource'); +const k8sGetMock = createK8sUtilMock('K8sQueryListResourceItems'); +const k8sDeleteMock = createK8sUtilMock('K8sQueryDeleteResource'); -describe('createSBRs', () => { - it('should create SBRs for all usernames', async () => { +afterEach(() => { + jest.clearAllMocks(); +}); + +describe('createRBs', () => { + it('should create RBs for all usernames', async () => { k8sCreateMock.mockImplementation((obj) => obj.resource); - const result = await createSBRs( - { usernames: ['user1', 'user2', 'user3'], role: 'maintainer' }, + const result = await createRBs( + { + usernames: ['user1', 'user2', 'user3'], + role: 'maintainer', + roleMap: defaultKonfluxRoleMap, + }, 'test-ns', ); - expect(k8sCreateMock).toHaveBeenCalledTimes(3); + // We give up expect(result).toEqual and expect.objectContaining() here. + // Because althrough we use expect.objectContaining(), removing some items + // of roleRef still make test fail. + const expectedResult = ['user1', 'user2', 'user3'].map((username) => ({ + ...mockRoleBinding, + metadata: { + ...mockRoleBinding.metadata, + name: `konflux-maintainer-${username}-actions-user`, + }, + subjects: mockRoleBinding.subjects.map((subject) => ({ + ...subject, + name: username, + })), + roleRef: { ...mockRoleBinding.roleRef, name: 'konflux-maintainer-user-actions' }, + })); + + expect(result).toEqual(expectedResult); + }); +}); + +describe('editRB', () => { + it('should replace the old role with new role', async () => { + k8sCreateMock.mockImplementation((obj) => obj.resource); + + const values = { + usernames: ['user1'], + role: 'admin' as NamespaceRole, + roleMap: defaultKonfluxRoleMap, + }; + + k8sGetMock.mockResolvedValue([mockRoleBinding]); + k8sDeleteMock.mockImplementation(); + const result = await editRB(values, mockRoleBinding); + + expect(k8sDeleteMock).toHaveBeenCalledTimes(1); + expect(k8sCreateMock).toHaveBeenCalledTimes(1); expect(result).toEqual([ expect.objectContaining({ - metadata: { generateName: 'user1-', namespace: 'test-ns' }, - spec: { masterUserRecord: 'user1', spaceRole: 'maintainer' }, - }), - expect.objectContaining({ - metadata: { generateName: 'user2-', namespace: 'test-ns' }, - spec: { masterUserRecord: 'user2', spaceRole: 'maintainer' }, - }), - expect.objectContaining({ - metadata: { generateName: 'user3-', namespace: 'test-ns' }, - spec: { masterUserRecord: 'user3', spaceRole: 'maintainer' }, + ...mockRoleBinding, + metadata: { + ...mockRoleBinding.metadata, + name: `konflux-admin-user1-actions-user`, + }, + subjects: mockRoleBinding.subjects.map((subject) => ({ + ...subject, + name: 'user1', + })), + roleRef: { ...mockRoleBinding.roleRef, name: 'konflux-admin-user-actions' }, }), ]); }); + + it('should handle errors from k8sCreateMock gracefully', async () => { + const values = { + usernames: ['user1'], + role: 'admin' as NamespaceRole, + roleMap: defaultKonfluxRoleMap, + }; + k8sGetMock.mockResolvedValue([mockRoleBinding]); + k8sCreateMock.mockRejectedValueOnce(new Error('Create failed')); + + await expect(editRB(values, mockRoleBinding)).rejects.toThrow('Create failed'); + + expect(k8sCreateMock).toHaveBeenCalledTimes(1); + // If users cannot be created successfully, we do not delete existing roles. + // In this way, it looks like users does not edit roles successfuly. + expect(k8sDeleteMock).toHaveBeenCalledTimes(0); + }); }); -describe('editSBR', () => { - it('should patch SBR with new role', async () => { - k8sPatchMock.mockResolvedValue({}); - const result = await editSBR( - { usernames: ['user1'], role: 'contributor' }, - { - apiVersion: 'v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { name: 'user1' }, - spec: { masterUserRecord: 'user1', spaceRole: 'maintainer' }, +describe('deleteRB', () => { + it('should call K8sQueryDeleteResource with correct parameters when dryRun is not passed', async () => { + k8sDeleteMock.mockResolvedValueOnce(undefined); + + await deleteRB(mockRoleBinding); + + expect(K8sQueryDeleteResource).toHaveBeenCalledTimes(1); + expect(K8sQueryDeleteResource).toHaveBeenCalledWith({ + model: expect.anything(), + queryOptions: { + name: 'metadata-name', + ns: 'test-ns', }, - ); - expect(result).toEqual([{}]); - expect(k8sPatchMock).toHaveBeenCalledTimes(1); - expect(k8sPatchMock).toHaveBeenCalledWith( - expect.objectContaining({ - patches: [{ op: 'replace', path: '/spec/spaceRole', value: 'contributor' }], - }), - ); + }); + }); + + it('should call K8sQueryDeleteResource with dryRun query parameter when dryRun is true', async () => { + k8sDeleteMock.mockResolvedValueOnce(undefined); + + await deleteRB(mockRoleBinding, true); + + expect(K8sQueryDeleteResource).toHaveBeenCalledTimes(1); + expect(K8sQueryDeleteResource).toHaveBeenCalledWith({ + model: expect.anything(), + queryOptions: { + name: 'metadata-name', + ns: 'test-ns', + queryParams: { + dryRun: 'All', + }, + }, + }); + }); + + it('should throw an error if K8sQueryDeleteResource fails', async () => { + k8sDeleteMock.mockRejectedValueOnce(new Error('Failed to delete')); + + await expect(deleteRB(mockRoleBinding)).rejects.toThrow('Failed to delete'); }); }); diff --git a/src/components/UserAccess/UserAccessForm/form-utils.ts b/src/components/UserAccess/UserAccessForm/form-utils.ts index 26fe4aa1..7f0b455e 100644 --- a/src/components/UserAccess/UserAccessForm/form-utils.ts +++ b/src/components/UserAccess/UserAccessForm/form-utils.ts @@ -1,11 +1,16 @@ import * as yup from 'yup'; -import { K8sQueryCreateResource, K8sQueryPatchResource } from '../../../k8s'; -import { SpaceBindingRequestGroupVersionKind, SpaceBindingRequestModel } from '../../../models'; -import { SpaceBindingRequest, WorkspaceRole } from '../../../types'; +import { + K8sQueryCreateResource, + K8sQueryDeleteResource, + K8sQueryListResourceItems, +} from '../../../k8s'; +import { RoleBindingGroupVersionKind, RoleBindingModel } from '../../../models'; +import { RoleBinding, NamespaceRole } from '../../../types'; export type UserAccessFormValues = { usernames: string[]; - role: WorkspaceRole; + role: NamespaceRole; + roleMap: object; }; export const userAccessFormSchema = yup.object({ @@ -20,29 +25,52 @@ export const userAccessFormSchema = yup.object({ .required('Required.'), }); -export const createSBRs = async ( +export const getRBs = (namespace: string): Promise => { + return K8sQueryListResourceItems({ + model: RoleBindingModel, + queryOptions: { + ns: namespace, + }, + }); +}; + +/** + * Create role-bindings to shared konflux cluster roles + */ +export const createRBs = async ( values: UserAccessFormValues, namespace: string, dryRun?: boolean, -): Promise => { - const { usernames, role } = values; - const objs: SpaceBindingRequest[] = usernames.map((username) => ({ - apiVersion: `${SpaceBindingRequestGroupVersionKind.group}/${SpaceBindingRequestGroupVersionKind.version}`, - kind: SpaceBindingRequestGroupVersionKind.kind, +): Promise => { + const { usernames, role, roleMap } = values; + const konfluxRoles = Object.keys(roleMap); + const roleRefName = konfluxRoles.find((konfluxRole) => konfluxRole.includes(role)); + const objs: RoleBinding[] = usernames.map((username) => ({ + apiVersion: `${RoleBindingGroupVersionKind.group}/${RoleBindingGroupVersionKind.version}`, + kind: RoleBindingGroupVersionKind.kind, metadata: { - generateName: `${username}-`, + name: `konflux-${role}-${username}-actions-user`, namespace, }, - spec: { - masterUserRecord: username, - spaceRole: role, + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + name: roleRefName, + kind: 'ClusterRole', }, + subjects: [ + { + kind: 'User', + apiGroup: 'rbac.authorization.k8s.io', + name: username, + namespace, + }, + ], })); return Promise.all( objs.map((obj) => K8sQueryCreateResource({ - model: SpaceBindingRequestModel, + model: RoleBindingModel, queryOptions: { ns: namespace, ...(dryRun && { queryParams: { dryRun: 'All' } }), @@ -53,32 +81,49 @@ export const createSBRs = async ( ); }; +export const deleteRB = async (roleBinding: RoleBinding, dryRun?: boolean): Promise => { + const queryOptions = { + model: RoleBindingModel, + queryOptions: { + name: roleBinding.metadata.name, + ns: roleBinding.metadata.namespace, + ...(dryRun && { queryParams: { dryRun: 'All' } }), + }, + }; + + await K8sQueryDeleteResource(queryOptions); +}; + /** - * Only updates one SBR, but returning array to - * keep it consistent with `createSBRs()` + * The metata.name of rolebing is name: `konflux-${role}-${username}-actions-user`. + * We can not patch it. + * So we need to create one with the new role then delete the + * obsoleted one. */ -export const editSBR = async ( +export const editRB = async ( values: UserAccessFormValues, - sbr: SpaceBindingRequest, + roleBinding: RoleBinding, dryRun?: boolean, -): Promise => { - const { role } = values; +): Promise => { + const usernames = roleBinding.subjects.map((subject) => subject.name); + const { role, roleMap } = values; - return Promise.all([ - K8sQueryPatchResource({ - model: SpaceBindingRequestModel, - queryOptions: { - name: sbr.metadata.name, - ns: sbr.metadata.namespace, - ...(dryRun && { queryParams: { dryRun: 'All' } }), - }, - patches: [ - { - op: 'replace', - path: '/spec/spaceRole', - value: role, - }, - ], - }), - ]) as Promise; + // Create new role bindings before we delete roles + const newRoleBinding = await createRBs( + { usernames, role, roleMap }, + roleBinding.metadata.namespace, + dryRun, + ); + + // Only delete if it exists and new role has been created + const existingRBs = await getRBs(roleBinding.metadata.namespace); + + const roleBindingExists = existingRBs.some( + (binding) => binding.metadata.name === roleBinding.metadata.name, + ); + + if (roleBindingExists && newRoleBinding) { + await deleteRB(roleBinding, dryRun); + } + return newRoleBinding; }; diff --git a/src/components/UserAccess/UserAccessListPage.tsx b/src/components/UserAccess/UserAccessListPage.tsx index 72642533..fce3d392 100644 --- a/src/components/UserAccess/UserAccessListPage.tsx +++ b/src/components/UserAccess/UserAccessListPage.tsx @@ -2,7 +2,7 @@ import React from 'react'; import { Divider, PageSection, PageSectionVariants } from '@patternfly/react-core'; import { FULL_APPLICATION_TITLE } from '../../consts/labels'; import { useDocumentTitle } from '../../hooks/useDocumentTitle'; -import { SpaceBindingRequestModel } from '../../models'; +import { RoleBindingModel } from '../../models'; import { useWorkspaceBreadcrumbs } from '../../utils/breadcrumb-utils'; import { useAccessReviewForModel } from '../../utils/rbac'; import PageLayout from '../PageLayout/PageLayout'; @@ -13,7 +13,7 @@ const UserAccessPage: React.FunctionComponent = () => { const breadcrumbs = useWorkspaceBreadcrumbs(); const { workspace } = useWorkspaceInfo(); - const [canCreateSBR] = useAccessReviewForModel(SpaceBindingRequestModel, 'create'); + const [canCreateRB] = useAccessReviewForModel(RoleBindingModel, 'create'); useDocumentTitle(`User access | ${FULL_APPLICATION_TITLE}`); @@ -32,7 +32,7 @@ const UserAccessPage: React.FunctionComponent = () => { { id: 'grant-access', label: 'Grant access', - disabled: !canCreateSBR, + disabled: !canCreateRB, disabledTooltip: 'You cannot grant access in this workspace', cta: { href: `/workspaces/${workspace}/access/grant`, diff --git a/src/components/UserAccess/UserAccessListView.tsx b/src/components/UserAccess/UserAccessListView.tsx index 4772f623..af86e02b 100644 --- a/src/components/UserAccess/UserAccessListView.tsx +++ b/src/components/UserAccess/UserAccessListView.tsx @@ -17,23 +17,24 @@ import { } from '@patternfly/react-core'; import { FilterIcon } from '@patternfly/react-icons/dist/esm/icons'; import emptyStateImgUrl from '../../assets/Integration-test.svg'; +import { useRoleBindings } from '../../hooks/useRoleBindings'; import { useSearchParam } from '../../hooks/useSearchParam'; -import { SpaceBindingRequestModel } from '../../models'; +import { RoleBindingModel } from '../../models'; import { Table } from '../../shared'; import AppEmptyState from '../../shared/components/empty-state/AppEmptyState'; import FilteredEmptyState from '../../shared/components/empty-state/FilteredEmptyState'; -import { WorkspaceBinding } from '../../types'; +import { RoleBinding } from '../../types'; import { useAccessReviewForModel } from '../../utils/rbac'; import { ButtonWithAccessTooltip } from '../ButtonWithAccessTooltip'; import { useWorkspaceInfo } from '../Workspace/useWorkspaceInfo'; -import { SBRListHeader } from './SBRListHeader'; -import { SBRListRow } from './SBRListRow'; +import { RBListHeader } from './RBListHeader'; +import { RBListRow } from './RBListRow'; const UserAccessEmptyState: React.FC< React.PropsWithChildren<{ - canCreateSBR: boolean; + canCreateRB: boolean; }> -> = ({ canCreateSBR }) => { +> = ({ canCreateRB }) => { const { workspace } = useWorkspaceInfo(); return ( @@ -49,7 +50,7 @@ const UserAccessEmptyState: React.FC< } - isDisabled={!canCreateSBR} + isDisabled={!canCreateRB} tooltip="You cannot grant access in this workspace" analytics={{ link_name: 'grant-access', @@ -64,28 +65,30 @@ const UserAccessEmptyState: React.FC< }; export const UserAccessListView: React.FC> = () => { - const { workspace, workspaceResource } = useWorkspaceInfo(); - const [canCreateSBR] = useAccessReviewForModel(SpaceBindingRequestModel, 'create'); + const { workspace, namespace } = useWorkspaceInfo(); + const [canCreateRB] = useAccessReviewForModel(RoleBindingModel, 'create'); const [usernameFilter, setUsernameFilter, clearFilters] = useSearchParam('name', ''); + const [roleBindings, loading] = useRoleBindings(namespace, true); - const filteredSBRs = React.useMemo( + const filterRBs = React.useMemo( () => - workspaceResource?.status?.bindings?.filter((binding) => - binding.masterUserRecord.includes(usernameFilter.toLowerCase()), + roleBindings.filter((rb) => + rb.subjects.some((subject) => + subject.name.toLowerCase().includes(usernameFilter.toLowerCase()), + ), ), - [usernameFilter, workspaceResource], + [roleBindings, usernameFilter], ); - - if (!workspaceResource) { + if (loading) { return ( ); - } - if (!workspaceResource.status?.bindings?.length) { - return ; + if (!filterRBs) { + return ; + } } return ( @@ -119,7 +122,7 @@ export const UserAccessListView: React.FC> = () component={(props) => ( )} - isDisabled={!canCreateSBR} + isDisabled={!canCreateRB} tooltip="You cannot grant access in this workspace" analytics={{ link_name: 'grant-access', @@ -133,15 +136,15 @@ export const UserAccessListView: React.FC> = () - {filteredSBRs.length ? ( + {filterRBs.length ? ( ({ - id: obj.masterUserRecord, + getRowProps={(obj: RoleBinding) => ({ + id: obj.metadata.name, })} /> ) : ( diff --git a/src/components/UserAccess/__tests__/RBListRow.spec.tsx b/src/components/UserAccess/__tests__/RBListRow.spec.tsx new file mode 100644 index 00000000..3cfe38c9 --- /dev/null +++ b/src/components/UserAccess/__tests__/RBListRow.spec.tsx @@ -0,0 +1,42 @@ +import { render } from '@testing-library/react'; +import { defaultKonfluxRoleMap } from '../../../__data__/role-data'; +import { mockRoleBinding } from '../../../__data__/rolebinding-data'; +import { useRoleMap } from '../../../hooks/useRole'; +import { createK8sWatchResourceMock } from '../../../utils/test-utils'; +import { RBListRow } from '../RBListRow'; + +jest.mock('react-router-dom', () => ({ + Link: (props) => {props.children}, +})); + +jest.mock('../../../hooks/useRole', () => ({ + useRoleMap: jest.fn(), +})); + +const watchMock = createK8sWatchResourceMock(); + +describe('RBListRow', () => { + it('should render correct user info', () => { + const mockUseRoleMap = useRoleMap as jest.Mock; + watchMock.mockReturnValueOnce([mockRoleBinding, false]); + mockUseRoleMap.mockReturnValueOnce([defaultKonfluxRoleMap, false, null]); + const wrapper = render(, { + container: document.createElement('tr'), + }); + const cells = wrapper.container.getElementsByTagName('td'); + wrapper.debug(); + + expect(cells[0]).toHaveTextContent('user1'); + expect(cells[1]).toHaveTextContent('contributor'); + }); + + it('should render spinner', () => { + const mockUseRoleMap = useRoleMap as jest.Mock; + watchMock.mockReturnValueOnce([mockRoleBinding, false]); + mockUseRoleMap.mockReturnValueOnce([defaultKonfluxRoleMap, true, null]); + const wrapper = render(, { + container: document.createElement('div'), + }); + wrapper.container.getElementsByTagName('spinner'); + }); +}); diff --git a/src/components/UserAccess/__tests__/RevokeAccessModal.spec.tsx b/src/components/UserAccess/__tests__/RevokeAccessModal.spec.tsx index 3c7e2bad..88c31cd9 100644 --- a/src/components/UserAccess/__tests__/RevokeAccessModal.spec.tsx +++ b/src/components/UserAccess/__tests__/RevokeAccessModal.spec.tsx @@ -1,4 +1,5 @@ import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { mockRoleBinding } from '../../../__data__/rolebinding-data'; import { createK8sUtilMock } from '../../../utils/test-utils'; import { RevokeAccessModal } from '../RevokeAccessModal'; @@ -7,14 +8,10 @@ const k8sDeleteMock = createK8sUtilMock('K8sQueryDeleteResource'); describe('RevokeAccessModal', () => { it('should render revoke modal', () => { render( - , + , ); expect(screen.getByTestId('description').textContent).toBe( - 'The user test-user will lose access to this workspace and all of its applications, environments, and any other dependent items.', + 'The user user1 will lose access to this workspace and all of its applications, environments, and any other dependent items.', ); expect(screen.getByRole('button', { name: 'Revoke' })).toBeVisible(); expect(screen.getByRole('button', { name: 'Cancel' })).toBeVisible(); @@ -25,8 +22,8 @@ describe('RevokeAccessModal', () => { k8sDeleteMock.mockResolvedValue({}); render( , @@ -41,8 +38,8 @@ describe('RevokeAccessModal', () => { k8sDeleteMock.mockRejectedValue(new Error('Unable to delete')); render( , diff --git a/src/components/UserAccess/__tests__/SBRListRow.spec.tsx b/src/components/UserAccess/__tests__/SBRListRow.spec.tsx deleted file mode 100644 index f566404b..00000000 --- a/src/components/UserAccess/__tests__/SBRListRow.spec.tsx +++ /dev/null @@ -1,55 +0,0 @@ -import { render } from '@testing-library/react'; -import { SpaceBindingRequest } from '../../../types'; -import { createK8sWatchResourceMock } from '../../../utils/test-utils'; -import { SBRListRow } from '../SBRListRow'; - -jest.mock('react-router-dom', () => ({ - Link: (props) => {props.children}, -})); - -const watchMock = createK8sWatchResourceMock(); - -const mockSBR: SpaceBindingRequest = { - apiVersion: 'appstudio.redhat.com/v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { - name: 'test-sbr', - }, - spec: { - masterUserRecord: 'user', - spaceRole: 'contributor', - }, - status: { - conditions: [ - { - reason: 'Provisioned', - status: 'True', - }, - ], - }, -}; - -describe('SBRListRow', () => { - it('should render correct user info', () => { - watchMock.mockReturnValueOnce([mockSBR, true]); - const wrapper = render( - , - { - container: document.createElement('tr'), - }, - ); - const cells = wrapper.container.getElementsByTagName('td'); - wrapper.debug(); - - expect(cells[0]).toHaveTextContent('user'); - expect(cells[1]).toHaveTextContent('contributor'); - expect(cells[2]).toHaveTextContent('Provisioned'); - }); -}); diff --git a/src/components/UserAccess/__tests__/SBRStatusLabel.spec.tsx b/src/components/UserAccess/__tests__/SBRStatusLabel.spec.tsx deleted file mode 100644 index c0071129..00000000 --- a/src/components/UserAccess/__tests__/SBRStatusLabel.spec.tsx +++ /dev/null @@ -1,86 +0,0 @@ -import { screen, render } from '@testing-library/react'; -import { SpaceBindingRequest } from '../../../types'; -import { createK8sWatchResourceMock } from '../../../utils/test-utils'; -import { SBRStatusLabel } from '../SBRStatusLabel'; - -const watchMock = createK8sWatchResourceMock(); - -describe('SBRStatusLabel', () => { - it('should not render label if status is not available', () => { - const sbr: SpaceBindingRequest = { - apiVersion: 'appstudio.redhat.com/v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { - name: 'test-sbr', - }, - spec: { - masterUserRecord: 'user', - spaceRole: 'contributor', - }, - }; - - watchMock.mockReturnValueOnce([sbr, true]); - render(); - expect(screen.getByText('-')).toBeVisible(); - }); - - it('should not render label if sbr is not loaded', () => { - watchMock.mockReturnValueOnce([null, false]); - render(); - expect(screen.getByText('-')).toBeVisible(); - }); - - it('should render green label for provisioned sbr', () => { - const sbr: SpaceBindingRequest = { - apiVersion: 'appstudio.redhat.com/v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { - name: 'test-sbr', - }, - spec: { - masterUserRecord: 'user', - spaceRole: 'contributor', - }, - status: { - conditions: [ - { - reason: 'Provisioned', - status: 'True', - }, - ], - }, - }; - - watchMock.mockReturnValueOnce([sbr, true]); - render(); - expect(screen.getByText('Provisioned')).toBeVisible(); - expect(screen.getByText('Provisioned').parentElement.parentElement).toHaveClass('pf-m-green'); - }); - - it('should render gold label for non-provisioned sbr', () => { - const sbr: SpaceBindingRequest = { - apiVersion: 'appstudio.redhat.com/v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { - name: 'test-sbr', - }, - spec: { - masterUserRecord: 'user', - spaceRole: 'contributor', - }, - status: { - conditions: [ - { - reason: 'UnknownError', - status: 'False', - }, - ], - }, - }; - - watchMock.mockReturnValueOnce([sbr, true]); - render(); - expect(screen.getByText('UnknownError')).toBeVisible(); - expect(screen.getByText('UnknownError').parentElement.parentElement).toHaveClass('pf-m-gold'); - }); -}); diff --git a/src/components/UserAccess/__tests__/UserAccessListView.spec.tsx b/src/components/UserAccess/__tests__/UserAccessListView.spec.tsx index a6e6c3aa..96d68e95 100644 --- a/src/components/UserAccess/__tests__/UserAccessListView.spec.tsx +++ b/src/components/UserAccess/__tests__/UserAccessListView.spec.tsx @@ -1,16 +1,32 @@ -import * as React from 'react'; import { Table as PfTable, TableHeader } from '@patternfly/react-table/deprecated'; -import { act, fireEvent, screen } from '@testing-library/react'; -import { SpaceBindingRequest } from '../../../types'; -import { createK8sWatchResourceMock, namespaceRenderer } from '../../../utils/test-utils'; -import { SBRListRow } from '../SBRListRow'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { defaultKonfluxRoleMap } from '../../../__data__/role-data'; +import { mockRoleBinding, mockRoleBindings } from '../../../__data__/rolebinding-data'; +import { useRoleMap } from '../../../hooks/useRole'; +import { useRoleBindings } from '../../../hooks/useRoleBindings'; +import { useSearchParam } from '../../../hooks/useSearchParam'; +import { useAccessReviewForModel } from '../../../utils/rbac'; +import { useWorkspaceInfo } from '../../Workspace/useWorkspaceInfo'; +import { RBListRow } from '../RBListRow'; import { UserAccessListView } from '../UserAccessListView'; jest.mock('react-router-dom', () => ({ Link: (props) => {props.children}, - useSearchParams: () => React.useState(() => new URLSearchParams()), + useParams: jest.fn(), + useNavigate: jest.fn().mockReturnValue(jest.fn()), })); +jest.mock('../../Workspace/useWorkspaceInfo', () => ({ + useWorkspaceInfo: jest.fn(), +})); + +jest.mock('../../../utils/rbac'); +jest.mock('../../../hooks/useRoleBindings'); +jest.mock('../../../hooks/useRole'); +jest.mock('../../../hooks/useSearchParam'); +jest.mock('../../../hooks/useSearchParam', () => ({ + useSearchParam: jest.fn(), +})); jest.mock('../../../shared/components/table', () => { const actual = jest.requireActual('../../../shared/components/table'); return { @@ -25,7 +41,7 @@ jest.mock('../../../shared/components/table', () => { {props.data.map((d, i) => ( - + ))} @@ -35,203 +51,86 @@ jest.mock('../../../shared/components/table', () => { }; }); -jest.mock('../../../utils/rbac', () => ({ - useAccessReviewForModel: jest.fn(() => [true, true]), -})); - -const watchMock = createK8sWatchResourceMock(); - -const mockSBR: SpaceBindingRequest = { - apiVersion: 'appstudio.redhat.com/v1alpha1', - kind: 'SpaceBindingRequest', - metadata: { - name: 'test-sbr', - }, - spec: { - masterUserRecord: 'user1', - spaceRole: 'contributor', - }, - status: { - conditions: [ - { - reason: 'Provisioned', - status: 'True', - }, - ], - }, -}; +jest.mock('@patternfly/react-core', () => { + const originalModule = jest.requireActual('@patternfly/react-core'); + return { + ...originalModule, + Toolbar: ({ children }) =>
{children}
, + }; +}); describe('UserAccessListView', () => { - it('should render empty state if no bindings are present', () => { - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: { - apiVersion: 'v1alpha1', - apiGroup: 'toolchain.dev.openshift.com', - kind: 'Workspace', - status: { - namespaces: [], - role: 'admin', - owner: 'my-user', - }, - }, - }); - expect(screen.getByText('Grant user access')).toBeVisible(); - const addButton = screen.queryByText('Grant access'); - expect(addButton).toBeInTheDocument(); - expect(addButton.closest('a').href).toContain( - `http://localhost/workspaces/test-ws/access/grant`, - ); + let setUsernameFilterMock: jest.Mock; + + const mockWorkspaceInfo = { + workspace: 'test-ws', + namespace: 'test-ns', + }; + + beforeEach(() => { + setUsernameFilterMock = jest.fn(); + (useSearchParam as jest.Mock).mockReturnValue(['', setUsernameFilterMock, jest.fn()]); + (useWorkspaceInfo as jest.Mock).mockReturnValue(mockWorkspaceInfo); + (useAccessReviewForModel as jest.Mock).mockReturnValue([true]); + (useRoleBindings as jest.Mock).mockReturnValue([mockRoleBindings, false]); + (useRoleMap as jest.Mock).mockReturnValue([defaultKonfluxRoleMap, false]); + (useSearchParam as jest.Mock).mockReturnValue(['', jest.fn(), jest.fn()]); }); - it('should render filter present in the view', () => { - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: { - apiVersion: 'v1alpha1', - apiGroup: 'toolchain.dev.openshift.com', - kind: 'Workspace', - status: { - namespaces: [], - role: 'admin', - owner: 'my-user', - bindings: [{ masterUserRecord: 'my-user', role: 'admin' }], - }, - }, - }); - expect(screen.getByPlaceholderText('Search by username...')).toBeVisible(); + it('should display loading state while fetching role bindings', () => { + (useRoleBindings as jest.Mock).mockReturnValue([[], true]); + + render(); + + expect(screen.getByRole('progressbar')).toBeInTheDocument(); }); - it('should render row of user data', () => { - watchMock.mockReturnValue([mockSBR, true]); - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: { - apiVersion: 'v1alpha1', - apiGroup: 'toolchain.dev.openshift.com', - kind: 'Workspace', - status: { - namespaces: [], - role: 'admin', - owner: 'my-user', - bindings: [ - { - masterUserRecord: 'user1', - role: 'contributor', - bindingRequest: { name: 'test-sbr', namespace: 'test-ns' }, - }, - ], - }, - }, - }); - expect(screen.getByText('user1')).toBeVisible(); - expect(screen.getByText('contributor')).toBeVisible(); - expect(screen.getByText('Provisioned')).toBeVisible(); + it('should display role bindings when loaded', () => { + (useRoleBindings as jest.Mock).mockReturnValue([[mockRoleBinding], false]); + render(); + expect(screen.getByText('user1')).toBeInTheDocument(); }); - it('should match the user if filtered by name', () => { - watchMock.mockReturnValue([mockSBR, true]); - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: { - apiVersion: 'v1alpha1', - apiGroup: 'toolchain.dev.openshift.com', - kind: 'Workspace', - status: { - namespaces: [], - role: 'admin', - owner: 'my-user', - bindings: [ - { - masterUserRecord: 'user1', - role: 'contributor', - bindingRequest: { name: 'test-sbr', namespace: 'test-ns' }, - }, - ], - }, - }, + it('should display empty state if no role bindings match the filter', async () => { + (useSearchParam as jest.Mock).mockReturnValue(['user111', jest.fn(), jest.fn()]); + render(); + + await waitFor(() => { + expect(screen.getByText(/No results match this filter/)).toBeInTheDocument(); }); + }); + it('should filter role bindings by username', () => { + render(); const filter = screen.getByPlaceholderText('Search by username...'); - act(() => { - fireEvent.input(filter, { - target: { value: 'user1' }, - }); - }); - expect(screen.queryByText('user1')).toBeInTheDocument(); + fireEvent.input(filter, { target: { value: 'user1' } }); + + expect(screen.getByText('user1')).toBeInTheDocument(); }); - it('should perform case insensitive filter by name', () => { - watchMock.mockReturnValue([mockSBR, true]); - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: { - apiVersion: 'v1alpha1', - apiGroup: 'toolchain.dev.openshift.com', - kind: 'Workspace', - status: { - namespaces: [], - role: 'admin', - owner: 'my-user', - bindings: [ - { - masterUserRecord: 'user1', - role: 'contributor', - bindingRequest: { name: 'test-sbr', namespace: 'test-ns' }, - }, - ], - }, - }, - }); + it('should show the empty state when there are no role bindings', () => { + (useRoleBindings as jest.Mock).mockReturnValue([[], false]); - const filter = screen.getByPlaceholderText('Search by username...'); - act(() => { - fireEvent.input(filter, { - target: { value: 'USER1' }, - }); - }); - expect(screen.queryByText('user1')).toBeInTheDocument(); + render(); + + expect(screen.getByText('Grant access')).toBeInTheDocument(); }); - it('should not match the user if filtered by unmatched name', () => { - watchMock.mockReturnValue([mockSBR, true]); - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: { - apiVersion: 'v1alpha1', - apiGroup: 'toolchain.dev.openshift.com', - kind: 'Workspace', - status: { - namespaces: [], - role: 'admin', - owner: 'my-user', - bindings: [ - { - masterUserRecord: 'user1', - role: 'contributor', - bindingRequest: { name: 'test-sbr', namespace: 'test-ns' }, - }, - ], - }, - }, - }); + it('should allow creating a role binding if the user has permission', () => { + (useAccessReviewForModel as jest.Mock).mockReturnValue([true]); - const filter = screen.getByPlaceholderText('Search by username...'); - act(() => { - fireEvent.change(filter, { - target: { value: 'invalid-user-2' }, - }); - }); - expect(screen.queryByText('user1')).not.toBeInTheDocument(); - expect(screen.queryByText('No results found')).toBeInTheDocument(); - expect(screen.queryByText('Clear all filters')).toBeInTheDocument(); + render(); + + const grantAccessButton = screen.getByText('Grant access'); + expect(grantAccessButton).toBeEnabled(); }); - it('should render spinner while data is not loaded', () => { - namespaceRenderer(, 'my-ns', { - workspace: 'test-ws', - workspaceResource: null, - }); - expect(screen.getByRole('progressbar')).toBeVisible(); + it('should not allow creating a role binding if the user does not have permission', () => { + (useAccessReviewForModel as jest.Mock).mockReturnValue([false]); + + render(); + + const grantAccessButton = screen.getByText('Grant access'); + expect(grantAccessButton.getAttribute('aria-disabled')).toEqual('true'); }); }); diff --git a/src/components/UserAccess/__tests__/user-access-actions.spec.ts b/src/components/UserAccess/__tests__/user-access-actions.spec.ts index aa56ce11..8b2dad0d 100644 --- a/src/components/UserAccess/__tests__/user-access-actions.spec.ts +++ b/src/components/UserAccess/__tests__/user-access-actions.spec.ts @@ -1,100 +1,111 @@ -import '@testing-library/jest-dom'; -import { renderHook } from '@testing-library/react-hooks'; +import { mockRoleBinding, mockRoleBindingWithoutUser } from '../../../__data__/rolebinding-data'; import { useAccessReviewForModel } from '../../../utils/rbac'; -import { createUseWorkspaceInfoMock } from '../../../utils/test-utils'; -import { useSBRActions } from '../user-access-actions'; +import { useModalLauncher } from '../../modal/ModalProvider'; +import { useWorkspaceInfo } from '../../Workspace/useWorkspaceInfo'; +import { useRBActions } from '../user-access-actions'; +// Mock the dependencies jest.mock('../../../utils/rbac', () => ({ useAccessReviewForModel: jest.fn(), })); -const accessReviewMock = useAccessReviewForModel as jest.Mock; +jest.mock('../../Workspace/useWorkspaceInfo', () => ({ + useWorkspaceInfo: jest.fn(), +})); + +jest.mock('../../modal/ModalProvider', () => ({ + useModalLauncher: jest.fn(), +})); -describe('useSBRActions', () => { - createUseWorkspaceInfoMock({ workspace: 'test-ws' }); +describe('useRBActions', () => { + const mockWorkspace = 'test-workspace'; beforeEach(() => { - accessReviewMock.mockReturnValue([true, true]); + // Reset mock functions before each test + (useAccessReviewForModel as jest.Mock).mockClear(); + (useWorkspaceInfo as jest.Mock).mockClear(); + (useModalLauncher as jest.Mock).mockClear(); }); - it('should return enabled actions', () => { - const { result } = renderHook(() => - useSBRActions({ - availableActions: ['update', 'delete'], - masterUserRecord: 'my-user', - role: 'admin', - bindingRequest: { name: 'my-sbr', namespace: 'my-ns' }, - }), - ); - const actions = result.current; + it('should return Edit and Revoke actions when the user has permissions', () => { + (useAccessReviewForModel as jest.Mock).mockReturnValue([true, true]); + (useWorkspaceInfo as jest.Mock).mockReturnValue({ workspace: mockWorkspace }); + (useModalLauncher as jest.Mock).mockReturnValue(jest.fn()); + + const actions = useRBActions(mockRoleBinding); - expect(actions[0]).toEqual( - expect.objectContaining({ + expect(actions).toEqual([ + { label: 'Edit access', + id: 'edit-access-user1', disabled: false, + disabledTooltip: "You don't have permission to edit access", cta: { - href: '/workspaces/test-ws/access/edit/my-user', + href: `/workspaces/${mockWorkspace}/access/edit/user1`, }, - }), - ); - - expect(actions[1]).toEqual( - expect.objectContaining({ + }, + { label: 'Revoke access', + id: 'revoke-access-user1', disabled: false, - }), - ); + disabledTooltip: "You don't have permission to revoke access", + cta: expect.any(Function), + }, + ]); }); - it('should return disabled actions due to access', () => { - accessReviewMock.mockReturnValue([false, true]); - const { result } = renderHook(() => - useSBRActions({ - availableActions: [], - masterUserRecord: 'my-user', - role: 'admin', - bindingRequest: { name: 'my-sbr', namespace: 'my-ns' }, - }), - ); - const actions = result.current; + it('should disable Edit and Revoke actions if the user does not have permissions', () => { + // Mocking the hooks with no permissions + (useAccessReviewForModel as jest.Mock).mockReturnValue([false, false]); + (useWorkspaceInfo as jest.Mock).mockReturnValue({ workspace: mockWorkspace }); + (useModalLauncher as jest.Mock).mockReturnValue(jest.fn()); + + const actions = useRBActions(mockRoleBinding); - expect(actions[0]).toEqual( - expect.objectContaining({ + expect(actions).toEqual([ + { label: 'Edit access', + id: 'edit-access-user1', disabled: true, disabledTooltip: "You don't have permission to edit access", - }), - ); - expect(actions[1]).toEqual( - expect.objectContaining({ + cta: { + href: `/workspaces/${mockWorkspace}/access/edit/user1`, + }, + }, + { label: 'Revoke access', + id: 'revoke-access-user1', disabled: true, disabledTooltip: "You don't have permission to revoke access", - }), - ); + cta: expect.any(Function), + }, + ]); }); - it('should return disabled actions if binding request is not provided', () => { - const { result } = renderHook(() => - useSBRActions({ - availableActions: ['update', 'delete'], - masterUserRecord: 'my-user', - role: 'admin', - }), - ); - const actions = result.current; + it('should disable actions if no user is found in the subjects array', () => { + (useAccessReviewForModel as jest.Mock).mockReturnValue([true, true]); + (useWorkspaceInfo as jest.Mock).mockReturnValue({ workspace: mockWorkspace }); + (useModalLauncher as jest.Mock).mockReturnValue(jest.fn()); - expect(actions[0]).toEqual( - expect.objectContaining({ + const actions = useRBActions(mockRoleBindingWithoutUser); + + expect(actions).toEqual([ + { label: 'Edit access', + id: 'edit-access-undefined', disabled: true, - }), - ); - expect(actions[1]).toEqual( - expect.objectContaining({ + disabledTooltip: "You don't have permission to edit access", + cta: { + href: '/workspaces/test-workspace/access/edit/undefined', + }, + }, + { label: 'Revoke access', + id: 'revoke-access-undefined', disabled: true, - }), - ); + disabledTooltip: "You don't have permission to revoke access", + cta: expect.any(Function), + }, + ]); }); }); diff --git a/src/components/UserAccess/index.ts b/src/components/UserAccess/index.ts index 093151d2..a7627e44 100644 --- a/src/components/UserAccess/index.ts +++ b/src/components/UserAccess/index.ts @@ -1,4 +1,4 @@ -import { SpaceBindingRequestModel } from '../../models'; +import { RoleBindingModel, SpaceBindingRequestModel } from '../../models'; import { createLoaderWithAccessCheck } from '../../utils/rbac'; export const userAccessListPageLoader = createLoaderWithAccessCheck(() => null, { @@ -11,6 +11,12 @@ export const grantAccessPageLoader = createLoaderWithAccessCheck(() => null, { verb: 'create', }); +// We delete and create roles which looks like 'patch'. +export const editAccessPageLoader = createLoaderWithAccessCheck(() => null, { + model: RoleBindingModel, + verb: 'patch', +}); + export { default as UserAccessListPage } from './UserAccessListPage'; export { default as GrantAccessPage } from './UserAccessForm/GrantAccessPage'; export { default as EditAccessPage } from './UserAccessForm/EditAccessPage'; diff --git a/src/components/UserAccess/user-access-actions.ts b/src/components/UserAccess/user-access-actions.ts index 90ee1cc0..d04f21cf 100644 --- a/src/components/UserAccess/user-access-actions.ts +++ b/src/components/UserAccess/user-access-actions.ts @@ -1,41 +1,42 @@ -import { SpaceBindingRequestModel } from '../../models'; +import { RoleBindingModel } from '../../models'; import { Action } from '../../shared/components/action-menu/types'; -import { WorkspaceBinding } from '../../types'; +import { RoleBinding } from '../../types'; import { useAccessReviewForModel } from '../../utils/rbac'; import { createRawModalLauncher } from '../modal/createModalLauncher'; import { useModalLauncher } from '../modal/ModalProvider'; import { useWorkspaceInfo } from '../Workspace/useWorkspaceInfo'; import { RevokeAccessModal } from './RevokeAccessModal'; -const revokeAccessModalLauncher = (username: string, sbr: WorkspaceBinding['bindingRequest']) => +const revokeAccessModalLauncher = (username: string, rb: RoleBinding) => createRawModalLauncher(RevokeAccessModal, { 'data-test': 'revoke-access-modal', title: 'Revoke access?', titleIconVariant: 'warning', - })({ username, sbr }); + })({ username, rb }); -export const useSBRActions = (binding: WorkspaceBinding): Action[] => { +export const useRBActions = (binding: RoleBinding): Action[] => { const showModal = useModalLauncher(); const { workspace } = useWorkspaceInfo(); - const [canUpdateSBR] = useAccessReviewForModel(SpaceBindingRequestModel, 'update'); - const [canDeleteSBR] = useAccessReviewForModel(SpaceBindingRequestModel, 'delete'); - const canUpdate = binding.bindingRequest && canUpdateSBR; - const canDelete = binding.bindingRequest && canDeleteSBR; - + const [canUpdateRB] = useAccessReviewForModel(RoleBindingModel, 'update'); + const [canDeleteRB] = useAccessReviewForModel(RoleBindingModel, 'delete'); + // Check if the user exists in the subjects array (subject kind is User) + const userSubject = binding.subjects.find((subject) => subject.kind === 'User'); + // Can update or delete based on permissions and user subject + const canUpdate = userSubject && canUpdateRB; + const canDelete = userSubject && canDeleteRB; return [ { label: 'Edit access', - id: `edit-access-${binding.masterUserRecord}`, + id: `edit-access-${userSubject?.name}`, disabled: !canUpdate, disabledTooltip: "You don't have permission to edit access", cta: { - href: `/workspaces/${workspace}/access/edit/${binding.masterUserRecord}`, + href: `/workspaces/${workspace}/access/edit/${userSubject?.name}`, }, }, { - cta: () => - showModal(revokeAccessModalLauncher(binding.masterUserRecord, binding.bindingRequest)), - id: `revoke-access-${binding.masterUserRecord}`, + cta: () => showModal(revokeAccessModalLauncher(userSubject?.name, binding)), + id: `revoke-access-${userSubject?.name}`, label: 'Revoke access', disabled: !canDelete, disabledTooltip: "You don't have permission to revoke access", diff --git a/src/hooks/__tests__/useRole.spec.ts b/src/hooks/__tests__/useRole.spec.ts new file mode 100644 index 00000000..6970e717 --- /dev/null +++ b/src/hooks/__tests__/useRole.spec.ts @@ -0,0 +1,98 @@ +import { renderHook } from '@testing-library/react-hooks'; +import { + defaultKonfluxRoleMap, + invalidMockConfigMap, + mockConfigMap, +} from '../../__data__/role-data'; +import { useK8sWatchResource } from '../../k8s'; +import { useRoleMap } from '../useRole'; + +jest.mock('../../k8s', () => ({ + useK8sWatchResource: jest.fn(), +})); + +describe('useRoleMap', () => { + it('should return loading state initially', () => { + (useK8sWatchResource as jest.Mock).mockReturnValue({ + data: undefined, + isLoading: true, + error: null, + }); + + const { result } = renderHook(() => useRoleMap(true)); + + expect(result.current[0]).toEqual({}); + expect(result.current[1]).toBe(true); + expect(result.current[2]).toBeNull(); + }); + + it('should handle error state correctly', () => { + (useK8sWatchResource as jest.Mock).mockReturnValue({ + data: undefined, + isLoading: false, + error: 'Error occurred', + }); + + const { result } = renderHook(() => useRoleMap(true)); + + expect(result.current[0]).toEqual({}); + expect(result.current[1]).toBe(false); + expect(result.current[2]).toBe('Error occurred'); + }); + + it('should transform and return role map correctly when data is available', () => { + (useK8sWatchResource as jest.Mock).mockReturnValue({ + data: mockConfigMap, + isLoading: false, + error: null, + }); + + const { result } = renderHook(() => useRoleMap(true)); + + expect(result.current[0]).toEqual(defaultKonfluxRoleMap); + expect(result.current[1]).toBe(false); + expect(result.current[2]).toBeNull(); + }); + + it('should handle malformed JSON gracefully', () => { + (useK8sWatchResource as jest.Mock).mockReturnValue({ + data: invalidMockConfigMap, + isLoading: false, + error: null, + }); + + const { result } = renderHook(() => useRoleMap(true)); + + // Ensure that even with malformed JSON, the role map is still empty + expect(result.current[0]).toEqual({}); + expect(result.current[1]).toBe(false); + expect(result.current[2]).toBeNull(); + }); + + it('should memoize the role map transformation', () => { + (useK8sWatchResource as jest.Mock).mockReturnValue({ + data: mockConfigMap, + isLoading: false, + error: null, + }); + + const { result, rerender } = renderHook(() => useRoleMap(true)); + + const initialRoleMap = result.current[0]; + + // Rerender with the same props and check if memoization works (no change) + rerender(); + expect(result.current[0]).toBe(initialRoleMap); // Role map should not change + + // Mock the hook again with the updated configMap + (useK8sWatchResource as jest.Mock).mockReturnValue({ + data: invalidMockConfigMap, + isLoading: false, + error: null, + }); + + rerender(); + // Role map should have changed + expect(result.current[0]).not.toBe(initialRoleMap); + }); +}); diff --git a/src/hooks/__tests__/useRoleBindings.spec.ts b/src/hooks/__tests__/useRoleBindings.spec.ts new file mode 100644 index 00000000..6e3a97be --- /dev/null +++ b/src/hooks/__tests__/useRoleBindings.spec.ts @@ -0,0 +1,96 @@ +import { renderHook } from '@testing-library/react'; +import { defaultKonfluxRoleMap } from '../../__data__/role-data'; +import { mockRoleBindings, mockRoleBinding } from '../../__data__/rolebinding-data'; +import { createK8sUtilMock } from '../../utils/test-utils'; +import { useRoleMap } from '../useRole'; +import { useRoleBindings } from '../useRoleBindings'; + +const k8sWatchMock = createK8sUtilMock('useK8sWatchResource'); +jest.mock('../useRole', () => ({ + useRoleMap: jest.fn(), +})); + +describe('useRoleBindings', () => { + const mockNamespace = 'test-ns'; + const mockUseRoleMap = useRoleMap as jest.Mock; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should return an empty array when bindings are loading', () => { + k8sWatchMock.mockReturnValue({ + data: [], + isLoading: true, + error: null, + }); + + mockUseRoleMap.mockReturnValue([{}, false]); + const { result } = renderHook(() => useRoleBindings(mockNamespace, true)); + expect(result.current[0]).toEqual([]); + expect(result.current[1]).toBe(true); + }); + + it('should return filtered bindings when roleMap is ready', () => { + k8sWatchMock.mockReturnValue({ + data: [mockRoleBinding], + isLoading: false, + }); + + mockUseRoleMap.mockReturnValue([defaultKonfluxRoleMap, false]); + + const { result } = renderHook(() => useRoleBindings(mockNamespace, true)); + + expect(result.current[0]).toEqual([mockRoleBinding]); + expect(result.current[1]).toBe(false); // not loading + }); + + it('should return an empty array if roleMap is empty', () => { + k8sWatchMock.mockReturnValue({ + data: mockRoleBindings, + isLoading: false, + error: null, + }); + + mockUseRoleMap.mockReturnValue([{}, false]); + + const { result } = renderHook(() => useRoleBindings(mockNamespace, true)); + expect(result.current[0]).toEqual([]); + }); + + it('should handle error state in bindings', () => { + k8sWatchMock.mockReturnValue({ + data: [], + isLoading: false, + error: 'Error fetching role bindings', + }); + + mockUseRoleMap.mockReturnValue([defaultKonfluxRoleMap, false]); + const { result } = renderHook(() => useRoleBindings(mockNamespace, true)); + expect(result.current[2]).toBe('Error fetching role bindings'); + }); + + it('should handle error state in roleMap', () => { + k8sWatchMock.mockReturnValue({ + data: [], + isLoading: false, + error: null, + }); + + mockUseRoleMap.mockReturnValue([defaultKonfluxRoleMap, false, 'Error fetching roleMap']); + const { result } = renderHook(() => useRoleBindings(mockNamespace, true)); + expect(result.current[2]).toBe('Error fetching roleMap'); + }); + + it('should not filter role bindings while roleMap is loading', () => { + k8sWatchMock.mockReturnValue({ + data: mockRoleBindings, + isLoading: false, + error: null, + }); + + mockUseRoleMap.mockReturnValue([{}, true]); + const { result } = renderHook(() => useRoleBindings(mockNamespace, true)); + expect(result.current[0]).toEqual([]); + }); +}); diff --git a/src/hooks/useRole.ts b/src/hooks/useRole.ts new file mode 100644 index 00000000..95e75486 --- /dev/null +++ b/src/hooks/useRole.ts @@ -0,0 +1,40 @@ +import React from 'react'; +import { useK8sWatchResource } from '../k8s'; +import { ConfigMapGroupVersionKind, ConfigMapModel } from '../models'; +import { ConfigMap } from '../types/configmap'; + +export const useRoleMap = (watch?: boolean): [object, boolean, unknown] => { + const { + data: configMap, + isLoading: roleLoading, + error, + } = useK8sWatchResource( + { + groupVersionKind: ConfigMapGroupVersionKind, + namespace: 'konflux-info', + name: 'konflux-public-info', + watch, + }, + ConfigMapModel, + ); + + const roleMap = {}; + + // Memoize the result of role map transformation for performance + const transformedRoleMap = React.useMemo(() => { + if (!roleLoading && !error && configMap?.data) { + const rbacJson = JSON.parse(configMap.data['info.json'] as string); + const rbacItems = Array.isArray(rbacJson?.rbac) ? rbacJson.rbac : []; + + rbacItems.forEach((role) => { + if (role?.roleRef?.name && role.displayName) { + roleMap[role.roleRef.name] = role.displayName; + } + }); + } + return roleMap; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [configMap, roleLoading, error]); + + return [transformedRoleMap, roleLoading, error]; +}; diff --git a/src/hooks/useRoleBindings.ts b/src/hooks/useRoleBindings.ts new file mode 100644 index 00000000..61a7cbff --- /dev/null +++ b/src/hooks/useRoleBindings.ts @@ -0,0 +1,34 @@ +import React from 'react'; +import { useK8sWatchResource } from '../k8s'; +import { RoleBindingGroupVersionKind, RoleBindingModel } from '../models'; +import { RoleBinding } from '../types'; +import { useRoleMap } from './useRole'; + +export const useRoleBindings = ( + namespace: string, + watch?: boolean, +): [RoleBinding[], boolean, unknown] => { + const { + data: bindings, + isLoading: bindingsLoading, + error, + } = useK8sWatchResource( + { + groupVersionKind: RoleBindingGroupVersionKind, + namespace, + isList: true, + watch, + }, + RoleBindingModel, + ); + const [roleMap, roleMapLoading, roleMapError] = useRoleMap(watch); + const konfluxRBs: RoleBinding[] = React.useMemo( + () => + !roleMapError && !bindingsLoading && !roleMapLoading && Array.isArray(bindings) && roleMap + ? bindings?.filter((rb) => Object.keys(roleMap).includes(rb.roleRef.name)) + : [], + [bindings, bindingsLoading, roleMap, roleMapLoading, roleMapError], + ); + + return [konfluxRBs, bindingsLoading, error || roleMapError]; +}; diff --git a/src/hooks/useSpaceBindingRequests.ts b/src/hooks/useSpaceBindingRequests.ts deleted file mode 100644 index a3833370..00000000 --- a/src/hooks/useSpaceBindingRequests.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { useK8sWatchResource } from '../k8s'; -import { SpaceBindingRequestGroupVersionKind, SpaceBindingRequestModel } from '../models'; -import { SpaceBindingRequest } from '../types'; - -export const useSpaceBindingRequest = ( - namespace: string, - workspace: string, - name: string, -): [SpaceBindingRequest, boolean, unknown] => { - const { - data: binding, - isLoading, - error, - } = useK8sWatchResource( - { - groupVersionKind: SpaceBindingRequestGroupVersionKind, - name, - namespace, - workspace, - }, - SpaceBindingRequestModel, - ); - return [binding, !isLoading, error]; -}; diff --git a/src/models/index.ts b/src/models/index.ts index 994896d2..55772296 100644 --- a/src/models/index.ts +++ b/src/models/index.ts @@ -22,5 +22,6 @@ export * from './limit-range'; export * from './remotesecret'; export * from './workspace'; export * from './space-binding-request'; +export * from './role-binding'; export * from './config-map'; export * from './image-repository'; diff --git a/src/models/role-binding.ts b/src/models/role-binding.ts new file mode 100644 index 00000000..1c279786 --- /dev/null +++ b/src/models/role-binding.ts @@ -0,0 +1,14 @@ +import { K8sModelCommon, K8sGroupVersionKind } from '../types/k8s'; + +export const RoleBindingModel: K8sModelCommon = { + apiVersion: 'v1', + apiGroup: 'rbac.authorization.k8s.io', + kind: 'RoleBinding', + plural: 'rolebindings', +}; + +export const RoleBindingGroupVersionKind: K8sGroupVersionKind = { + group: 'rbac.authorization.k8s.io', + version: 'v1', + kind: 'RoleBinding', +}; diff --git a/src/routes/index.tsx b/src/routes/index.tsx index a4088d04..2f9fbbfc 100644 --- a/src/routes/index.tsx +++ b/src/routes/index.tsx @@ -80,6 +80,8 @@ import { import { GrantAccessPage, grantAccessPageLoader, + EditAccessPage, + editAccessPageLoader, UserAccessListPage, userAccessListPageLoader, } from '../components/UserAccess'; @@ -348,8 +350,8 @@ export const router = createBrowserRouter([ }, { path: `/workspaces/:${RouterParams.workspaceName}/access/edit/:${RouterParams.bindingName}`, - element: , - errorElement: , + loader: editAccessPageLoader, + element: , }, { path: `/workspaces/:${RouterParams.workspaceName}/access`, diff --git a/src/types/configmap.ts b/src/types/configmap.ts new file mode 100644 index 00000000..b4608914 --- /dev/null +++ b/src/types/configmap.ts @@ -0,0 +1,11 @@ +import { K8sResourceCommon } from './k8s'; + +export type ConfigMap = K8sResourceCommon & { + kind: string; + apiVersion: string; + metadata: { + name: string; + namespace: string; + }; + data: string; +}; diff --git a/src/types/index.ts b/src/types/index.ts index 60187edf..9f916319 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -16,7 +16,7 @@ export * from './rbac'; export * from './secret'; export * from './gitops-deployment'; export * from './release'; -export * from './space-binding-request'; +export * from './role-binding'; export * from './workspace'; export * from './image-repository'; export * from './enterprise-contract'; diff --git a/src/types/k8s.ts b/src/types/k8s.ts index 46c094e3..1f64818a 100644 --- a/src/types/k8s.ts +++ b/src/types/k8s.ts @@ -157,7 +157,7 @@ export type K8sResourceKindReference = GroupVersionKind; export type WatchK8sResource = { groupVersionKind: K8sGroupVersionKind; name?: string; - workspace: string; + workspace?: string; namespace: string; isList?: boolean; selector?: Selector; diff --git a/src/types/role-binding.ts b/src/types/role-binding.ts new file mode 100644 index 00000000..8403f1ce --- /dev/null +++ b/src/types/role-binding.ts @@ -0,0 +1,15 @@ +import { K8sResourceCommon } from './k8s'; + +export type RoleBinding = K8sResourceCommon & { + roleRef: { + apiGroup: string; + kind: string; + name: string; + }; + subjects: { + apiGroup: string; + kind: string; + name: string; + namespace: string; + }[]; +}; diff --git a/src/types/space-binding-request.ts b/src/types/space-binding-request.ts deleted file mode 100644 index b57cf006..00000000 --- a/src/types/space-binding-request.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { K8sResourceCommon } from './k8s'; -import { WorkspaceRole } from './workspace'; - -export type SpaceBindingRequest = K8sResourceCommon & { - spec: { - masterUserRecord: string; - spaceRole: WorkspaceRole; - }; - status?: { - conditions?: { - reason?: string; - message?: string; - status?: string; - ready?: string; - }[]; - }; -}; diff --git a/src/types/workspace.ts b/src/types/workspace.ts index 2fdb888a..eecbff17 100644 --- a/src/types/workspace.ts +++ b/src/types/workspace.ts @@ -5,9 +5,6 @@ export interface Workspace extends K8sResourceCommon { type?: string; namespaces: Namespace[]; owner: string; - role: WorkspaceRole; - availableRoles?: WorkspaceRole[]; - bindings?: WorkspaceBinding[]; }; } @@ -16,14 +13,4 @@ export interface Namespace { type?: string; } -export type WorkspaceRole = 'contributor' | 'maintainer' | 'admin'; - -export interface WorkspaceBinding { - masterUserRecord: string; - role: WorkspaceRole; - availableActions?: ('update' | 'delete')[]; - bindingRequest?: { - name: string; - namespace: string; - }; -} +export type NamespaceRole = 'contributor' | 'maintainer' | 'admin';