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 cookies not deleting on opt-out #5338

Merged
merged 31 commits into from
Oct 16, 2024

Conversation

jpople
Copy link
Contributor

@jpople jpople commented Sep 27, 2024

Closes PROD-2822

Associated fidesplus PR- https://github.com/ethyca/fidesplus/pull/1653

Description Of Changes

Fixes a bug where cookies were not being deleted when a user opted out due to js-cookie attempting to exactly match a provided domain even when it was null.

Adds a new field on privacyexperienceconfig table, auto_subdomain_cookie_deletion; when true, opting out will cause cookies from subdomains (prefixed with .s) to be deleted as well as cookies from the top-level domain.

Note that I've manually tested the migration using the following steps:

  1. Added some privacy experience configs to mock pre-existing customers' configs
  2. Ran migration
  3. Confirmed the auto_subdomain_cookie_deletion was false
  4. Added net new privacy experience config
  5. Confirmed the auto_subdomain_cookie_deletionwastrue` ✅

Code Changes

  • Add toggle to experience config in admin-ui that controls whether we automatically delete subdomain cookies
  • Updates appropriate models across the fides ecosystem to account for this new field in the BE
  • Adds migration to write the auto_subdomain_cookie_deletion to "false" for existing privacy experience configs

Steps to Confirm

Test using local fides-js against a staging Ethyca site which GETs localhost:3001 for fides-js.

Run the associated Fidesplus branch (this is pointed to the aa28ea20aeee81d4f73db25538af5413718b9ef1 fides commit since there are BE changes here)

Toggle the auto_subdomain_cookie_deletion field on your privacy experience to false:

  • Opt in to a privacy notice
  • Should save cookies
  • Opt out
  • Should delete only top-level cookies with domain of ethyca.com, not subdomain cookies with domain of .ethyca.com

Toggle auto_subdomain_cookie_deletion on your privacy experience to true and re-run, then:

  • Opt in, should save as before
  • On opt out, should delete cookies both with top-level and subdomains

Pre-Merge Checklist

  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Copy link

vercel bot commented Sep 27, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Oct 16, 2024 3:13pm

Copy link
Contributor

@tvandort tvandort left a comment

Choose a reason for hiding this comment

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

Neat! Thanks!

I'm curious, was the js-cookie library not working for removing these if we passed the same domains?

Copy link

cypress bot commented Sep 27, 2024

fides    Run #10468

Run Properties:  status check passed Passed #10468  •  git commit 3e80c67836 ℹ️: Merge dadabee99f86effa25aa50719c92266784e10a13 into 88a32f00b57b3bc29b7c638fb0d9...
Project fides
Run status status check passed Passed #10468
Run duration 00m 38s
Commit git commit 3e80c67836 ℹ️: Merge dadabee99f86effa25aa50719c92266784e10a13 into 88a32f00b57b3bc29b7c638fb0d9...
Committer jpople
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

@jpople jpople marked this pull request as ready for review September 30, 2024 19:41
Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

Looking good @jpople ! In addition to my comment below:

Could we add a test that basically asserts we do not support cookie deletion for compass cookies yet, and link to https://ethyca.atlassian.net/browse/PROD-2830 as the follow-up? E.g. when cookie domain is ["google.com", "googleadservices.com"]' cookie removal won't work.

If that's gonna take too much time, I'd say just leave a link to the PROD ticket in the cookie removal logic 👍

clients/fides-js/src/lib/cookie.ts Outdated Show resolved Hide resolved
@jpople
Copy link
Contributor Author

jpople commented Oct 3, 2024

@eastandwestwind I added a test case to cover a cookie with a passed-in domain, but the tests on removeCookieFromBrowser are just detecting whether cookies.remove() is called with the right arguments as they were before. I'm not sure if it's possible to test whether that actually results in a cookie being deleted or not.

…ion to write false initial val for existing customers
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.57%. Comparing base (88a32f0) to head (dadabee).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5338      +/-   ##
==========================================
+ Coverage   85.56%   85.57%   +0.01%     
==========================================
  Files         379      379              
  Lines       23982    23985       +3     
  Branches     2623     2623              
==========================================
+ Hits        20520    20526       +6     
+ Misses       2910     2907       -3     
  Partials      552      552              

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

@eastandwestwind eastandwestwind merged commit 7591e3f into main Oct 16, 2024
45 checks passed
@eastandwestwind eastandwestwind deleted the jpople/prod-2822/cookie-deletion-bug branch October 16, 2024 15:57
Copy link

cypress bot commented Oct 16, 2024

fides    Run #10470

Run Properties:  status check passed Passed #10470  •  git commit 7591e3fb38: Fix cookies not deleting on opt-out (#5338)
Project fides
Run status status check passed Passed #10470
Run duration 00m 42s
Commit git commit 7591e3fb38: Fix cookies not deleting on opt-out (#5338)
Committer jpople
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

@Kelsey-Ethyca Kelsey-Ethyca mentioned this pull request Oct 16, 2024
14 tasks
Kelsey-Ethyca pushed a commit that referenced this pull request Oct 16, 2024
Co-authored-by: eastandwestwind <eastandwestwind@gmail.com>
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.

3 participants