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] Add Role-Based Permissions [#2606] #2671

Merged
merged 16 commits into from
Feb 27, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Feb 22, 2023

Closes #2606

Code Changes

  • Update the permission check in verify_oauth_client to see if you have the scope from one of two means: either a scope you were assigned directly OR if your role(s) are associated with that scope.
  • Add client.roles and fidesuserpermission.roles column. Default values are empty lists and scopes are changed likewise.
    • The FidesUserPermission.roles column persists the user's roles, while the client.roles column is temporary, for login.
  • Add a new endpoint GET /oauth/role to surface the possible roles and their associated scopes to which you can be assigned
  • Update POST /oauth/token to add an admin role to the the token if the client is the root client.
  • Update PATCH user permissions to be able to update both roles and/or scopes. Current UI can just update scopes, but we're moving to a UI that updates roles. Technically, we'll still support updating scopes in the backend.
  • Update GET user permissions to return both the scope registry and admin roles if this is the root user. Also we surface the roles the user has in addition to their direct scopes (current behavior)
  • Update CLI command "fides user permissions" to return both roles and directly-assigned scopes
  • Update CLI command "fides user create" which creates a user with full permissions to also have the admin role.
  • Add ExecutionSettings.root_user_roles
  • Fix the default values on Client.scopes and FidesUserPermission.scopes - this did not appear to be working as intended, but we weren't hitting the error in practice.
  • Try to ensure that if a client's roles or scopes are accessed, it returns an empty list instead of null if they don't exist.
  • Add first draft of role -> scope mapping.
  • Remove old, unused privileges code. We were once intending to group scopes into "privileges" but never finished that work. This replaces that effort.
  • On login, give the client both root_user_scopes and root_user_roles if it's the root user. Persist permission scopes and roles to the client if the client doesn't exist.
  • Prevent logging in without any roles or scopes.

Steps to Confirm

  • Create a user via the API with a client that has permissions to do so:
    POST {{host}}/user
{
    "username": "test_user",
    "password": "Testpassword1!"

}
  • Update the role of that user, one of admin, viewer_and_privacy_request_manager and viewer, privacy_request_manager:

PUT {{host}}/user/{{user_id}}/permission

{
    "id": "{user_id}",
    "roles": ["viewer"]
}
  • Login as that user in the admin UI
  • Navigate through various parts of the UI with different roles and observe in the network tab that some routes are restricted depending on your role. Note that which scopes go with which role are still in flux.

Pre-Merge Checklist

Description Of Changes

Optionally add the ability to grant larger roles directly to users instead of just individual scopes. Roles will be associated with a list of scopes.

Currently supporting 4 roles:

  • Admin role: has all scopes
  • Viewer + Privacy Request Manager: Has all read permissions (minus user permissions read) + Privacy Request Scopes
  • Viewer: Has read scopes (minus user permissions read)
  • Privacy Request Manager: Only privacy request scopes

…f just scopes. Roles will be associated with a list of scopes.

 - Update the permission check in verify_oauth_client to see if you have the scope from one of two means: either a scope you were assigned directly OR if  your role(s) are associated with that scope.
- Add client.roles and fidesuserpermission.roles column. Make FidesUserPermissions.scopes and client.scopes nullable, as you can assign roles or scopes.
- Add a new endpoint /oauth/role to get the possible roles and their associated scopes to which you can be assigned
- Update POST /oauth/token to add an admin role if the client is the root client.
- Update Patch user permissions to be able to update both roles and scopes
- Update get user permissions to return both the scope registry and admin roles if this is the root user.
- Update CLI command "fides user permissions" to return both roles and directly-assigned scopes
- Update CLI command "fides user create" which creates a user with full permissions to also have the admin role.
- Add ExecutionSettings.root_user_roles
- Fix the default values on Client.scopes and FidesUserPermission.scopes - this did not appear to be working as intended
- Try to ensure that if a client's roles or scopes are accessed, it returns an empty list instead of null if they don't exist.
- Add first draft of role -> scope mapping.
- Remove old, unused privileges code.  We were once intending to group scopes into "privileges" but never finished that work.  This replaces that effort.
- On login, give the client both root_user_scopes and root_user_roles if it's the root user. Persist permission scopes and roles to the client if the client doesn't exist.
# Conflicts:
#	src/fides/core/user.py
#	src/fides/lib/models/fides_user_permissions.py
#	src/fides/lib/oauth/privileges.py
#	tests/lib/conftest.py
#	tests/lib/test_client_model.py
#	tests/lib/test_oauth_util.py
@pattisdr pattisdr changed the title Add Role-Based Permissions[#2606] Add Role-Based Permissions [#2606] Feb 22, 2023
@cypress
Copy link

cypress bot commented Feb 22, 2023

Passing run #436 ↗︎

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 12a0c27 into e1d4302...
Project: fides Commit: 9f3e35e579 ℹ️
Status: Passed Duration: 00:38 💡
Started: Feb 27, 2023 2:44 PM Ended: Feb 27, 2023 2:45 PM

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

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Base: 86.46% // Head: 86.50% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (12a0c27) compared to base (e1d4302).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2671      +/-   ##
==========================================
+ Coverage   86.46%   86.50%   +0.03%     
==========================================
  Files         289      288       -1     
  Lines       15982    16025      +43     
  Branches     2027     2035       +8     
==========================================
+ Hits        13819    13862      +43     
  Misses       1779     1779              
  Partials      384      384              
Impacted Files Coverage Δ
src/fides/cli/commands/user.py 100.00% <ø> (ø)
.../fides/api/ops/api/v1/endpoints/oauth_endpoints.py 92.47% <100.00%> (+0.42%) ⬆️
.../ops/api/v1/endpoints/user_permission_endpoints.py 96.00% <100.00%> (+0.16%) ⬆️
src/fides/api/ops/api/v1/urn_registry.py 100.00% <100.00%> (ø)
src/fides/api/ops/schemas/user_permission.py 100.00% <100.00%> (ø)
src/fides/api/ops/util/oauth_util.py 92.50% <100.00%> (+1.59%) ⬆️
src/fides/core/config/security_settings.py 98.85% <100.00%> (+0.02%) ⬆️
src/fides/core/user.py 100.00% <100.00%> (ø)
src/fides/lib/cryptography/schemas/jwt.py 100.00% <100.00%> (ø)
src/fides/lib/models/client.py 100.00% <100.00%> (ø)
... and 3 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…ng - likely because we had two sets of user permissions schemas from the fideslib merge.

- Make the "id" optional in the request to edit user permissions.  The UI still passes this in but it's not used and we don't need it.
- Annotate the db_dataset with the new fields.
- Assert 422 when trying to create user perms w/ invalid scope.
@pattisdr pattisdr marked this pull request as ready for review February 23, 2023 05:03
src/fides/api/ops/util/oauth_util.py Show resolved Hide resolved
src/fides/lib/models/client.py Show resolved Hide resolved
src/fides/api/ops/schemas/user_permission.py Show resolved Hide resolved
src/fides/core/config/security_settings.py Outdated Show resolved Hide resolved
src/fides/core/user.py Outdated Show resolved Hide resolved
src/fides/core/user.py Outdated Show resolved Hide resolved
src/fides/lib/models/client.py Outdated Show resolved Hide resolved
@pattisdr pattisdr changed the title Add Role-Based Permissions [#2606] [Backend] Add Role-Based Permissions [#2606] Feb 23, 2023
# Conflicts:
#	src/fides/core/config/security_settings.py
#	src/fides/core/user.py
#	src/fides/lib/models/fides_user_permissions.py
#	tests/lib/conftest.py
#	tests/lib/test_client_model.py
#	tests/lib/test_fides_user_permissions.py
#	tests/lib/test_oauth_util.py
…ole to the root user, pull this from the config settings instead.

Permissions tests deleted (these were the "privileges" tests which is obsolete.)
@pattisdr
Copy link
Contributor Author

pattisdr commented Feb 23, 2023

hi @TheAndrewJackson @ThomasLaPiana @sanders41 @seanpreston - I'm looking for a reviewer that's worked on permissions-related items if one of you has time to look (or anyone else too!)

In short, this work adds the ability to assign a larger role to a user (which is associated with a list of scopes) in addition to still preserving the ability to assign scopes directly. Permission checking still looks to see if the user has the necessary scope - whether it was assigned directly or the user has a role associated with that scope.

@ThomasLaPiana
Copy link
Contributor

Looking now 👁️

@ThomasLaPiana
Copy link
Contributor

@pattisdr is it worth updating the CLI fides user create command to assign roles instead of scopes? It seems like it'll work fine either way, but figured we might as well update it to reflect best practices if this is how we're going to try to do things from now on

CONFLICT (content): Merge conflict in tests/lib/conftest.py
CONFLICT (content): Merge conflict in tests/lib/test_client_model.py
CONFLICT (content): Merge conflict in tests/ops/api/v1/endpoints/test_connection_config_endpoints.py
CONFLICT (content): Merge conflict in tests/ops/api/v1/endpoints/test_user_endpoints.py
CONFLICT (content): Merge conflict in tests/ops/api/v1/endpoints/test_user_permission_endpoints.py
CONFLICT (content): Merge conflict in tests/ops/conftest.py
…n't nullable previously). Default to an empty list.
…client - need all roles for test purposes so the client has the same role as the token.
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Looks good for merge if the checklist is satisfied

@pattisdr
Copy link
Contributor Author

Thanks for your reviews @sanders41 and @ThomasLaPiana!

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.

Add Role-Based Permissions Foundation
4 participants