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

[Backend] Make New Users Viewers By Default [#2866] #2900

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Mar 22, 2023

Closes #2866

Code Changes

  • When creating a brand new FidesUser, adjust the FidesUserPermissions object created in the background so it is given a VIEWER role by default.
  • Update both POST and PUT {{host}}/user/{{user_id}}/permission endpoints to require an explicit "roles" key in the request to be intentional. An empty list value will remove the user's roles. (This doesn't have a practical effect in the UI - as it always has the roles key, and doesn't surface the option to remove roles entirely, but still technically allowing this in the backend). Also note that the UI does not hit the POST user permissions endpoint at all since create user adds the initial permissions resource already.

Steps to Confirm

  • As the root user, login and navigate to user management http://localhost:3000/user-management
  • Click "Add New User"
  • Fill out username, firstname, last name, pw. Click Save
  • Verify in the network tab that the followup request to the UI to fetch the user's permissions returns the viewer role that was automatically added by the b/e http://0.0.0.0:8080/api/v1/user/<user_id>/permission
  • Click on the Permissions tab. Verify the Viewer tab is already selected as they are technically a Viewer

Pre-Merge Checklist

Description Of Changes

Give new users a Viewer role by default. Also require that the get/create permissions endpoints specify a role key in the request (this is new behavior, it was previously given an empty list by default), and assert that an empty list will remove the users roles (this was existing behavior, just adding tests)

… created in the background so it is given a VIEWER role by default.

- The separate endpoints for creating and editing user permissions, require that a "roles" key is in the request.  If that "roles" value is an empty list, that effectively adjusts the user to have no roles. For the permissions endpoints intentionally allow the user to set no roles if they want to.
@pattisdr pattisdr changed the title Make New Users Viewers By Default [Backend] Make New Users Viewers By Default [#2866] Mar 22, 2023
@pattisdr pattisdr marked this pull request as ready for review March 22, 2023 19:16
@cypress
Copy link

cypress bot commented Mar 22, 2023

Passing run #977 ↗︎

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 d10bb36 into dfabd1d...
Project: fides Commit: 795f92aee7 ℹ️
Status: Passed Duration: 00:37 💡
Started: Mar 24, 2023 2:17 AM Ended: Mar 24, 2023 2:17 AM

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

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (dfabd1d) 86.60% compared to head (d10bb36) 86.60%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2900   +/-   ##
=======================================
  Coverage   86.60%   86.60%           
=======================================
  Files         299      299           
  Lines       16814    16815    +1     
  Branches     2148     2148           
=======================================
+ Hits        14562    14563    +1     
  Misses       1841     1841           
  Partials      411      411           
Impacted Files Coverage Δ
src/fides/api/ops/schemas/user_permission.py 100.00% <100.00%> (ø)
src/fides/lib/oauth/api/routes/user_endpoints.py 95.40% <100.00%> (+0.05%) ⬆️

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.

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

ah, I'm relieved the UI picked up on the change without any problems 😌 thanks for the update!!

@pattisdr
Copy link
Contributor Author

Really appreciate all the review you've been doing for me the last couple days @allisonking 🏆

@pattisdr
Copy link
Contributor Author

Argh changelog 😠

@pattisdr pattisdr merged commit 9082c94 into main Mar 24, 2023
@pattisdr pattisdr deleted the fides_2866_viewer_role_by_default branch March 24, 2023 13:55
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.

New users are not assigned a default role
2 participants