Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer project kebab check and disable #3003

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import {
ProjectModel,
ProjectRequestModel,
RouteModel,
SelfSubjectAccessReviewModel,
} from '~/__tests__/cypress/cypress/utils/models';
import { mock200Status } from '~/__mocks__/mockK8sStatus';
import { mockNotebookK8sResource, mockRouteK8sResource } from '~/__mocks__';
import { mockPodK8sResource } from '~/__mocks__/mockPodK8sResource';
import { mockSelfSubjectAccessReview } from '~/__mocks__/mockSelfSubjectAccessReview';
import { asProjectAdminUser } from '~/__tests__/cypress/cypress/utils/users';
import { notebookConfirmModal } from '~/__tests__/cypress/cypress/pages/workbench';
import { testPagination } from '~/__tests__/cypress/cypress/utils/pagination';
Expand Down Expand Up @@ -78,7 +80,16 @@ describe('Data science projects details', () => {
it('should delete project', () => {
initIntercepts();
projectListPage.visit();
projectListPage.getProjectRow('Test Project').findKebabAction('Delete project').click();
cy.interceptK8s(
'POST',
SelfSubjectAccessReviewModel,
mockSelfSubjectAccessReview({ allowed: true }),
).as('selfSubjectAccessReviewsCall');
const deleteProject = projectListPage
.getProjectRow('Test Project')
.findKebabAction('Delete project');
cy.wait('@selfSubjectAccessReviewsCall');
deleteProject.click();
deleteModal.shouldBeOpen();
deleteModal.findSubmitButton().should('be.disabled');
deleteModal.findCancelButton().should('be.enabled').click();
Expand Down Expand Up @@ -154,6 +165,31 @@ describe('Data science projects details', () => {
projectListPage.findProjectLink('renamed').should('not.exist');
});

it('should disable kebab actions with insufficient permissions', () => {
initIntercepts();
projectListPage.visit();
cy.interceptK8s(
'POST',
SelfSubjectAccessReviewModel,
mockSelfSubjectAccessReview({ allowed: false }),
).as('selfSubjectAccessReviewsCall');

const editProject = projectListPage
.getProjectRow('Test Project')
.findKebabAction('Edit project');
const editPermission = projectListPage
.getProjectRow('Test Project')
.findKebabAction('Edit permissions');
const deleteProject = projectListPage
.getProjectRow('Test Project')
.findKebabAction('Delete project');
cy.wait('@selfSubjectAccessReviewsCall');

editProject.should('have.attr', 'aria-disabled', 'true');
editPermission.should('have.attr', 'aria-disabled', 'true');
deleteProject.should('have.attr', 'aria-disabled', 'true');
});

describe('Table filter', () => {
it('filter by name', () => {
initIntercepts();
Expand Down
33 changes: 18 additions & 15 deletions frontend/src/api/useAccessReview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const checkAccess = ({

export const useAccessReview = (
resourceAttributes: AccessReviewResourceAttributes,
shouldRunCheck = true,
): [boolean, boolean] => {
const [loaded, setLoaded] = React.useState(false);
const [isAllowed, setAllowed] = React.useState(false);
Expand All @@ -51,22 +52,24 @@ export const useAccessReview = (
} = resourceAttributes;

React.useEffect(() => {
checkAccess({ group, resource, subresource, verb, name, namespace })
.then((result) => {
if (result.status) {
setAllowed(result.status.allowed);
} else {
if (shouldRunCheck) {
checkAccess({ group, resource, subresource, verb, name, namespace })
.then((result) => {
if (result.status) {
setAllowed(result.status.allowed);
} else {
setAllowed(true);
}
setLoaded(true);
})
.catch((e) => {
// eslint-disable-next-line no-console
console.warn('SelfSubjectAccessReview failed', e);
setAllowed(true);
}
setLoaded(true);
})
.catch((e) => {
// eslint-disable-next-line no-console
console.warn('SelfSubjectAccessReview failed', e);
setAllowed(true);
setLoaded(true);
});
}, [group, name, namespace, resource, subresource, verb]);
setLoaded(true);
});
}
}, [group, name, namespace, resource, subresource, verb, shouldRunCheck]);

return [isAllowed, loaded];
};
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ const ProjectTableRow: React.FC<ProjectTableRowProps> = ({
const navigate = useNavigate();
const owner = getProjectOwner(project);

const item = useProjectTableRowItems(project, isRefreshing, setEditData, setDeleteData);
const [item, runAccessCheck] = useProjectTableRowItems(
project,
isRefreshing,
setEditData,
setDeleteData,
);
const [notebookStates, loaded] = useProjectNotebookStates(project.metadata.name);

return (
Expand Down Expand Up @@ -130,6 +135,8 @@ const ProjectTableRow: React.FC<ProjectTableRowProps> = ({
className="odh-project-table__action-column"
isActionCell
rowSpan={notebookStates.length || 1}
onMouseEnter={runAccessCheck}
onClick={runAccessCheck}
>
<ActionsColumn items={item} />
</Td>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import * as React from 'react';
import { useNavigate } from 'react-router-dom';
import { TooltipProps } from '@patternfly/react-core';
import { useAccessReview } from '~/api';
import { AccessReviewResourceAttributes, ProjectKind } from '~/k8sTypes';

type KebabItem = {
title?: string;
isDisabled?: boolean;
isAriaDisabled?: boolean;
isSeparator?: boolean;
onClick?: () => void;
tooltipProps?: TooltipProps;
};
const accessReviewResource: AccessReviewResourceAttributes = {
group: 'rbac.authorization.k8s.io',
Expand All @@ -18,40 +21,72 @@ const useProjectTableRowItems = (
isRefreshing: boolean,
setEditData: (data: ProjectKind) => void,
setDeleteData: (data: ProjectKind) => void,
): KebabItem[] => {
const [allowCreate] = useAccessReview({
...accessReviewResource,
namespace: project.metadata.name,
});
): [KebabItem[], () => void] => {
const navigate = useNavigate();
const [shouldRunCheck, setShouldRunCheck] = React.useState(false);

const [allowUpdate, allowUpdateLoaded] = useAccessReview(
{
...accessReviewResource,
namespace: project.metadata.name,
verb: 'update',
},
shouldRunCheck,
);
const [allowCreate, allowCreateLoaded] = useAccessReview(
{
...accessReviewResource,
namespace: project.metadata.name,
},
shouldRunCheck,
);
const [allowDelete, allowDeleteLoaded] = useAccessReview(
{
...accessReviewResource,
namespace: project.metadata.name,
verb: 'delete',
},
shouldRunCheck,
);

const runAccesCheck = React.useCallback(() => {
setShouldRunCheck(true);
}, []);

const noPermissionToolTip = (allow: boolean, loaded: boolean): Partial<KebabItem> | undefined =>
!allow && loaded
? { tooltipProps: { content: 'You do not have permissions to perform this action' } }
: undefined;

const item: KebabItem[] = [
{
title: 'Edit project',
isDisabled: isRefreshing,
isAriaDisabled: isRefreshing || !allowUpdate || !allowUpdateLoaded,
onClick: () => {
setEditData(project);
},
...noPermissionToolTip(allowUpdate, allowUpdateLoaded),
},
{
title: 'Edit permissions',
isAriaDisabled: !allowCreate || !allowCreateLoaded,
onClick: () => {
navigate(`/projects/${project.metadata.name}?section=permissions`);
},
...noPermissionToolTip(allowCreate, allowCreateLoaded),
},
...(allowCreate
? [
{
title: 'Edit permissions',
onClick: () => {
navigate(`/projects/${project.metadata.name}?section=permissions`);
},
},
]
: []),
{
isSeparator: true,
},
{
title: 'Delete project',
isAriaDisabled: !allowDelete || !allowDeleteLoaded,
onClick: () => {
setDeleteData(project);
},
...noPermissionToolTip(allowDelete, allowDeleteLoaded),
},
];
return item;
return [item, runAccesCheck];
};
export default useProjectTableRowItems;
Loading