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

Bug 1833146: Hide Project Access Tab when the user have no access to role bindings #5350

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ const ProjectAccess: React.FC<ProjectAccessProps> = ({ formName, namespace, role
/>{' '}
.
</PageHeading>
<hr />
<div className="co-m-pane__body">
{roleBindings.loadError ? (
<StatusBox loaded={roleBindings.loaded} loadError={roleBindings.loadError} />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as React from 'react';
import { match as RMatch } from 'react-router';
import { history } from '@console/internal/components/utils';
import { history, useAccessReview } from '@console/internal/components/utils';
import { ALL_NAMESPACES_KEY } from '@console/shared';
import { NamespaceDetails, projectMenuActions } from '@console/internal/components/namespace';
import { ProjectModel } from '@console/internal/models';
import { ProjectModel, RoleBindingModel } from '@console/internal/models';
import { DetailsPage } from '@console/internal/components/factory';
import { ProjectDashboard } from '@console/internal/components/dashboard/project-dashboard/project-dashboard';
import { withStartGuide } from '@console/internal/components/start-guide';
Expand All @@ -29,6 +29,20 @@ const handleNamespaceChange = (newNamespace: string): void => {
export const ProjectDetailsPage: React.FC<MonitoringPageProps> = ({ match, ...props }) => {
const activeNamespace = match.params.ns;

const canListRoleBindings = useAccessReview({
group: RoleBindingModel.apiGroup,
resource: RoleBindingModel.plural,
verb: 'list',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be the use case if the user has list permission but do not have create or delete permission?
In this case, can we show this page with all the form field in the read-only state?

see, this user has list access but not create and delete access.
Screenshot 2020-05-08 at 11 29 05 AM

cc: @serenamarie125 @christianvogt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vikram-raj please raise an issue to track this. It's a general problem we need to get better at. We see the same missed experience for users with view but not edit access for health checks.

For simplicity right now it's best to stick to hiding if they cannot edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christianvogt, For now, do I need to hide the Project Access tab if the user has no permission to create & delete access?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to hide if the user cannot create role bindings.
There's other checks we should make but better to not let someone even attempt to do something they cannot complete right now.

Copy link
Member

@vikram-raj vikram-raj May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, We need to check for both create and list access here.

Created an issue to track it https://issues.redhat.com/browse/ODC-3813
https://issues.redhat.com/browse/ODC-3819

namespace: activeNamespace,
});

const canCreateRoleBindings = useAccessReview({
group: RoleBindingModel.apiGroup,
resource: RoleBindingModel.plural,
verb: 'create',
namespace: activeNamespace,
});

return (
<>
<Helmet>
Expand Down Expand Up @@ -60,11 +74,12 @@ export const ProjectDetailsPage: React.FC<MonitoringPageProps> = ({ match, ...pr
name: 'Details',
component: NamespaceDetails,
},
{
href: 'access',
name: 'Project Access',
component: ProjectAccessPage,
},
canListRoleBindings &&
canCreateRoleBindings && {
href: 'access',
name: 'Project Access',
component: ProjectAccessPage,
},
]}
/>
) : (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import { shallow } from 'enzyme';
import { BreadCrumbs } from '@console/internal/components/utils';
import * as _ from 'lodash';
import * as utils from '@console/internal/components/utils';
import { ProjectDetailsPage } from '../ProjectDetailsPage';
import ProjectListPage from '../../ProjectListPage';
import NamespacedPage from '../../../NamespacedPage';
Expand All @@ -27,6 +28,14 @@ describe('ProjectDetailsPage', () => {
// Currently rendering the breadcrumbs will buck-up against the redirects and not work as expected
const component = shallow(<ProjectDetailsPage match={testProjectMatch} />);

expect(component.find(BreadCrumbs).exists()).not.toBe(true);
expect(component.find(utils.BreadCrumbs).exists()).not.toBe(true);
});

it('should not render the Project Access tab if user has no access to role bindings', () => {
const spyUseAccessReview = jest.spyOn(utils, 'useAccessReview');
spyUseAccessReview.mockReturnValue(false);
const component = shallow(<ProjectDetailsPage match={testProjectMatch} />);
const pages = component.find(DetailsPage).prop('pages');
expect(_.find(pages, { name: 'Project Access' })).toBe(undefined);
});
});