-
Notifications
You must be signed in to change notification settings - Fork 77
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
TCF bundling based on geography #4131
TCF bundling based on geography #4131
Conversation
Passing run #4256 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
@@ -38,16 +34,6 @@ interface Props { | |||
renderModalContent: (props: RenderModalContent) => VNode; | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized this wasn't needed anymore once I fixed how the updating cookie logic in place worked
@@ -136,7 +141,8 @@ const updateCookie = async ( | |||
}; | |||
|
|||
const tcString = await generateTcString({ tcStringPreferences, experience }); | |||
return { ...oldCookie, tc_string: tcString }; | |||
const tcfConsent = transformTcfPreferencesToCookieKeys(tcStringPreferences); | |||
return { ...oldCookie, tc_string: tcString, tcf_consent: tcfConsent }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably should have been in #4124 but I didn't realize it was missing until I started switching between TCF and non-TCF
@@ -252,18 +262,12 @@ export const initialize = async ({ | |||
isPrivacyExperience(effectiveExperience) && | |||
experienceIsValid(effectiveExperience, options) | |||
) { | |||
// Overwrite cookie consent with experience-based consent values | |||
// eslint-disable-next-line no-param-reassign | |||
cookie.consent = buildCookieConsentForExperiences( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was messing up when going from Notices --> TCF because TCF doesn't have any notices, so this was making the consent
object {}
which would erase if someone had made choices in CA. So this logic got moved out to only notices. TCF was already reassigning the cookie, so this makes notices do the same, by having it also pass in an updateCookie
func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does have an interesting side effect which I'm not sure if we can avoid. Let's say our cookie starts as
{
consent: {
data_sales_and_sharing: false
}
}
Now we switch to TCF. when TCF initializes, there are no notices, so it only has the consent defaults, which it merges with the existing cookie
{
consent: {
data_sales: true,
tracking: true,
analytics: true,
data_sales_and_sharing: true
}
}
I'm not sure if it's ok to have the consent defaults hanging around like that. That's from this function which I think is there so that we continue support for the legacy config. We could potentially say just ignore those defaults if we are in TCF mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, thinking beyond one impl on a customer's site, suppose a web user were to visit 2 different websites, site A uses Fides non-TCF and the site B uses Fides TCF. The user has submitted consent for site A, then navigates to site B. I'd think we'd want to preserve any existing consent the user has given on site A, rather than overwrite with defaults from TCF, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
site A and site B would use different cookies, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think so. It should just be the same fides_consent
cookie
transformUserPreferenceToBoolean(consentPreference), | ||
]) | ||
); | ||
const consentCookieKey: CookieKeyConsent = Object.fromEntries(noticeMap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to https://github.com/ethyca/fides/pull/4131/files#r1333616066, this moves the cookie assignment logic out of the shared function, and into a notice-specific function. this is because otherwise, when we save TCF (which has no notices), we would overwrite cookie.consent
to equal {}
which isn't right!
// Update the cookie object | ||
// eslint-disable-next-line no-param-reassign | ||
cookie.consent = consentCookieKey; | ||
const fidesUserPreferences: Array<ConsentOptionCreate> | undefined = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same, but wrapped in a ternary to be a little more careful about what preferences we pass to the backend. we don't want to accidentally overwrite notice preferences when working with tcf data!
debugLog(debug, "Updating window.Fides"); | ||
window.Fides.consent = cookie.consent; | ||
window.Fides.tc_string = cookie.tc_string; | ||
window.Fides.tcf_consent = cookie.tcf_consent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also probably should have been in #4124
// 2. Update the window.Fides.consent object | ||
// 2. Update the cookie object based on new preferences | ||
const updatedCookie = await updateCookie(cookie); | ||
Object.assign(cookie, updatedCookie); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned something weird— assigning to the function param with something like
param = updatedParam;
doesn't work! (param
in the parent scope is unchanged). However, doing an Object.assign
does work, and so does assigning to individual attributes such as
param.consent = updatedParam.consent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 questions for you!
// We determine server-side whether or not to send the TCF bundle, which is based | ||
// on whether or not the experience is marked as TCF. This means for TCF, we *must* | ||
// be able to prefetch the experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like something we should log. So if the experience is TCF, and prefetch is NOT enabled in the env vars, we should log a warning or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit of a chicken and egg problem, where the backend will never return that the experience is TCF if prefetch is not enabled 😕 the flow is
- Get the user's location
- Prefetch the experience, passing in the user location
- Check if it's TCF, which can only happen if both 1 and 2 work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, I've moved this suggestion here instead- https://ethyca.atlassian.net/browse/PROD-1069?focusedCommentId=36963
@@ -252,18 +262,12 @@ export const initialize = async ({ | |||
isPrivacyExperience(effectiveExperience) && | |||
experienceIsValid(effectiveExperience, options) | |||
) { | |||
// Overwrite cookie consent with experience-based consent values | |||
// eslint-disable-next-line no-param-reassign | |||
cookie.consent = buildCookieConsentForExperiences( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, thinking beyond one impl on a customer's site, suppose a web user were to visit 2 different websites, site A uses Fides non-TCF and the site B uses Fides TCF. The user has submitted consent for site A, then navigates to site B. I'd think we'd want to preserve any existing consent the user has given on site A, rather than overwrite with defaults from TCF, right?
…geography-bundling
Closes #3957
Description Of Changes
We can now allow both a notice experience and a TCF experience!
Code Changes
TCF_ENABLED
environment variable from the PC—now we determine if it's TCF purely from the backend 🎉Steps to Confirm
nox -s "fides_env(slim)" -- prod_prerelease core_version=2.20.2a2
turbo dev
inclients/privacy-center
. I used these env variables (note: no moreTCF_ENABLED
🎉 )localhost:8080/docs
Available
.localhost:3000
. Choose California from the geo picker. You should see "Manage preferences" show up at the bottom. Click it and you should see "Data sales and sharing". The banner won't come up since that's not required for CAPre-Merge Checklist
CHANGELOG.md