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

Add auth to the masking endpoints #2909

Merged
merged 6 commits into from
Mar 27, 2023
Merged

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Mar 24, 2023

Closes https://github.com/ethyca/security-issues/issues/13

Code Changes

  • rename verify_oauth_client_cli -> verify_oauth_client_prod
  • add MASKING scopes for MASKING:EXEC and MASKING:READ
  • add the new MASKING scopes to the VIEWER role (they are low-risk and without side-effects)
  • add the Security dependency to the masking endpoints

Steps to Confirm

  • in a prod security environment, try to hit the masking endpoints, expect a 401
  • after authenticating (logging in), try again and see it work as expected

Pre-Merge Checklist

Description Of Changes

This is PR adds authentication to the masking endpoints as well as accompanying scopes. Note that it is only authenticated in prod mode to make it a non-breaking change.

@ThomasLaPiana ThomasLaPiana self-assigned this Mar 24, 2023
@cypress
Copy link

cypress bot commented Mar 24, 2023

Passing run #1011 ↗︎

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 5c5c969 into 2c4e492...
Project: fides Commit: c2d58c34f5 ℹ️
Status: Passed Duration: 00:41 💡
Started: Mar 27, 2023 4:32 AM Ended: Mar 27, 2023 4:33 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 24, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2c4e492) 86.63% compared to head (5c5c969) 86.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2909   +/-   ##
=======================================
  Coverage   86.63%   86.63%           
=======================================
  Files         299      299           
  Lines       16832    16837    +5     
  Branches     2148     2148           
=======================================
+ Hits        14582    14587    +5     
  Misses       1841     1841           
  Partials      409      409           
Impacted Files Coverage Δ
src/fides/lib/oauth/roles.py 100.00% <ø> (ø)
src/fides/api/ctl/routes/admin.py 95.83% <100.00%> (ø)
src/fides/api/ctl/routes/crud.py 87.67% <100.00%> (ø)
src/fides/api/ctl/routes/datamap.py 57.89% <100.00%> (ø)
src/fides/api/ctl/routes/generate.py 46.39% <100.00%> (ø)
src/fides/api/ctl/routes/validate.py 63.63% <100.00%> (ø)
src/fides/api/ctl/view.py 29.26% <100.00%> (ø)
src/fides/api/main.py 79.14% <100.00%> (ø)
...ides/api/ops/api/v1/endpoints/masking_endpoints.py 92.10% <100.00%> (+0.43%) ⬆️
src/fides/api/ops/api/v1/scope_registry.py 100.00% <100.00%> (ø)
... and 1 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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review March 24, 2023 14:29
Copy link
Contributor

@RobertKeyser RobertKeyser left a comment

Choose a reason for hiding this comment

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

Looks good from my end. No concerns, but others should probably give the explicit approval :)

@ThomasLaPiana ThomasLaPiana merged commit 128f017 into main Mar 27, 2023
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-add-masking-auth branch March 27, 2023 04:47
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.

4 participants