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

Public Endpoints Privacy Experiences and Save Privacy Preferences #3339

Merged
merged 7 commits into from
May 22, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented May 19, 2023

Closes #3229, #3228

Code Changes

  • Make the Privacy Experience List endpoint public
  • Separate out rate limiting the limits for public endpoints can be configured differently
  • Add new public rate limits to GET Experience List and PATCH Privacy Experience Endpoints
  • Remove unused GET Privacy Experience Detail endpoint - seems like the UI is just making use of the List endpoint, and we shouldn't have an extra public endpoint if we don't need it
  • Add validation on fides user device id type

Steps to Confirm

  • Set the CONFIG.security.public_request_rate_limit locally to be something like 5/minute and then send GET PrivacyExperience List and PATCH PrivacyPreference endpoints locally in rapid succession to confirm this separate rate limiting
  • Verify token no longer required for Privacy Experience List
  • Attempt to save preferences with a badly formatted fides user device id and verify this fails.

Pre-Merge Checklist

Description Of Changes

Make the Privacy Experience List endpoint public and add rate limiting.

@cypress
Copy link

cypress bot commented May 19, 2023

Passing run #2115 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge db9b405 into c83e84e...
Project: fides Commit: cc726e2d0c ℹ️
Status: Passed Duration: 01:00 💡
Started: May 22, 2023 9:17 PM Ended: May 22, 2023 9:18 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 May 20, 2023

Codecov Report

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

Comparison is base (c83e84e) 87.09% compared to head (db9b405) 87.07%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3339      +/-   ##
==========================================
- Coverage   87.09%   87.07%   -0.03%     
==========================================
  Files         313      313              
  Lines       18988    18997       +9     
  Branches     2475     2474       -1     
==========================================
+ Hits        16537    16541       +4     
- Misses       2009     2014       +5     
  Partials      442      442              
Impacted Files Coverage Δ
...i/api/v1/endpoints/privacy_experience_endpoints.py 90.90% <100.00%> (-9.10%) ⬇️
...i/api/v1/endpoints/privacy_preference_endpoints.py 98.64% <100.00%> (+<0.01%) ⬆️
src/fides/api/api/v1/endpoints/utils.py 100.00% <100.00%> (ø)
src/fides/api/app_setup.py 77.17% <100.00%> (ø)
src/fides/api/schemas/redis_cache.py 100.00% <100.00%> (ø)
src/fides/core/config/security_settings.py 98.87% <100.00%> (+0.01%) ⬆️

☔ 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 May 20, 2023 02:09
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 looks great, really clean implementation! is there a test for a uuid that isn't the right format? I didn't see one, but I very well may have missed it in the large diff 😄

pattisdr added 3 commits May 22, 2023 15:29
# Conflicts:
#	CHANGELOG.md
#	tests/ops/api/v1/endpoints/test_privacy_experience_endpoints.py
@pattisdr
Copy link
Contributor Author

is there a test for a uuid that isn't the right format?

Good call, @allisonking. None of our previous tests had the UUID in the right format on the backend, they were all "test_id" or similar so I changed all of those and neglected to add the test you mention!

@pattisdr pattisdr merged commit 52bcd75 into main May 22, 2023
@pattisdr pattisdr deleted the public_privacy_preference_endpoints branch May 22, 2023 21:37
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.

Improve security around public API for privacy experiences
2 participants