Backend System-Level Cookie Support #4383

pattisdr · 2023-11-03T19:35:48Z

Closes https://ethyca.atlassian.net/browse/PROD-1310

❗ Contains migration; check downrev before merge
❗ Blocked by fideslang release: https://github.com/ethyca/fideslang/pull/181
❗ Blocked by FE accompaniment: https://ethyca.atlassian.net/browse/PROD-1319

Description Of Changes

Add system-level cookie support, (cookies that are attached at the System level and not directly to a data use). Remove the functionality that keeps a cookie attached to a System if its PrivacyDeclaration is deleted. Cookies should now be linked to a privacy declaration OR a system, not both.

Separately, while here:

Make a small change to SystemHistory.edited_by property, to allow automated System Updates to not show null in System History because they don't correspond to a user
Add new system "previous_vendor_id" column to Systems history table so we can update fideslang

Code Changes

Temporarily pin to fideslang commit that adds "cookies" to System schema.
Remove cookies from SystemResponse schema now that it is on the inherited System schema
Update "create_system" to pop off system level cookies from the request and upsert them separately
Update "update_system" to pop off system level cookies and upsert separately
Update "upsert_cookies" method to take in a system or a privacy declaration and link to the appropriate resource, not both.
Now cascade delete Cookies when a Privacy Declaration is updated instead of setting the privacy declaration id to null
Small data migration to detach Cookies from Systems that are also attached to a Privacy Declaration
Update SystemHistory.edited_by to return the user_id as-is if there is no matching user
Add new System.previous_vendor_id column to support being able to upgrade fideslang and pass in with System request in FE

Steps to Confirm

You can now POST cookies at the System level directly. These are not associated with a data use:

curl -X 'POST' \
  'http://localhost:8080/api/v1/system' \
  -H 'accept: application/json' \
  -H 'Authorization: Bearer <REDACTED>' \
  -H 'Content-Type: application/json' \
  -d '{
  "fides_key": "system_a",
  "system_type": "system",
  "name": "System A",
  "description": "test description",
  "privacy_declarations": [],
  "cookies": [
    {
      "name": "test_cookie",
      "path": "/",
      "domain": "https://www.example.com"
    }
  ]
}'

Also adding Cookies in the UI to a Privacy Declaration and then deleting the Privacy Declaration will now delete the Cookie (verify in the System response that it is not on System.cookies)

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

…tem level and not directly to a data use. Remove the functionality that keeps a cookie attached to a System if its PrivacyDeclaration is deleted. Cookies should now be linked to a privacy declaration or a system, not both. - Temporarily pin to fideslang commit that adds "cookies" to System schema. - Remove cookies from SystemResponse schema now that it is on the inherited System schema - Update "create_system" to pop off system level cookies from the request and upsert them separately - Update "update_system" to pop off system level cookies and upsert separately - Update "upsert_cookies" method to take in a system or a privacy declaration and link to the appropriate resource, not both. - Now cascade delete Cookies when a Privacy Declaration is updated instead of setting the privacy declaration id to null - Small data migration to detach Cookies from Systems that are also attached to a Privacy Declaration

…tem history response when no user id can be found. Just return the original id. This will be useful for automated system updates.

cypress · 2023-11-03T19:52:41Z

Passing run #5068 ↗︎

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

Details:

Merge `6564fb5` into `cd37e68`...
Project: fides	Commit: `d3013d91d3 ℹ️`
Status: Passed	Duration: 00:34 💡
Started: Nov 6, 2023 10:57 PM	Ended: Nov 6, 2023 10:58 PM

Review all test suite changes for PR #4383 ↗︎

… reporting - that cookie updates on systems can show up in the history.

pattisdr · 2023-11-03T20:20:56Z

@allisonking you can add the FE here directly if that's easier!

pattisdr · 2023-11-03T20:25:13Z

we might need to push a fideslang alpha tag for this if that not being merged yet blocks you here -

codecov · 2023-11-03T20:58:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cd37e68) 87.04% compared to head (6564fb5) 87.07%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4383      +/-   ##
==========================================
+ Coverage   87.04%   87.07%   +0.02%     
==========================================
  Files         328      328              
  Lines       20267    20279      +12     
  Branches     2608     2610       +2     
==========================================
+ Hits        17642    17658      +16     
+ Misses       2162     2159       -3     
+ Partials      463      462       -1

Files	Coverage Δ
src/fides/api/db/system.py	`90.37% <100.00%> (+0.93%)`	⬆️
src/fides/api/models/sql_models.py	`98.18% <100.00%> (+<0.01%)`	⬆️
src/fides/api/models/system_history.py	`92.85% <100.00%> (+14.28%)`	⬆️
src/fides/api/schemas/system.py	`100.00% <ø> (ø)`

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

adamsachs

this looks great to me @pattisdr ! the code reads very cleanly, and the functionality here feels a lot more straightforward and maintainable.

thanks for revisiting this and taking the time to resolve this the right way

requirements.txt

src/fides/api/db/system.py

src/fides/api/models/system_history.py

src/fides/api/db/system.py

allisonking · 2023-11-06T18:30:11Z

Did a separate PR for the FE just because a good number of files changed (all very small changes!) #4395

Co-authored-by: Adam Sachs <[email protected]>

vercel · 2023-11-06T18:58:36Z

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 6, 2023 10:47pm

Co-authored-by: Adam Sachs <[email protected]>

pattisdr · 2023-11-06T21:25:05Z

Lots of failing tests since bumping fideslang requirement - investigating.

clients/admin-ui/src/types/api/models/BasicSystemResponse.ts

pattisdr added 3 commits November 3, 2023 14:33

Small update to change how the system editor is serialized in the sys…

5bf32cf

…tem history response when no user id can be found. Just return the original id. This will be useful for automated system updates.

Update changelog

6c65dbb

pattisdr requested a review from adamsachs November 3, 2023 20:04

Add test asserting the main side effect in this PR for system history…

dbbd6ee

… reporting - that cookie updates on systems can show up in the history.

allisonking mentioned this pull request Nov 6, 2023

System level cookie handling #4395

Merged

16 tasks

adamsachs approved these changes Nov 6, 2023

View reviewed changes

Update src/fides/api/db/system.py

2d51391

Co-authored-by: Adam Sachs <[email protected]>

pattisdr and others added 3 commits November 6, 2023 12:58

Update src/fides/api/db/system.py

b98814b

Co-authored-by: Adam Sachs <[email protected]>

Updated fideslang requirement

476e542

System level cookie handling (#4395)

9a5f306

vercel bot deployed to Preview November 6, 2023 19:46 View deployment

pattisdr added 2 commits November 6, 2023 16:00

Add nullable previous_vendor_id column to System model.

8c4c259

Add previous_vendor_id to system form.

d8cbeeb

vercel bot deployed to Preview November 6, 2023 22:28 View deployment

pattisdr commented Nov 6, 2023

View reviewed changes

clients/admin-ui/src/types/api/models/BasicSystemResponse.ts Show resolved Hide resolved

pattisdr added 2 commits November 6, 2023 16:43

Add previous_vendor_id to dataset annotations.

fffc19b

Merge branch 'main' into PROD-1310_system_cookies

6564fb5

pattisdr merged commit 5122e33 into main Nov 7, 2023
48 checks passed

pattisdr deleted the PROD-1310_system_cookies branch November 7, 2023 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend System-Level Cookie Support #4383

Backend System-Level Cookie Support #4383

pattisdr commented Nov 3, 2023 •

edited

Loading

cypress bot commented Nov 3, 2023 •

edited

Loading

pattisdr commented Nov 3, 2023

pattisdr commented Nov 3, 2023

codecov bot commented Nov 3, 2023 •

edited

Loading

adamsachs left a comment

allisonking commented Nov 6, 2023

vercel bot commented Nov 6, 2023 •

edited

Loading

pattisdr commented Nov 6, 2023

Backend System-Level Cookie Support #4383

Backend System-Level Cookie Support #4383

Conversation

pattisdr commented Nov 3, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

cypress bot commented Nov 3, 2023 • edited Loading

Passing run #5068 ↗︎

Review all test suite changes for PR #4383 ↗︎

pattisdr commented Nov 3, 2023

pattisdr commented Nov 3, 2023

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

adamsachs left a comment

Choose a reason for hiding this comment

allisonking commented Nov 6, 2023

vercel bot commented Nov 6, 2023 • edited Loading

pattisdr commented Nov 6, 2023

pattisdr commented Nov 3, 2023 •

edited

Loading

cypress bot commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

vercel bot commented Nov 6, 2023 •

edited

Loading