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] Restrict Owner Role Creation [#2855] #2888

Merged
merged 6 commits into from
Mar 22, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Mar 21, 2023

Closes #2855

Code Changes

  • Add a new scope: USER_PERMISSION_ASSIGN_OWNERS, that is only granted to owners
  • Add an additional permissions check in the create and update permissions endpoints that you need this scope if you are making someone an owner.
    • Viewers, approvers, and viewer_and_approvers already could not do this because they don't have USER_PERMISSION_CREATE and USER_PERMISSION_UPDATE scopes

Steps to Confirm

  • Run nox -s "fides_env(test)"
  • Login as the root user
  • Create a user with an owner role
  • Login as owner. Create another owner and a contributor.
  • Login as the contributor. Create another contributor and a viewer.
  • Via the API, verify that contributor cannot update their own permission's or the viewer's permissions to owner:
PUT {{host}}/user/<user_id>/permission
{
    "roles": ["owner"]
}
  • Login as viewer. Verify the view can't update anyone's permissions (via the API)

Pre-Merge Checklist

Description Of Changes

This effectively prevents Contributors from creating Owners on the backend.

…PERMISSION_ASSIGN_OWNERS scope to just "owners". This effectively prevents contributors from making other users "owners".
@cypress
Copy link

cypress bot commented Mar 21, 2023

Passing run #928 ↗︎

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 bcb1eb8 into d6e3536...
Project: fides Commit: 7fcf67ebcc ℹ️
Status: Passed Duration: 00:36 💡
Started: Mar 22, 2023 5:56 PM Ended: Mar 22, 2023 5:56 PM

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

@pattisdr pattisdr marked this pull request as ready for review March 21, 2023 18:14
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@d6e3536). Click here to learn what that means.
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head b7c6421 differs from pull request most recent head bcb1eb8. Consider uploading reports for the commit bcb1eb8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2888   +/-   ##
=======================================
  Coverage        ?   86.62%           
=======================================
  Files           ?      298           
  Lines           ?    16791           
  Branches        ?     2148           
=======================================
  Hits            ?    14545           
  Misses          ?     1835           
  Partials        ?      411           
Impacted Files Coverage Δ
src/fides/lib/oauth/roles.py 100.00% <ø> (ø)
.../ops/api/v1/endpoints/user_permission_endpoints.py 96.42% <100.00%> (ø)
src/fides/api/ops/api/v1/scope_registry.py 100.00% <100.00%> (ø)

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.

@pattisdr pattisdr changed the title Restrict Owner Role Creation [Backend] Restrict Owner Role Creation Mar 21, 2023
@pattisdr pattisdr changed the title [Backend] Restrict Owner Role Creation [Backend] Restrict Owner Role Creation [#2855] Mar 21, 2023
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.

this is great! I made a corresponding FE ticket here #2896 which doesn't have to go in right away

@pattisdr
Copy link
Contributor Author

Thanks so much for the review @allisonking! I'll get this up to date with main, update the changelog and merge.

@pattisdr pattisdr merged commit 627340c into main Mar 22, 2023
@pattisdr pattisdr deleted the fides_2855_restrict_assigning_higher_perms branch March 22, 2023 18:07
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.

Restrict ability for a User to assign "higher" roles to any other User via the API
2 participants