-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
add support for TCF2 #4911
add support for TCF2 #4911
Conversation
This pull request introduces 1 alert when merging e114843 into bd5408c - view on LGTM.com new alerts:
|
} else if (inASafeFrame() && typeof window.$sf.ext.cmp === 'function') { | ||
callCmpWhileInSafeFrame('getConsentData', callbackHandler.consentDataCallback); | ||
callCmpWhileInSafeFrame('getVendorConsents', callbackHandler.vendorConsentsCallback); | ||
utils.logInfo('Detected Prebid.js is encased in a SafeFrame and CMP is registered, calling it now...'); |
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.
Hi @jaiminpanchal27 , Could you please add a comment in code about "only TCF1 is handled in the case of SafeFrame and in case of TCF2 post-messages will do the job and sf.ext.cmp is not required." Everyone looking at the code may not be aware of the new change :-)
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.
Hi @pm-harshad-mane
Thanks for this feedback. I did add a comment here to note it was only used for the v1 spec. However, I also updated the conditional to check if the cmpVersion
variable was equal to 1 (which should be detected from the findCMP()
logic); this is to ensure it's only used for the v1 spec.
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.
Cool, Thanks @jaiminpanchal27
This pull request introduces 1 alert when merging cd5ae8b into 4de8941 - view on LGTM.com new alerts:
|
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.
one question
if (success) { | ||
if (tcfData.eventStatus === 'tcloaded' || tcfData.eventStatus === 'useractioncomplete') { | ||
cmpSuccess(tcfData, hookConfig); | ||
} else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString.length > 0 && tcfData.purposeOneTreatment === true) { |
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 don't understand why purposeOneTreatment
or purpose 1
in general matters here.
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 the advice from Xandr legal counsel. We might in the future add a flag to get around it, but for now it's an edge case in the flowchart.
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.
Trying to understand the lawyer's rational - Purpose 1 has to do with device access, right? And this conditional says that we're passing the consent string (if it exists) through to the bidders while the cmp ui is still being shown to the user, as long as they've given device access consent. What does passing consent on to the bidders have to do with device access?
Taking a shot at an explanation - If we know that the user has already given some positive consent (good) pass the string along. If not (not as good), give them the opportunity to change it.
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.
It's saying that we can move on with an older or incomplete version of consent without waiting for the useractioncomplete event as long as the string contains indication that publisher declared the ability to "read the device". It's unclear to me that this is an obvious fit for purposeOneTreatment, but P1T's actual meaning is murky and this doesn't seem like it should hurt anything.
If P1T isn't utilized, the PBJS will just wait for the user action to complete.
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.
ok
This pull request introduces 1 alert when merging 9e290df into 703b898 - view on LGTM.com new alerts:
|
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.
LGTM
* initial support for TCF2 * update wording in err message * update logic and comment around safeframe workflow * fix typo
* initial support for TCF2 * update wording in err message * update logic and comment around safeframe workflow * fix typo
* initial support for TCF2 * update wording in err message * update logic and comment around safeframe workflow * fix typo
Type of change
Description of change
Implements the feature described in #4801
Key things to note:
NOTE This does slightly change the logic used for the v1 checks, which only checked the current window then the top window when finding the CMP API. I think this should be fine.
removeEventListeners
references in order to keep the listeners open and ready to respond. This also includes removing thedelete cmpCallbacks[i.callId];
code we had for the iframe route (so we can act properly when a follow-up event is triggered from the originalcallId
).userId/index.js
to handle looking at the updated version of thevendorData
object for the TCF2 spec.As a follow-up to the last note, the are 8 bid adapters (currently) who read the
gdprConsent.vendorData
field to either do some logic or pass along some of the values in their request. These bidders are as followed:Ideally, these adapters should be updated to read the new
gdprConsent.apiVersion
field so they can look at the updated locations/field names for thevendorData
they need to know. I (hopefully) tagged all the main contributors so they are aware.