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

Fix FidesUserPermissions Scopes Mutation #2883

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Mar 21, 2023

Closes Unticketed

Code Changes

  • Copies the FidesUserPermission.scopes before modifying in the FidesUserPermission.total_scopes property

Steps to Confirm

  • Repeated calls to get user permissions should not increase the length of a user's scopes

Pre-Merge Checklist

Description Of Changes

Calling FidesUserPermission.total_scopes could mutate whatever list was passed in for FidesUserPermission.scopes: taking every scope that was on the user's role and adding it to the list of scopes.

This could have side effects such as a token for a root client being created that had many duplicate scopes. Note that the root client is not a database object, but the CONFIG.security.root_user_scopes value could get mutated on subsequent calls to the get user permissions endpoint.

@cypress
Copy link

cypress bot commented Mar 21, 2023

Passing run #886 ↗︎

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 cc017b7 into 92a1191...
Project: fides Commit: ee585568f6 ℹ️
Status: Passed Duration: 00:51 💡
Started: Mar 21, 2023 1:04 AM Ended: Mar 21, 2023 1:05 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 21, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (92a1191) 86.73% compared to head (2b87e52) 86.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2883      +/-   ##
==========================================
- Coverage   86.73%   86.72%   -0.02%     
==========================================
  Files         295      295              
  Lines       16760    16760              
  Branches     2146     2146              
==========================================
- Hits        14537    14535       -2     
- Misses       1821     1822       +1     
- Partials      402      403       +1     
Impacted Files Coverage Δ
src/fides/api/ops/schemas/privacy_request.py 100.00% <100.00%> (ø)
src/fides/lib/models/fides_user_permissions.py 100.00% <100.00%> (ø)
src/fides/lib/oauth/api/routes/user_endpoints.py 95.40% <100.00%> (ø)

... and 1 file with indirect coverage changes

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 marked this pull request as ready for review March 21, 2023 01:27
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.

excellent test addition, too!

@pattisdr pattisdr merged commit cb77946 into main Mar 21, 2023
@pattisdr pattisdr deleted the fidesuserpermission_scopes_mutation branch March 21, 2023 14:19
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.

2 participants