Skip to content

ROX-13378: Access Control page permissions #3720

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.

Sign up for GitHub

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

Merged
ivan-degtiarenko merged 10 commits into from
Nov 14, 2022
Merged

ROX-13378: Access Control page permissions #3720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f31039f
WIP
ivan-degtiarenko Nov 6, 2022
f66ab8b
Show sub page name as lower case
ivan-degtiarenko Nov 7, 2022
b2f7fd8
protect AccessControl.tsx UI if user has neither Access or Role
ivan-degtiarenko Nov 7, 2022
9b2d4cf
Fix role duplication
ivan-degtiarenko Nov 8, 2022
220e216
Fix after rebase
ivan-degtiarenko Nov 10, 2022
731f8a9
Conditional fetching of auth providers, roles and groups in AuthProvi…
ivan-degtiarenko Nov 10, 2022
fc39fd6
Move access check right after "useEffect"
ivan-degtiarenko Nov 10, 2022
bc73c5c
Fix AccessControl.tsx
ivan-degtiarenko Nov 10, 2022
4ebe7a7
Access + Role READ to read any 'Access Control' page;
ivan-degtiarenko Nov 14, 2022
6113b92
Fix access control UI tests
ivan-degtiarenko Nov 14, 2022
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
6 changes: 1 addition & 5 deletions ui/apps/platform/cypress/integration/accessControl/accessControlAccessScopes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,9 @@ describe('Access Control Access scopes', () => {
cy.wait('@GetMyPermissions');

cy.get(selectors.h1).should('have.text', h1);
cy.get(selectors.navLink).should('not.exist');

cy.get(selectors.h2).should('not.exist');

cy.get(selectors.alertTitle).should(
'contain', // not have.text because it contains "Info alert:" for screen reader
'You do not have permission to view Access Control'
'You do not have permission to view access scopes.'
);
});

Expand Down
4 changes: 1 addition & 3 deletions ui/apps/platform/cypress/integration/accessControl/accessControlAuthProviders.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,10 @@ describe('Access Control Auth providers', () => {
visitWithStaticResponseForPermissions(authProvidersUrl, staticResponseForPermissions);

cy.get(`${selectors.h1}:contains("${h1}")`);
cy.get(selectors.navLink).should('not.exist');
cy.get(selectors.h2).should('not.exist');

cy.get(selectors.alertTitle).should(
'contain', // instead of have.text because of "Info alert:" for screen reader
'You do not have permission to view Access Control'
'You do not have permission to view auth providers.'
);
});

Expand Down
5 changes: 1 addition & 4 deletions ui/apps/platform/cypress/integration/accessControl/accessControlPermissionSets.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,10 @@ describe('Access Control Permission sets', () => {
cy.wait('@GetMyPermissions');

cy.get(selectors.h1).should('have.text', h1);
cy.get(selectors.navLink).should('not.exist');

cy.get(selectors.h2).should('not.exist');

cy.get(selectors.alertTitle).should(
'contain', // not have.text because it contains "Info alert:" for screen reader
'You do not have permission to view Access Control'
'You do not have permission to view permission sets.'
);
});

Expand Down
5 changes: 1 addition & 4 deletions ui/apps/platform/cypress/integration/accessControl/accessControlRoles.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,10 @@ describe('Access Control Roles', () => {
cy.wait('@GetMyPermissions');

cy.get(selectors.h1).should('have.text', h1);
cy.get(selectors.navLink).should('not.exist');

cy.get(selectors.h2).should('not.exist');

cy.get(selectors.alertTitle).should(
'contain', // not have.text because it contains "Info alert:" for screen reader
'You do not have permission to view Access Control'
'You do not have permission to view roles.'
);
});

Expand Down
60 changes: 24 additions & 36 deletions ui/apps/platform/src/Containers/AccessControl/AccessControl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import React, { ReactElement } from 'react';
import { Redirect, Route, Switch } from 'react-router-dom';

import usePermissions from 'hooks/usePermissions';

import { Alert, List, ListItem } from '@patternfly/react-core';
import { accessControlBasePath, accessControlPath, getEntityPath } from './accessControlPaths';

import AccessControlNoPermission from './AccessControlNoPermission';
import AccessControlRouteNotFound from './AccessControlRouteNotFound';
import AccessScopes from './AccessScopes/AccessScopes';
import AuthProviders from './AuthProviders/AuthProviders';
Expand All @@ -16,11 +13,6 @@ import Roles from './Roles/Roles';
const paramId = ':entityId?';

function AccessControl(): ReactElement {
// TODO is read access required for all routes in improved Access Control?
// TODO Is write access required anywhere in classic Access Control?
const { hasReadAccess } = usePermissions();
const hasReadAccessForAccessControlPages = hasReadAccess('Access');

return (
<>
<Alert
Expand Down Expand Up @@ -73,34 +65,30 @@ function AccessControl(): ReactElement {
</>
}
/>
{hasReadAccessForAccessControlPages ? (
Copy link
Copy Markdown
Member

@rukletsov rukletsov Nov 7, 2022

Choose a reason for hiding this comment

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

I would have probably kept the check extending it to hasReadAccess('Access') || hasReadAccess('Role')

Copy link
Copy Markdown
Contributor

@pedrottimark pedrottimark Nov 7, 2022

Choose a reason for hiding this comment

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

In theory

  • We could have kept this logic by replacing Access with Role (if it is true that all 4 entities need Role)
  • We could have added only extra Access dependency for Auth providers page.

But if future improvement in resources means that Access is the only dependency for all 4 entities, then we will move resource dependency from container code to declarative description of routes. Therefore,

  • Deletions from this page now mean no more changes required here in the future.
  • Additions to other pages will need to be deleted in the future.

Copy link
Copy Markdown
Contributor Author

@ivan-degtiarenko ivan-degtiarenko Nov 7, 2022

Choose a reason for hiding this comment

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

I think currently(meaning until after Access is fused with Role+with changes in this PR) we get reasonable behaviour when giving user Access but no Role permission:

  1. Auth providers are visible
  2. Minimum access role name is visible but impossible to access
  3. It's impossible for user to create either groups or change minimum access role.

<Switch>
<Route exact path={accessControlBasePath}>
<Redirect to={getEntityPath('AUTH_PROVIDER')} />
</Route>
<Route path={accessControlPath}>
<Switch>
<Route path={getEntityPath('AUTH_PROVIDER', paramId)}>
<AuthProviders />
</Route>
<Route path={getEntityPath('ROLE', paramId)}>
<Roles />
</Route>
<Route path={getEntityPath('PERMISSION_SET', paramId)}>
<PermissionSets />
</Route>
<Route path={getEntityPath('ACCESS_SCOPE', paramId)}>
<AccessScopes />
</Route>
<Route>
<AccessControlRouteNotFound />
</Route>
</Switch>
</Route>
</Switch>
) : (
<AccessControlNoPermission />
)}
<Switch>
<Route exact path={accessControlBasePath}>
<Redirect to={getEntityPath('AUTH_PROVIDER')} />
</Route>
<Route path={accessControlPath}>
<Switch>
<Route path={getEntityPath('AUTH_PROVIDER', paramId)}>
<AuthProviders />
</Route>
<Route path={getEntityPath('ROLE', paramId)}>
<Roles />
</Route>
<Route path={getEntityPath('PERMISSION_SET', paramId)}>
<PermissionSets />
</Route>
<Route path={getEntityPath('ACCESS_SCOPE', paramId)}>
<AccessScopes />
</Route>
<Route>
<AccessControlRouteNotFound />
</Route>
</Switch>
</Route>
</Switch>
</>
);
}
Expand Down
17 changes: 14 additions & 3 deletions ui/apps/platform/src/Containers/AccessControl/AccessControlNoPermission.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,26 @@ import React, { ReactElement } from 'react';
import { Alert, AlertVariant, PageSection, PageSectionVariants } from '@patternfly/react-core';

import AccessControlHeading from './AccessControlHeading';
import { AccessControlEntityType } from '../../constants/entityTypes';

function AccessControlNoPermission(): ReactElement {
type AccessControlNoPermissionProps = {
subPage: string;
entityType?: AccessControlEntityType;
isNavHidden?: boolean;
};

function AccessControlNoPermission({
subPage,
entityType,
isNavHidden = false,
}: AccessControlNoPermissionProps): ReactElement {
return (
<>
<AccessControlHeading isNavHidden />
<AccessControlHeading isNavHidden={isNavHidden} entityType={entityType} />
<PageSection variant={PageSectionVariants.light}>
<Alert
className="pf-u-mt-md"
title="You do not have permission to view Access Control"
title={`You do not have permission to view ${subPage}. To access this page, you should have access to both Role and Access resources.`}
variant={AlertVariant.info}
isInline
/>
Expand Down
5 changes: 4 additions & 1 deletion ui/apps/platform/src/Containers/AccessControl/AccessScopes/AccessScopeFormWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
getIsValidRules,
} from './accessScopes.utils';
import AccessScopeForm from './AccessScopeForm';
import usePermissions from '../../../hooks/usePermissions';

export type AccessScopeFormWrapperProps = {
isActionable: boolean;
Expand All @@ -45,6 +46,8 @@ function AccessScopeFormWrapper({
}: AccessScopeFormWrapperProps): ReactElement {
const [isSubmitting, setIsSubmitting] = useState(false);
const [alertSubmit, setAlertSubmit] = useState<ReactElement | null>(null);
const { hasReadWriteAccess } = usePermissions();
const hasWriteAccessForPage = hasReadWriteAccess('Role') && hasReadWriteAccess('Access');
Copy link
Copy Markdown
Contributor

@pedrottimark pedrottimark Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not a blocker to merge, but for my understanding: does access scope require 'Access' resource?

The following function calls which require 'Role' correct?

  • fetchAccessScopes
  • fetchRolesAsArray

Copy link
Copy Markdown
Contributor Author

@ivan-degtiarenko ivan-degtiarenko Nov 14, 2022

Choose a reason for hiding this comment

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

@pedrottimark that's our agreement with @rukletsov(if I got it right): bring Access and Role permission in the UI closer - if one is required at Read/Write level , then the other is also treated as required at Read/Write level


// Disable Save button while editing label selectors.
const [labelSelectorsEditingState, setLabelSelectorsEditingState] =
Expand Down Expand Up @@ -127,7 +130,7 @@ function AccessScopeFormWrapper({
<Button
variant="primary"
onClick={handleEdit}
isDisabled={action === 'edit'}
isDisabled={!hasWriteAccessForPage || action === 'edit'}
isSmall
>
Edit access scope
Expand Down
20 changes: 19 additions & 1 deletion ui/apps/platform/src/Containers/AccessControl/AccessScopes/AccessScopes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,15 @@ import './AccessScopes.css';
import AccessControlHeading from '../AccessControlHeading';
import AccessControlBreadcrumbs from '../AccessControlBreadcrumbs';
import AccessControlHeaderActionBar from '../AccessControlHeaderActionBar';
import AccessControlNoPermission from '../AccessControlNoPermission';
import usePermissions from '../../../hooks/usePermissions';

const entityType = 'ACCESS_SCOPE';

function AccessScopes(): ReactElement {
const { hasReadAccess, hasReadWriteAccess } = usePermissions();
const hasReadAccessToPage = hasReadAccess('Role') && hasReadAccess('Access');
const hasWriteAccessForPage = hasReadWriteAccess('Role') && hasReadWriteAccess('Access');
const history = useHistory();
const { search } = useLocation();
const queryObject = getQueryObject(search);
Expand Down Expand Up @@ -101,6 +106,15 @@ function AccessScopes(): ReactElement {
});
}, []);

// Return "no access" page immediately if user doesn't have enough permissions.
if (!hasReadAccessToPage) {
return (
<>
<AccessControlNoPermission subPage="access scopes" entityType={entityType} />
</>
);
}

function handleCreate() {
history.push(getEntityPath(entityType, undefined, { action: 'create' }));
}
Expand Down Expand Up @@ -163,7 +177,11 @@ function AccessScopes(): ReactElement {
</AccessControlDescription>
}
actionComponent={
<Button variant="primary" onClick={handleCreate}>
<Button
isDisabled={!hasWriteAccessForPage}
variant="primary"
onClick={handleCreate}
>
Create access scope
</Button>
}
Expand Down
4 changes: 4 additions & 0 deletions ui/apps/platform/src/Containers/AccessControl/AccessScopes/AccessScopesList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { AccessScope, getIsDefaultAccessScopeId } from 'services/AccessScopesSer
import { Role } from 'services/RolesService';

import { AccessControlEntityLink, RolesLink } from '../AccessControlLinks';
import usePermissions from '../../../hooks/usePermissions';

const entityType = 'ACCESS_SCOPE';

Expand All @@ -32,6 +33,8 @@ function AccessScopesList({
const [idDeleting, setIdDeleting] = useState('');
const [nameConfirmingDelete, setNameConfirmingDelete] = useState<string | null>(null);
const [alertDelete, setAlertDelete] = useState<ReactElement | null>(null);
const { hasReadWriteAccess } = usePermissions();
const hasWriteAccessForPage = hasReadWriteAccess('Role') && hasReadWriteAccess('Access');

function onClickDelete(id: string) {
setIdDeleting(id);
Expand Down Expand Up @@ -101,6 +104,7 @@ function AccessScopesList({
<Td
actions={{
disable:
!hasWriteAccessForPage ||
idDeleting === id ||
getIsDefaultAccessScopeId(id) ||
roles.some(({ accessScopeId }) => accessScopeId === id),
Expand Down
38 changes: 24 additions & 14 deletions ui/apps/platform/src/Containers/AccessControl/AuthProviders/AuthProviders.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable no-nested-ternary */
import React, { ReactElement, useEffect, useState } from 'react';
import { useSelector, useDispatch } from 'react-redux';
import { useDispatch, useSelector } from 'react-redux';
import { createStructuredSelector } from 'reselect';
import { selectors } from 'reducers';
import { useHistory, useLocation, useParams } from 'react-router-dom';
Expand All @@ -21,11 +21,7 @@ import EmptyStateTemplate from 'Components/PatternFly/EmptyStateTemplate';
import NotFoundMessage from 'Components/NotFoundMessage';
import { actions as authActions, types as authActionTypes } from 'reducers/auth';
import { actions as groupActions } from 'reducers/groups';
import {
actions as roleActions,
types as roleActionTypes,
getHasReadWritePermission,
} from 'reducers/roles';
import { actions as roleActions, types as roleActionTypes } from 'reducers/roles';
import { AuthProvider } from 'services/AuthService';

import { getEntityPath, getQueryObject } from '../accessControlPaths';
Expand All @@ -39,6 +35,8 @@ import AuthProvidersList from './AuthProvidersList';
import AccessControlBreadcrumbs from '../AccessControlBreadcrumbs';
import AccessControlHeading from '../AccessControlHeading';
import AccessControlHeaderActionBar from '../AccessControlHeaderActionBar';
import usePermissions from '../../../hooks/usePermissions';
import AccessControlNoPermission from '../AccessControlNoPermission';

const entityType = 'AUTH_PROVIDER';

Expand All @@ -65,6 +63,9 @@ function getNewAuthProviderObj(type) {
}

function AuthProviders(): ReactElement {
const { hasReadAccess, hasReadWriteAccess } = usePermissions();
const hasReadAccessForPage = hasReadAccess('Role') && hasReadAccess('Access');
const hasWriteAccessForPage = hasReadWriteAccess('Role') && hasReadWriteAccess('Access');
const history = useHistory();
const { search } = useLocation();
const queryObject = getQueryObject(search);
Expand All @@ -78,18 +79,27 @@ function AuthProviders(): ReactElement {
groups,
isFetchingAuthProviders,
isFetchingRoles,
userRolePermissions,
availableProviderTypes,
} = useSelector(authProviderState);
const hasWriteAccess = getHasReadWritePermission('Access', userRolePermissions);

const authProvidersWithRules = mergeGroupsWithAuthProviders(authProviders, groups);

useEffect(() => {
dispatch(authActions.fetchAuthProviders.request());
dispatch(roleActions.fetchRoles.request());
dispatch(groupActions.fetchGroups.request());
}, [dispatch]);
if (hasReadAccessForPage) {
dispatch(authActions.fetchAuthProviders.request());
dispatch(roleActions.fetchRoles.request());
dispatch(groupActions.fetchGroups.request());
}
}, [dispatch, hasReadAccessForPage]);

// Return "no access" page immediately if user doesn't have enough permissions.
if (!hasReadAccessForPage) {
return (
<>
<AccessControlNoPermission subPage="auth providers" entityType={entityType} />
</>
);
}

function onToggleCreateMenu(isOpen) {
setIsCreateMenuOpen(isOpen);
Expand Down Expand Up @@ -152,7 +162,7 @@ function AuthProviders(): ReactElement {
</AccessControlDescription>
}
actionComponent={
hasWriteAccess && (
hasWriteAccessForPage && (
<Dropdown
className="auth-provider-dropdown"
onSelect={onClickCreate}
Expand Down Expand Up @@ -214,7 +224,7 @@ function AuthProviders(): ReactElement {
/>
) : (
<AuthProviderForm
isActionable={hasWriteAccess}
isActionable={hasWriteAccessForPage}
action={action}
selectedAuthProvider={selectedAuthProvider ?? getNewAuthProviderObj(type)}
onClickCancel={onClickCancel}
Expand Down
5 changes: 4 additions & 1 deletion ui/apps/platform/src/Containers/AccessControl/PermissionSets/PermissionSetForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { PermissionSet } from 'services/RolesService';
import { AccessControlQueryAction } from '../accessControlPaths';

import PermissionsTable from './PermissionsTable';
import usePermissions from '../../../hooks/usePermissions';

export type PermissionSetFormProps = {
isActionable: boolean;
Expand All @@ -45,6 +46,8 @@ function PermissionSetForm({
}: PermissionSetFormProps): ReactElement {
const [isSubmitting, setIsSubmitting] = useState(false);
const [alertSubmit, setAlertSubmit] = useState<ReactElement | null>(null);
const { hasReadWriteAccess } = usePermissions();
const hasWriteAccessForPage = hasReadWriteAccess('Role') && hasReadWriteAccess('Access');

const { dirty, errors, handleChange, isValid, resetForm, setFieldValue, values } = useFormik({
initialValues: permissionSet,
Expand Down Expand Up @@ -128,7 +131,7 @@ function PermissionSetForm({
<Button
variant="primary"
onClick={handleEdit}
isDisabled={action === 'edit'}
isDisabled={!hasWriteAccessForPage || action === 'edit'}
isSmall
>
Edit permission set
Expand Down
Loading