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

Reduce size of tcf_consent payload in fides_consent cookie #4480

Merged
merged 20 commits into from
Dec 5, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Dec 4, 2023

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

Description Of Changes

This PR refactors core TCF logic to rely on Fides str (in cookie.fides_string) for most TCF consent preferences instead of using our tcf_consent obj in the cookie. This reduces the size of our tcf_consent payload we send in every HTTP req between client <---> server which reduces risk for any slowdown due to cookie size.

Code Changes

  • Removes unneeded props from tcf_consent, refactor usages to rely on the fides string instead.
  • Add e2e tests

Steps to Confirm

  • Configure 1k+ systems by bulk adding the entire GVL list
  • Open the TCF banner in a sample app and accept all vendors
  • Inspect the size of the fides_consent cookie. It should be smaller than before
  • Additionally, this should be fully regression tested, for mostly TCF banner / modal, but also for notice-based consent. Confirm consent selections persist correctly when you make a selection on either the banner/modal, then force the modal to open on next page load via "manage preferences" link.

Pre-Merge Checklist

Copy link

vercel bot commented Dec 4, 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 Dec 5, 2023 6:29pm

Copy link

cypress bot commented Dec 4, 2023

Passing run #5499 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 5ee1414 into c94a3ea...
Project: fides Commit: 2189040424 ℹ️
Status: Passed Duration: 00:33 💡
Started: Dec 5, 2023 6:40 PM Ended: Dec 5, 2023 6:41 PM

Review all test suite changes for PR #4480 ↗︎

@allisonking
Copy link
Contributor Author

I've updated tests to no longer expect a tcf_consent to contain info for purposes, special features, or vendors. The tests that assert for tcf_consent.system_* are still there since that key should still exist.

There are two tests which are failing but which once we implement this ticket should pass. They're labeled with "TODO: CURRENTLY FAILING!!". That said, I wouldn't be surprised if more things came up during implementation that we need to tweak the tests for!

clients/fides-js/src/lib/tcf/constants.ts Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Show resolved Hide resolved
clients/fides-js/src/lib/cookie.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/lib/tcf/constants.ts Outdated Show resolved Hide resolved
@@ -484,165 +444,6 @@ describe("updateExperienceFromCookieConsent", () => {
]);
});
});

describe("tcf", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to remove these tests because updateExperienceFromCookieConsent actually needs to use the new updateExperienceFromCookieConsentTcf method.

I had some issues with jest / preact exporting it from fides-tcf.ts, but decided to remove it as a) this is already covered by our e2e tests and b) updateExperienceFromCookieConsentTcf exists in fides-tcf.ts and is therefore not a candidate for a cookie.ts unit test.

Copy link
Contributor

@eastandwestwind eastandwestwind Dec 5, 2023

Choose a reason for hiding this comment

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

implemented @allisonking 's suggestion to we could also move that function to one of the tcf/lib files (or a new one) and then move that test to a different test file (not cookie.test.ts)

@eastandwestwind eastandwestwind marked this pull request as ready for review December 5, 2023 18:29
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

Nice... let's ship it.

Comment on lines +241 to 258
if (fidesString) {
try {
// Make sure TC string is valid before we assign it
const { tc: tcString } = decodeFidesString(fidesString);
TCString.decode(tcString);
const updatedCookie: Partial<FidesCookie> = {
fides_string: fidesString,
tcf_version_hash:
overrides.consentPrefsOverrides?.version_hash ??
cookie.tcf_version_hash,
};
Object.assign(cookie, updatedCookie);
} catch (error) {
debugLog(
config.options.debug,
`Could not decode tcString from ${fidesString}, it may be invalid. ${error}`
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this got... a lot simpler! That's actually a pretty good sign 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I agree! was a pleasant surprise 😄

Comment on lines -288 to -292
debugLog(
config.options.debug,
"fides_string was missing from cookie, so it has been generated based on tcf_consent",
cookie.fides_string
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally makes sense that this case is removed now. Nice.

@allisonking allisonking merged commit a8efadd into main Dec 5, 2023
13 checks passed
@allisonking allisonking deleted the prod-1413/tcf-cookie-refactor branch December 5, 2023 22:29
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