From 6ab6afd15a3728143681c8591244fe3aaba8990e Mon Sep 17 00:00:00 2001 From: Pushpa Padti <99261071+ppadti@users.noreply.github.com> Date: Fri, 11 Oct 2024 21:40:05 +0530 Subject: [PATCH] Disable manage permissions when no rolebinding present for the default group (#3306) --- frontend/src/__mocks__/mockModelRegistry.ts | 36 +++++------ .../cypress/pages/modelRegistrySettings.ts | 4 ++ .../modelRegistrySettings.cy.ts | 60 ++++++++++++++++++- .../ModelRegistriesTable.tsx | 12 +++- .../ModelRegistriesTableRow.tsx | 35 ++++++++--- .../ModelRegistrySettings.tsx | 9 ++- 6 files changed, 125 insertions(+), 31 deletions(-) diff --git a/frontend/src/__mocks__/mockModelRegistry.ts b/frontend/src/__mocks__/mockModelRegistry.ts index 66cda6238c..d734162a3a 100644 --- a/frontend/src/__mocks__/mockModelRegistry.ts +++ b/frontend/src/__mocks__/mockModelRegistry.ts @@ -1,13 +1,30 @@ -import { ModelRegistryKind } from '~/k8sTypes'; +import { K8sCondition, ModelRegistryKind } from '~/k8sTypes'; type MockModelRegistryType = { name?: string; namespace?: string; + conditions?: K8sCondition[]; }; export const mockModelRegistry = ({ name = 'modelregistry-sample', namespace = 'odh-model-registries', + conditions = [ + { + lastTransitionTime: '2024-03-22T09:30:02Z', + message: 'Deployment for custom resource modelregistry-sample was successfully created', + reason: 'CreatedDeployment', + status: 'True', + type: 'Progressing', + }, + { + lastTransitionTime: '2024-03-14T08:11:26Z', + message: 'Deployment for custom resource modelregistry-sample is available', + reason: 'DeploymentAvailable', + status: 'True', + type: 'Available', + }, + ], }: MockModelRegistryType): ModelRegistryKind => ({ apiVersion: 'modelregistry.opendatahub.io/v1alpha1', kind: 'ModelRegistry', @@ -39,21 +56,6 @@ export const mockModelRegistry = ({ }, }, status: { - conditions: [ - { - lastTransitionTime: '2024-03-22T09:30:02Z', - message: 'Deployment for custom resource modelregistry-sample was successfully created', - reason: 'CreatedDeployment', - status: 'True', - type: 'Progressing', - }, - { - lastTransitionTime: '2024-03-14T08:11:26Z', - message: 'Deployment for custom resource modelregistry-sample is available', - reason: 'DeploymentAvailable', - status: 'True', - type: 'Available', - }, - ], + conditions, }, }); diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts index 12c5fcc3ba..7b9e779252 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts @@ -90,6 +90,10 @@ class ModelRegistrySettings { return cy.findByTestId('modal-submit-button'); } + findManagePermissionsTooltip() { + return cy.findByRole('tooltip'); + } + findTable() { return cy.findByTestId('model-registries-table'); } diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts index e28c313a8b..a9e907203f 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts @@ -12,6 +12,17 @@ import { asProjectAdminUser, } from '~/__tests__/cypress/cypress/utils/mockUsers'; import { mockModelRegistry } from '~/__mocks__/mockModelRegistry'; +import type { RoleBindingSubject } from '~/k8sTypes'; +import { mockRoleBindingK8sResource } from '~/__mocks__/mockRoleBindingK8sResource'; +import { verifyRelativeURL } from '~/__tests__/cypress/cypress/utils/url'; + +const groupSubjects: RoleBindingSubject[] = [ + { + kind: 'Group', + apiGroup: 'rbac.authorization.k8s.io', + name: 'example-mr-users', + }, +]; const setupMocksForMRSettingAccess = ({ hasModelRegistries = true, @@ -47,7 +58,19 @@ const setupMocksForMRSettingAccess = ({ ? [ mockModelRegistry({ name: 'test-registry-1' }), mockModelRegistry({ name: 'test-registry-2' }), - mockModelRegistry({ name: 'test-registry-3' }), + mockModelRegistry({ + name: 'test-registry-3', + conditions: [ + { + lastTransitionTime: '2024-03-22T09:30:02Z', + message: + 'Deployment for custom resource modelregistry-sample was successfully created', + reason: 'CreatedDeployment', + status: 'True', + type: 'Progressing', + }, + ], + }), ] : [], ), @@ -71,6 +94,26 @@ const setupMocksForMRSettingAccess = ({ req.reply(500); // Something went wrong on the backend when decoding the secret }, ); + + cy.interceptOdh( + 'GET /api/modelRegistryRoleBindings', + mockK8sResourceList([ + mockRoleBindingK8sResource({ + namespace: 'odh-model-registries', + name: 'test-registry-1-user', + subjects: groupSubjects, + roleRefName: 'registry-user-test-registry-1', + modelRegistryName: 'test-registry-1', + }), + mockRoleBindingK8sResource({ + namespace: 'odh-model-registries', + name: 'test-registry-2-user', + subjects: groupSubjects, + roleRefName: 'registry-user-test-registry-2', + modelRegistryName: 'test-registry-2', + }), + ]), + ); }; it('Model registry settings should not be available for non product admins', () => { @@ -170,14 +213,25 @@ describe('ViewDatabaseConfigModal', () => { }); }); -describe('ManagePermissionsModal', () => { - beforeEach(() => { +describe('ManagePermissions', () => { + it('Manage permission is enabled, when there is a rolebinding', () => { setupMocksForMRSettingAccess({}); modelRegistrySettings.visit(true); modelRegistrySettings .findModelRegistryRow('test-registry-1') .findByText('Manage permissions') .click(); + verifyRelativeURL('/modelRegistrySettings/permissions/test-registry-1'); + }); + + it('Manage permission is disabled, when there is no rolebinding', () => { + setupMocksForMRSettingAccess({}); + modelRegistrySettings.visit(true); + modelRegistrySettings + .findModelRegistryRow('test-registry-3') + .findByText('Manage permissions') + .trigger('mouseenter'); + modelRegistrySettings.findManagePermissionsTooltip().should('be.visible'); }); }); diff --git a/frontend/src/pages/modelRegistrySettings/ModelRegistriesTable.tsx b/frontend/src/pages/modelRegistrySettings/ModelRegistriesTable.tsx index 011e7a67f4..3afdb311d7 100644 --- a/frontend/src/pages/modelRegistrySettings/ModelRegistriesTable.tsx +++ b/frontend/src/pages/modelRegistrySettings/ModelRegistriesTable.tsx @@ -1,18 +1,21 @@ import React from 'react'; import { Button, Toolbar, ToolbarContent, ToolbarItem } from '@patternfly/react-core'; import { Table } from '~/components/table'; -import { ModelRegistryKind } from '~/k8sTypes'; +import { ModelRegistryKind, RoleBindingKind } from '~/k8sTypes'; +import { ContextResourceData } from '~/types'; import { modelRegistryColumns } from './columns'; import ModelRegistriesTableRow from './ModelRegistriesTableRow'; type ModelRegistriesTableProps = { modelRegistries: ModelRegistryKind[]; refresh: () => Promise; + roleBindings: ContextResourceData; onCreateModelRegistryClick: () => void; }; const ModelRegistriesTable: React.FC = ({ modelRegistries, + roleBindings, refresh, onCreateModelRegistryClick, }) => ( @@ -36,7 +39,12 @@ const ModelRegistriesTable: React.FC = ({ } rowRenderer={(mr) => ( - + )} variant="compact" /> diff --git a/frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx b/frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx index 062085ed1b..55cb4b54e2 100644 --- a/frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx +++ b/frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx @@ -1,23 +1,34 @@ import React from 'react'; import { ActionsColumn, Td, Tr } from '@patternfly/react-table'; -import { Link } from 'react-router-dom'; -import { ModelRegistryKind } from '~/k8sTypes'; +import { useNavigate } from 'react-router-dom'; +import { Button, Tooltip } from '@patternfly/react-core'; +import { ModelRegistryKind, RoleBindingKind } from '~/k8sTypes'; import ResourceNameTooltip from '~/components/ResourceNameTooltip'; +import { ContextResourceData } from '~/types'; import ViewDatabaseConfigModal from './ViewDatabaseConfigModal'; import DeleteModelRegistryModal from './DeleteModelRegistryModal'; import { ModelRegistryTableRowStatus } from './ModelRegistryTableRowStatus'; type ModelRegistriesTableRowProps = { modelRegistry: ModelRegistryKind; + roleBindings: ContextResourceData; refresh: () => Promise; }; const ModelRegistriesTableRow: React.FC = ({ modelRegistry: mr, + roleBindings, refresh, }) => { const [isDatabaseConfigModalOpen, setIsDatabaseConfigModalOpen] = React.useState(false); const [isDeleteModalOpen, setIsDeleteModalOpen] = React.useState(false); + const navigate = useNavigate(); + const filteredRoleBindings = roleBindings.data.filter( + (rb) => + rb.metadata.labels?.['app.kubernetes.io/name'] === + (mr.metadata.name || mr.metadata.annotations?.['openshift.io/display-name']), + ); + return ( <> @@ -35,12 +46,20 @@ const ModelRegistriesTableRow: React.FC = ({ - - Manage permissions - + {filteredRoleBindings.length === 0 ? ( + + + + ) : ( + + )} { const [createModalOpen, setCreateModalOpen] = React.useState(false); - const [modelRegistries, loaded, loadError, refreshModelRegistries] = useModelRegistriesBackend(); + const [modelRegistries, mrloaded, loadError, refreshModelRegistries] = + useModelRegistriesBackend(); + const roleBindings = useContextResourceData(useModelRegistryRoleBindings()); const { refreshRulesReview } = React.useContext(ModelRegistrySelectorContext); + const loaded = mrloaded && roleBindings.loaded; const refreshAll = React.useCallback( () => Promise.all([refreshModelRegistries(), refreshRulesReview()]), @@ -65,6 +71,7 @@ const ModelRegistrySettings: React.FC = () => { > { setCreateModalOpen(true);