From fe177c6ab3d696e3bc986b57410510867ede9dbf Mon Sep 17 00:00:00 2001 From: jtimpe <111305129+jtimpe@users.noreply.github.com> Date: Tue, 22 Nov 2022 15:02:36 -0500 Subject: [PATCH] Fix/1744 permission guards (#2258) * fix role selector * permission guard component to conditionally render based on redux state * use permission guard for data file page * use permission guard for data file nav item * add component tests for data file permission guard * require account approval to access data files * update ci? * fix ci img * ci img * rm old heroku keys workaround Co-authored-by: Andrew <84722778+andrew-jameson@users.noreply.github.com> --- .../src/components/Header/Header.jsx | 15 +- .../src/components/Header/Header.test.js | 74 +++++++ .../PermissionGuard/PermissionGuard.jsx | 47 +++++ .../PermissionGuard/PermissionGuard.test.js | 194 ++++++++++++++++++ .../src/components/PermissionGuard/index.js | 3 + .../components/PrivateRoute/PrivateRoute.js | 33 ++- .../PrivateRoute/PrivateRoute.test.js | 28 ++- tdrs-frontend/src/components/Routes/Routes.js | 6 +- tdrs-frontend/src/selectors/auth.js | 4 +- 9 files changed, 381 insertions(+), 23 deletions(-) create mode 100644 tdrs-frontend/src/components/PermissionGuard/PermissionGuard.jsx create mode 100644 tdrs-frontend/src/components/PermissionGuard/PermissionGuard.test.js create mode 100644 tdrs-frontend/src/components/PermissionGuard/index.js diff --git a/tdrs-frontend/src/components/Header/Header.jsx b/tdrs-frontend/src/components/Header/Header.jsx index c97646e2f..60158504d 100644 --- a/tdrs-frontend/src/components/Header/Header.jsx +++ b/tdrs-frontend/src/components/Header/Header.jsx @@ -10,6 +10,7 @@ import { } from '../../selectors/auth' import NavItem from '../NavItem/NavItem' +import PermissionGuard from '../PermissionGuard' /** * This component is rendered on every page and contains the navigation bar. @@ -28,13 +29,6 @@ function Header() { const userAccessRequestPending = useSelector(accountIsInReview) const userAccessRequestApproved = useSelector(accountStatusIsApproved) - const hasPermission = (permissionName) => - user?.roles?.[0]?.permissions?.some( - (perm) => perm.codename === permissionName - ) - - const canViewDataFiles = hasPermission('view_datafile') - const menuRef = useRef() const keyListenersMap = useMemo(() => { @@ -118,13 +112,16 @@ function Header() { {authenticated && ( <> - {canViewDataFiles && ( + - )} + {(userAccessRequestPending || userAccessRequestApproved) && ( { expect(queryByText('Profile')).not.toBeInTheDocument() expect(queryByText('Admin')).not.toBeInTheDocument() }) + + it('should NOT show data-files nav item when the user does not have view_datafile and add_datafile permissions', () => { + const state = { + ...initialState, + auth: { + user: { + email: 'test@test.com', + roles: [{ id: 1, name: 'Developer', permissions: [] }], + access_request: true, + account_approval_status: 'Approved', + }, + authenticated: true, + }, + } + + const store = mockStore(state) + + const { queryByText } = render( + +
+ + ) + + expect(queryByText('Data Files')).not.toBeInTheDocument() + expect(queryByText('Profile')).toBeInTheDocument() + expect(queryByText('Admin')).toBeInTheDocument() + }) + + it('should NOT show data-files nav item when the user is not in an approved status', () => { + const state = { + ...initialState, + auth: { + user: { + email: 'test@test.com', + roles: [ + { + id: 1, + name: 'Developer', + permissions: ['add_datafile', 'view_datafile'], + }, + ], + access_request: true, + account_approval_status: 'Pending', + }, + authenticated: true, + }, + } + + const store = mockStore(state) + + const { queryByText } = render( + +
+ + ) + + expect(queryByText('Data Files')).not.toBeInTheDocument() + expect(queryByText('Profile')).toBeInTheDocument() + expect(queryByText('Admin')).toBeInTheDocument() + }) + + it('should show data-files nav item when the user has view_datafile and add_datafile permissions and is approved', () => { + const store = mockStore(initialState) + + const { queryByText } = render( + +
+ + ) + + expect(queryByText('Data Files')).toBeInTheDocument() + expect(queryByText('Profile')).toBeInTheDocument() + expect(queryByText('Admin')).toBeInTheDocument() + }) }) diff --git a/tdrs-frontend/src/components/PermissionGuard/PermissionGuard.jsx b/tdrs-frontend/src/components/PermissionGuard/PermissionGuard.jsx new file mode 100644 index 000000000..e046f5400 --- /dev/null +++ b/tdrs-frontend/src/components/PermissionGuard/PermissionGuard.jsx @@ -0,0 +1,47 @@ +import { useSelector } from 'react-redux' +import { + accountStatusIsApproved, + selectUserPermissions, +} from '../../selectors/auth' + +const isAllowed = ( + { permissions, isApproved }, + requiredPermissions, + requiresApproval +) => { + if (requiresApproval && !isApproved) { + return false + } + + if (!requiredPermissions) { + return true + } + + for (var i = 0; i < requiredPermissions.length; i++) { + if (!permissions.includes(requiredPermissions[i])) { + return false + } + } + + return true +} + +const PermissionGuard = ({ + children, + requiresApproval = false, + requiredPermissions = [], + notAllowedComponent = null, +}) => { + const permissions = useSelector(selectUserPermissions) + const isApproved = useSelector(accountStatusIsApproved) + + return isAllowed( + { permissions, isApproved }, + requiredPermissions, + requiresApproval + ) + ? children + : notAllowedComponent +} + +export default PermissionGuard diff --git a/tdrs-frontend/src/components/PermissionGuard/PermissionGuard.test.js b/tdrs-frontend/src/components/PermissionGuard/PermissionGuard.test.js new file mode 100644 index 000000000..2e6ccf13f --- /dev/null +++ b/tdrs-frontend/src/components/PermissionGuard/PermissionGuard.test.js @@ -0,0 +1,194 @@ +/* eslint-disable no-param-reassign */ +import React from 'react' +import thunk from 'redux-thunk' +import { Provider } from 'react-redux' +import configureStore from 'redux-mock-store' +import { render, screen } from '@testing-library/react' + +import PermissionGuard from '.' + +describe('PermissionGuard.js', () => { + const mockStore = configureStore([thunk]) + + const setup = (initialState, requiredPermissions, requiresApproval = false) => + render( + + not allowed

} + > +

hello, world

+ + + ) + + describe('not allowed', () => { + it('shows not allowed if missing one permission', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [{ permissions: [{ codename: 'some_stuff' }] }], + }, + }, + }, + ['allowed'] + ) + + expect(screen.queryByText('hello, world')).not.toBeInTheDocument() + expect(screen.queryByText('not allowed')).toBeInTheDocument() + }) + + it('shows not allowed if missing multiple permissions', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [{ permissions: [{ codename: 'allowed' }] }], + }, + }, + }, + ['allowed', 'super_allowed', 'super_duper_allowed'] + ) + + expect(screen.queryByText('hello, world')).not.toBeInTheDocument() + expect(screen.queryByText('not allowed')).toBeInTheDocument() + }) + + it('shows not allowed if multiple roles missing permissions', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [ + { permissions: [{ codename: 'allowed' }] }, + { permissions: [{ codename: 'super_allowed' }] }, + { permissions: [{ codename: 'nothing' }] }, + ], + }, + }, + }, + ['allowed', 'super_allowed', 'super_duper_allowed'] + ) + + expect(screen.queryByText('hello, world')).not.toBeInTheDocument() + expect(screen.queryByText('not allowed')).toBeInTheDocument() + }) + + it('shows not allowed if user has no roles', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: null, + }, + }, + }, + ['anything'] + ) + + expect(screen.queryByText('hello, world')).not.toBeInTheDocument() + expect(screen.queryByText('not allowed')).toBeInTheDocument() + }) + + it('shows not allowed if user has no permissions', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [{ permissions: [] }], + }, + }, + }, + ['anything'] + ) + + expect(screen.queryByText('hello, world')).not.toBeInTheDocument() + expect(screen.queryByText('not allowed')).toBeInTheDocument() + }) + + it('shows not allowed if requiresApproval and not approved', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [{ permissions: ['anything'] }], + account_approval_status: 'Pending', + }, + }, + }, + [], + true + ) + + expect(screen.queryByText('hello, world')).not.toBeInTheDocument() + expect(screen.queryByText('not allowed')).toBeInTheDocument() + }) + }) + + describe('allowed', () => { + it('shows allowed if no required permissions given', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: null, + }, + }, + }, + null + ) + + expect(screen.queryByText('hello, world')).toBeInTheDocument() + expect(screen.queryByText('not allowed')).not.toBeInTheDocument() + }) + + it('shows allowed if user matches all permissions', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [ + { permissions: [{ codename: 'allowed' }] }, + { permissions: [{ codename: 'super_allowed' }] }, + { permissions: [{ codename: 'super_duper_allowed' }] }, + ], + }, + }, + }, + ['allowed', 'super_allowed', 'super_duper_allowed'] + ) + + expect(screen.queryByText('hello, world')).toBeInTheDocument() + expect(screen.queryByText('not allowed')).not.toBeInTheDocument() + }) + + it('shows allowed if requiresApproval and approved', () => { + setup( + { + auth: { + authenticated: true, + user: { + roles: [{ permissions: ['anything'] }], + account_approval_status: 'Approved', + }, + }, + }, + [], + true + ) + + expect(screen.queryByText('hello, world')).toBeInTheDocument() + expect(screen.queryByText('not allowed')).not.toBeInTheDocument() + }) + }) +}) diff --git a/tdrs-frontend/src/components/PermissionGuard/index.js b/tdrs-frontend/src/components/PermissionGuard/index.js new file mode 100644 index 000000000..9d2b9f3a8 --- /dev/null +++ b/tdrs-frontend/src/components/PermissionGuard/index.js @@ -0,0 +1,3 @@ +import PermissionGuard from './PermissionGuard' + +export default PermissionGuard diff --git a/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.js b/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.js index af1f7ef30..33df4eb64 100644 --- a/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.js +++ b/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.js @@ -1,10 +1,11 @@ import React, { useEffect } from 'react' -import { useNavigate } from 'react-router-dom' +import { Navigate, useNavigate } from 'react-router-dom' import { useSelector, useDispatch } from 'react-redux' import { setAlert, clearAlert } from '../../actions/alert' import { ALERT_INFO } from '../Alert' import PrivateTemplate from '../PrivateTemplate' import IdleTimer from '../IdleTimer/IdleTimer' +import PermissionGuard from '../PermissionGuard' /** * @param {React.ReactNode} children - One or more React components to be @@ -12,9 +13,15 @@ import IdleTimer from '../IdleTimer/IdleTimer' * @param {string} title Page title passed in to PrivateTemplate * which is automatically passed via withRouter */ -function PrivateRoute({ children, title }) { +function PrivateRoute({ + children, + title, + requiredPermissions, + requiresApproval, +}) { const authenticated = useSelector((state) => state.auth.authenticated) const authLoading = useSelector((state) => state.auth.loading) + const navigate = useNavigate() const dispatch = useDispatch() @@ -33,12 +40,22 @@ function PrivateRoute({ children, title }) { } }, [authenticated, authLoading, dispatch, navigate]) - return authenticated ? ( - - {children} - - - ) : null + if (authenticated) { + return ( + } + > + + {children} + + + + ) + } + + return null } export default PrivateRoute diff --git a/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.test.js b/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.test.js index 0eaa65839..72aed6537 100644 --- a/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.test.js +++ b/tdrs-frontend/src/components/PrivateRoute/PrivateRoute.test.js @@ -18,11 +18,12 @@ describe('PrivateRoute.js', () => { + hello, world

} /> +

Hello Private Content

 } @@ -37,8 +38,15 @@ describe('PrivateRoute.js', () => { expect(screen.queryByText('Hello Private Content')).not.toBeInTheDocument() }) - it('returns children when user is authenticated', () => { - createWrapper({ auth: { authenticated: true } }) + it('returns children when user is authenticated and has required permissions', () => { + createWrapper({ + auth: { + authenticated: true, + user: { + roles: [{ permissions: [{ codename: 'can_view' }] }], + }, + }, + }) expect(screen.queryByText('Hello Private Content')).toBeInTheDocument() }) @@ -52,4 +60,18 @@ describe('PrivateRoute.js', () => { }) spy.mockRestore() }) + + it('redirects to home when the user does not have the required permissions', () => { + createWrapper({ + auth: { + authenticated: true, + user: { + roles: [{ permissions: [{ codename: 'some_stuff' }] }], + }, + }, + }) + + expect(screen.queryByText('Hello Private Content')).not.toBeInTheDocument() + expect(screen.queryByText('hello, world')).toBeInTheDocument() + }) }) diff --git a/tdrs-frontend/src/components/Routes/Routes.js b/tdrs-frontend/src/components/Routes/Routes.js index c9e84b2c1..530777cf2 100644 --- a/tdrs-frontend/src/components/Routes/Routes.js +++ b/tdrs-frontend/src/components/Routes/Routes.js @@ -41,7 +41,11 @@ const AppRoutes = () => { exact path="/data-files" element={ - + } diff --git a/tdrs-frontend/src/selectors/auth.js b/tdrs-frontend/src/selectors/auth.js index 54ae37493..f2f9d5d0e 100644 --- a/tdrs-frontend/src/selectors/auth.js +++ b/tdrs-frontend/src/selectors/auth.js @@ -20,9 +20,9 @@ export const selectUserPermissions = (state) => { const roles = selectUserRoles(state) let permissions = [] roles.forEach((role) => { - permissions = [...permissions, role['permissions']] + permissions = [...permissions, ...role['permissions']] }) - return permissions + return permissions.map((p) => p.codename) } export const accountStatusIsInitial = (state) =>