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

Consent Reporting Client IP #4440

Merged
merged 8 commits into from
Nov 22, 2023
Merged

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Nov 17, 2023

Closes #PROD-1412

Description Of Changes

For legal purposes, sometimes a truncated version of the client ip is needed to prove what actions were taken after consent was saved. Here we are running fides in such a way that the ip of the load balancer is being recorded, not the client ip.

Code Changes

  • Attempt to log first x-forwarded-for ip if it exists, otherwise, drop back to request.client.host

Steps to Confirm

  • Easiest way to check is to wait when RC tag is created for fides and is being used in Fidesplus
  • PATCH Privacy preferences with x-forwarded-for header
curl -X 'PATCH' \
  'http://localhost:8080/api/v1/privacy-preferences' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -H 'X-Forwarded-For: 35.226.242.253,1.1.1' \
  -d '{
  "purpose_consent_preferences": [{"id": 2, "preference": "opt_in"}],
  "browser_identity": {
    "fides_user_device_id": "3a0edd75-30dd-4c6c-acd0-4877a8d662a7"
  }
}'
  • Make a request to get historical privacy preferences: GET http://localhost:8080/api/v1/historical-privacy-preferences?page=1&size=50

  • In this case ,the truncated IP address is: 35.226.242.0

  • If IP address is invalid it is not saved

  • You can also test directly with served notices in Fides (not Plus) with x-forwarded-for header, but to validate the IP address involves inspecting the db directly

curl -X 'PATCH' \
  'http://localhost:8080/api/v1/notices-served' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -H 'X-Forwarded-For: 123.123.123.123,1.1.1.1,2.2.2.2' \
  -d '{
  "browser_identity": {
    "fides_user_device_id": "208f6661-60ac-456a-8143-4954ff427b6c"
  },
  "tcf_purpose_consents": [1],
   "serving_component": "overlay"
}'

Pre-Merge Checklist

Copy link

vercel bot commented Nov 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Nov 22, 2023 4:17pm

Copy link

cypress bot commented Nov 17, 2023

Passing run #5340 ↗︎

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 3b4f9bd into 5790645...
Project: fides Commit: e13cd5e34a ℹ️
Status: Passed Duration: 00:33 💡
Started: Nov 22, 2023 4:27 PM Ended: Nov 22, 2023 4:28 PM

Review all test suite changes for PR #4440 ↗︎

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5790645) 87.10% compared to head (3b4f9bd) 86.77%.

Files Patch % Lines
...i/api/v1/endpoints/privacy_preference_endpoints.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4440      +/-   ##
==========================================
- Coverage   87.10%   86.77%   -0.33%     
==========================================
  Files         329      329              
  Lines       20366    20377      +11     
  Branches     2623     2625       +2     
==========================================
- Hits        17739    17682      -57     
- Misses       2163     2238      +75     
+ Partials      464      457       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pattisdr pattisdr marked this pull request as ready for review November 20, 2023 21:50
Copy link
Contributor

@adamsachs adamsachs 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 to me! relatively straightforward in terms of app code changes, i think.

tentative approval but seems like we need to resolve your more deployment/config related question re: the uvicorn settings...

@pattisdr pattisdr merged commit 1459323 into main Nov 22, 2023
39 of 42 checks passed
@pattisdr pattisdr deleted the PROD-1412_ip_address_behind_load_balancer branch November 22, 2023 17:15
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