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

BHBC-2300: Apply 'Viewer' and 'Editor' Permissions to Project / Survey Views #1000

Merged
merged 39 commits into from
Apr 19, 2023

Conversation

curtisupshall
Copy link
Contributor

@curtisupshall curtisupshall commented Apr 12, 2023

Overview

As a Project Lead, I want to make sure that people I’ve invited to my team can only see the functions their role allows them to so they aren’t getting confused as to why some functionality appears broken.

Links to Jira tickets

Description of relevant changes

  • Created an API method to get the user's current roles for the given project
  • Created a Project Auth State context to determine the auth state for project and survey views
  • Created a component guard to hide UI elements based on accepted project roles
  • Created a route guard to protect app routes based on accepted project roles. Current protected routes include:
    • Project Manage Users page
    • Edit Project page
    • Create Survey page
    • Edit Survey page

Testing Procedures

  1. Create a BCeID Basic in the BCeID Dev environment. You can do this here: https://www.development.bceid.ca/register/.
  2. From your IDIR account, add your BCeID user to a project as a ‘Viewer’.
  3. Go to Manage Users. Ensure that your newly added BCeID user does not have admin permissions by changing their role to Creator.
  4. Open a new Incognito (Chrome)/Private Browsing (Firefox)/InPrivate (Edge) window (Ctrl + Shift + N). Sign into SIMS as your BCeID user.
  5. View the project as the 'Viewer' user. It should:
  • NOT show the following interactive components:
    • Add Survey
    • Upload Documents
    • Delete Document (see individual document row items)
    • Edit Location
  • NOT be able to click Project Settings
  1. From the project view, change your browser's URL from /details to /edit. It should:
  • Show the Forbidden page.
  1. From the project view, change your browser's URL from /details to /users. It should:
  • Show the Forbidden page.
  1. Navigate to a survey on the same project. It should:
  • NOT show the following interactive components:
    • Import Observations
    • Delete Observations
    • Import Summary Results
    • Delete Summary Results
    • Upload Documents
    • Delete Document (see individual document row items)
    • Edit Study Area
  • NOT allow you to click Survey Settings
  1. From the survey view, change your browser's URL from /details to /edit. It should:
  • Show the Forbidden page.
  1. From the survey view, change your browser's URL from /{surveyId}/details to /create. It should:
  • Show the Forbidden page.
  1. Change the project role of your BCeID user to be an ‘Editor’.
  2. View the project as the 'Editor' user. It should:
  • NOT show the following project settings options:
    • Delete Project
    • Manage Project Team
  1. From the project view, change your browser's URL from /details to /users. It should:
  • Show the Forbidden page.

@NickPhura
Copy link
Collaborator

NickPhura commented Apr 12, 2023

@curtisupshall Just skimmed what you have here, looks pretty good so far. A few thoughts, semi jumbled.

It might be ideal to turn that participant guard context into something more generic, like projectAuthStateContext (mirroring the existing authStateContext). It could then wrap the project routes, similar to the project context. Then we can leverage it however we want in the children (could even look into project route guards which we don't have atm). We could also look into a specific endpoint for fetching the project user info for that specific projectId, in case the user is on a ton of projects.

(I think the only funny project route atm is the edit one, as it doesn't fall under the :id path, but probably should. And then if we do build a project route guard, it could be protected the same way).

Overall I think its similar to the work you did with the project/survey contexts really and fixing the routers a bit in the process, etc.

@curtisupshall
Copy link
Contributor Author

curtisupshall commented Apr 12, 2023

@NickPhura

It might be ideal to turn that participant guard context into something more generic, like projectAuthStateContext (mirroring the existing authStateContext). It could then wrap the project routes, similar to the project context.

Sounds like a good approach!

Hmm, now I wonder if projectParticipantStateContext would be a better candidate... 🤔 Beyond checking the participant record, is there any metric not covered by keycloakWrapper by which we could authenticate for a project? Conversely, a project participant context could be used for authentication in addition to other project role functionallity

We could also look into a specific endpoint for fetching the project user info for that specific projectId, in case the user is on a ton of projects.

This was my initial thinking; I wrote the project/{projectId}/participants/self.ts endpoint to get the participant info for the user.

Overall I think its similar to the work you did with the project/survey contexts really and fixing the routers a bit in the process, etc.

Yup, agreed.

@NickPhura
Copy link
Collaborator

@NickPhura

It might be ideal to turn that participant guard context into something more generic, like projectAuthStateContext (mirroring the existing authStateContext). It could then wrap the project routes, similar to the project context.

Sounds like a good approach!

Hmm, now I wonder if projectParticipantStateContext would be a better candidate... 🤔 Beyond checking the participant record, is there any metric not covered by keycloakWrapper by which we could authenticate for a project? Conversely, a project participant context could be used for authentication in addition to other project role functionallity

We could also look into a specific endpoint for fetching the project user info for that specific projectId, in case the user is on a ton of projects.

This was my initial thinking; I wrote the project/{projectId}/participants/self.ts endpoint to get the participant info for the user.

Overall I think its similar to the work you did with the project/survey contexts really and fixing the routers a bit in the process, etc.

Yup, agreed.

Lets chat in Discord. I suspect we are on the same track tbh and its mostly just a semantics thing.

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #1000 (7fb449c) into dev (0327100) will decrease coverage by 0.17%.
The diff coverage is 40.12%.

@@            Coverage Diff             @@
##              dev    #1000      +/-   ##
==========================================
- Coverage   68.06%   67.90%   -0.17%     
==========================================
  Files         378      380       +2     
  Lines       11727    11821      +94     
  Branches     1990     2006      +16     
==========================================
+ Hits         7982     8027      +45     
- Misses       3297     3343      +46     
- Partials      448      451       +3     
Impacted Files Coverage Δ
...c/repositories/project-participation-repository.ts 83.33% <ø> (ø)
api/src/services/project-participation-service.ts 66.66% <ø> (ø)
api/src/services/project-service.ts 27.27% <ø> (ø)
app/src/components/security/RouteGuards.tsx 0.00% <0.00%> (ø)
app/src/contexts/surveyContext.tsx 7.14% <0.00%> (-1.56%) ⬇️
app/src/features/admin/AdminUsersRouter.tsx 0.00% <ø> (ø)
app/src/features/projects/ProjectsRouter.tsx 0.00% <ø> (ø)
app/src/features/projects/edit/EditProjectPage.tsx 0.00% <0.00%> (ø)
...rojects/participants/ProjectParticipantsHeader.tsx 0.00% <ø> (ø)
app/src/features/projects/view/ProjectHeader.tsx 5.26% <0.00%> (+0.38%) ⬆️
... and 22 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AlfredRosenthal
Copy link
Contributor

Might be out of scope but running your branch I see this when I try to manage users:
Screenshot 2023-04-13 124926

@curtisupshall
Copy link
Contributor Author

Might be out of scope but running your branch I see this when I try to manage users: Screenshot 2023-04-13 124926

Fixed by #999.

@curtisupshall curtisupshall added the Not Ready For Review Addressing feedback and/or refactoring label Apr 13, 2023
@curtisupshall curtisupshall changed the title BHBC-2300: Apply 'Viewer' and 'Editor' Permissions to Project / Survey Views #974 BHBC-2300: Apply 'Viewer' and 'Editor' Permissions to Project / Survey Views Apr 13, 2023
@curtisupshall curtisupshall added the Early Feedback Welcome PR is not finished, but early review feedback is welcomed label Apr 14, 2023
@curtisupshall curtisupshall added the Early Feedback Welcome PR is not finished, but early review feedback is welcomed label Apr 17, 2023
@KjartanE KjartanE self-assigned this Apr 17, 2023
@curtisupshall curtisupshall removed Do Not Merge PR should not be merged Early Feedback Welcome PR is not finished, but early review feedback is welcomed labels Apr 17, 2023
NickPhura
NickPhura previously approved these changes Apr 17, 2023
@curtisupshall curtisupshall requested a review from NickPhura April 17, 2023 23:27
NickPhura
NickPhura previously approved these changes Apr 17, 2023
Copy link
Contributor

@KjartanE KjartanE left a comment

Choose a reason for hiding this comment

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

image

\app\src\features\surveys\view\survey-observations\components\NoObservationsCard.tsx
\app\src\features\surveys\view\summary-results\components\NoSummaryResults.tsx

THese import button are still available

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.2% 2.2% Duplication

Copy link
Contributor

@KjartanE KjartanE left a comment

Choose a reason for hiding this comment

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

Functionally works well, no issues noticed. 🐈

@curtisupshall curtisupshall merged commit f54deb6 into dev Apr 19, 2023
@curtisupshall curtisupshall deleted the BHBC-2300 branch April 19, 2023 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants