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

Restrict user management capabilities when using the viewer role #2933

Merged
merged 10 commits into from
Mar 29, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Mar 28, 2023

relies on a fidesui change. need https://github.com/ethyca/fidesui/pull/46 to go in first, then update the import here, before merging

Closes https://github.com/ethyca/fidesplus/issues/700

Code Changes

  • Fix bug in password validation which was not allowing users to edit their names (save button wouldn't be enabled)
  • Disable permissions tab for viewers
  • Restrict the user management row so you can only click into your own profile if you are a viewer. The menu options do not show up either
  • Redirect a viewer who is trying to sneakily navigate directly to a user profile
  • Cypress tests

Steps to Confirm

  • Create a user with the viewer role
  • Go to the user-management page and click around. You should be able to click into your own profile, but not anyone else's. You also can't click into your permissions tab

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

@allisonking allisonking added the do not merge Please don't merge yet, bad things will happen if you do label Mar 28, 2023
@cypress
Copy link

cypress bot commented Mar 28, 2023

Passing run #1071 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 3f15b7a into 22349a8...
Project: fides Commit: d15c9cdca3 ℹ️
Status: Passed Duration: 00:37 💡
Started: Mar 29, 2023 3:04 PM Ended: Mar 29, 2023 3:04 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@allisonking
Copy link
Contributor Author

It appears the backend does not allow editing a user's name if they don't have the USER UPDATE permission (even if it's their own user)

The docstring here is not quite right

https://github.com/ethyca/fides/blob/aking/fidesplus-700/restrict-viewer-user-management/src/fides/api/ops/api/v1/endpoints/user_endpoints.py/#L81-L105

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

backend looks reasonable Allison! didn't review the FE

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (22349a8) 86.66% compared to head (3f15b7a) 86.66%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2933   +/-   ##
=======================================
  Coverage   86.66%   86.66%           
=======================================
  Files         300      300           
  Lines       16909    16912    +3     
  Branches     2157     2158    +1     
=======================================
+ Hits        14654    14657    +3     
  Misses       1844     1844           
  Partials      411      411           
Impacted Files Coverage Δ
...c/fides/api/ops/api/v1/endpoints/user_endpoints.py 95.20% <100.00%> (+0.11%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@allisonking allisonking removed the do not merge Please don't merge yet, bad things will happen if you do label Mar 29, 2023
@allisonking allisonking marked this pull request as ready for review March 29, 2023 14:50
@eastandwestwind
Copy link
Contributor

@allisonking I'm testing this out locally, and as a Viewer, I'm unable to view the user management nav item. Is this expected?

Screenshot 2023-03-29 at 6 32 00 PM

@eastandwestwind
Copy link
Contributor

I'm unsure if this was introduced here or has always been an issue, but attempting to update password where the old password field is incorrect leads to no err message in the UI telling the user about this error. There is instead a console err:

Screenshot 2023-03-29 at 6 47 15 PM

Otherwise, I've tested the happy path here as a viewer, and everything else looks and functions great!

@allisonking
Copy link
Contributor Author

Thanks @eastandwestwind ! Looks like that's a bug independent of this PR, so I'll log an issue for it and keep this PR trucking along 🚚

issue here: #2942

@allisonking allisonking merged commit 9d0c1fe into main Mar 29, 2023
@allisonking allisonking deleted the aking/fidesplus-700/restrict-viewer-user-management branch March 29, 2023 17:02
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.

3 participants