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 v2CmpResponseCallback handle #5564

Merged
merged 2 commits into from
Aug 13, 2020
Merged

Conversation

akiselicki-liveramp
Copy link
Contributor

@akiselicki-liveramp akiselicki-liveramp commented Aug 3, 2020

Type of change

  • Bugfix

Description of change

With current implementation we are getting timeout for all cmp calls that doesn't satisfy defined conditions
even if the response is valid.

tcfData.eventStatus will always have one of those three values, purposeOneTreatment won't always be true and if gdpr doesn't apply TCData object will be without tcString and purposeOneTreatment properties.
In this case there is no need for those checks, only value that matters for cmp response to be valid is success.

Related issues: #5529, #5546

@andyblackwell
Copy link
Contributor

addEventListener does trigger in the wild without a tcfData.eventStatus (the liveramp cmp for non-EU traffic)

This is how I dealt with the gdprApplies=false case, to not get rid of those other necessary checks:

  function v2CmpResponseCallback(tcfData, success) {
    utils.logInfo('Received a response from CMP', tcfData);
    if (success) {
      if (tcfData.gdprApplies === false) {
        cmpSuccess(tcfData, hookConfig);
      } else if (tcfData.eventStatus === 'tcloaded' || tcfData.eventStatus === 'useractioncomplete') {
        cmpSuccess(tcfData, hookConfig);
      } else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString && tcfData.purposeOneTreatment === true) {
        cmpSuccess(tcfData, hookConfig);
      }
    } else {
      cmpError('CMP unable to register callback function.  Please check CMP setup.', hookConfig);
    }
  }

@nielsbaarsma
Copy link

Hey @andyblackwell , Can you elaborate why these additional checks are necessary? We're not seeing it.

@patmmccann
Copy link
Collaborator

@nielsbaarsma just as some background from the blame

#5050

#4911 (review)

fyi @harpere @jsnellbaker

@harpere harpere self-assigned this Aug 4, 2020
@harpere harpere added needs 2nd review Core module updates require two approvals from the core team needs review labels Aug 4, 2020
@harpere harpere self-requested a review August 4, 2020 14:59
@andyblackwell
Copy link
Contributor

thanks @patmmccann for the references! Yeah, there are reasons for those checks, I've seen @chrispaterson (IAB core api dev) also mentioning their importance before, like here: #5462 (comment)

@akiselicki-liveramp
Copy link
Contributor Author

Hi @andyblackwell I still don't see why those checks are necessary.
If I'm looking at the right place, in processCmpData() cmp data will be checked

  function checkV2Data() {
    // if CMP does not respond with a gdprApplies boolean, use defaultGdprScope (gdprScope)
    let gdprApplies = consentObject && typeof consentObject.gdprApplies === 'boolean' ? consentObject.gdprApplies : gdprScope;
    let tcString = consentObject && consentObject.tcString;
    return !!(
      (typeof gdprApplies !== 'boolean') ||
      (gdprApplies === true && !utils.isStr(tcString))
    );
  }

so if checkV2Data() is checking for gdprApplies and tcString there's no need for any check in v2CmpResponseCallback() or I'm missing something?

@patmmccann patmmccann mentioned this pull request Aug 5, 2020
@chrispaterson
Copy link

chrispaterson commented Aug 5, 2020

I think @akiselicki-liveramp is right. Before all statuses were accounted for and responded to in the same way. I'm not too familiar with what is going on here, but it seems like this is only a proxy method and that whatever passes in the cmpSuccess method should be handling the different addEventListener statuses and not this proxy method.

One thing that has me concerned is looking at purposeOneTreatment. As was said to Vizzini by Indigo Montoya, I don't think that means what you think it means. It should not be used for PreBid; it's for determining different handling of display of purpose one in different jurisdictions because that pertains to the eprivacy directive and different EU countries have different legal requirements. If you're looking for consent for purpose one, you should be checking tcfData.purpose.consent['1'] is true.

@andyblackwell
Copy link
Contributor

I was going to mention that the LiveRamp cmp started off with a default tcString and data before any user response, which might have caused an issue with this change, but that looks to have been fixed with the latest release yesterday. So your change may be fine? But I'll leave that to people who know the rules more clearly.

thanks for chiming in @chrispaterson! Is the IAB worried at all about things still being kinda up-in-the air like this 10 days out from the deadline?

} else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString && tcfData.purposeOneTreatment === true) {
cmpSuccess(tcfData, hookConfig);
}
cmpSuccess(tcfData, hookConfig);
Copy link
Collaborator

Choose a reason for hiding this comment

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

when the eventStatus === "cmpuishown" we may not have a valid tcString yet, like we do with "tcloaded" and "useractioncomplete". I don't recall or understand why tcfData.purposeOneTreatment is significant here, but I don't think we want to call cmpSuccess() if we're still waiting for a valid tcString.

Choose a reason for hiding this comment

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

The previous implementation calls cmpSuccess(tcfData, hookConfig) in all cases.

I don't think we want to call cmpSuccess() if we're still waiting for a valid tcString.

A CMP returns a "Valid" TC string in all eventStatuses. Once "cmpuishown" is triggered legitimate interest has been established but consent hasn't been gathered. Some "Vendors" only rely on that legal basis. Vendors should be able to determine which state they need the TCString to be in.

Copy link
Collaborator

@harpere harpere Aug 5, 2020

Choose a reason for hiding this comment

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

I incorrectly said the tcString may not be "valid". It's valid, but user consent may not have been gathered yet. I believe cmpSuccess() is intended to be called when the user's consent preferences have been obtained. Most bidders will want the user's consent, if possible, before sending a bid request. Perhaps a change could be made to give the publisher the option to proceed with the auction without giving the user a chance to respond. Maybe setting the cmpTimeout to zero already accomplishes this?

Choose a reason for hiding this comment

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

There is more nuance to TCFv2 in that a legitimate interest basis for processing personal data as defined by GDPR Article 6(1)(f) is now represented in the TC String. There are Vendors who operate solely on Legitimate Interest and that's why these events were created. It may be true that most bidders will want the user's consent but there are some who were advocating for this feature through the design of TCFv2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Prebid doesn't work that way. There's no mechanism for some bidders to wait for additional consent from the user (or wait for anything else for that matter) while others do not. All bidders are called at the same time and have the same amount of time to respond before being timed out. If a publisher doesn't want bidders to wait for the user to respond, they could set the cmpTimeout to 0 (or another small number). Though I guess in that case we wouldn't be using the default tcData that was returned (Though we may want to update the code to save it as a backup if we don't get a response from the other events before the timeout). There may also be some GDPR Enforcement repercussions to be considered (when that module is included), if we only get a default tcData object.

@harpere
Copy link
Collaborator

harpere commented Aug 5, 2020

I think the point was to wait for a valid tcString before calling cmpSuccess(). eventStatus === "cmpuishown" means we're still waiting for the user to respond, which is why we don't call cmpSuccess() for that event. We'd wait for the "useractioncomplete" eventStatus, or timeout. - like I said in another comment - I'm not sure why purposeOneTreatment is significant in the original code.

@chrispaterson
Copy link

@andyblackwell CMPs will no longer have access to the v1 global vendor list after the 15th, however v1 strings are "valid" to be processed until September 30th. Ostensibly, that means that no new v1 consent will be gathered by CMPs while existing v1 consent strings will still be able to be surfaced to vendors.

It's obviously a monumental task to move the entire industry along, but based on rapidly evolving privacy laws and case rulings it has to be done to preserve lawfulness in ad tech.

@harpere
Copy link
Collaborator

harpere commented Aug 5, 2020

I think adding this makes sense though

if (tcfData.gdprApplies === false) {
        cmpSuccess(tcfData, hookConfig);
}

@chrispaterson
Copy link

I think adding this makes sense though

if (tcfData.gdprApplies === false) {
        cmpSuccess(tcfData, hookConfig);
}

I don't see why, because all possible use cases are handled in this pull request. If gdprApplies is true or false cmpSuccess is called. (Am I missing something?)

@harpere
Copy link
Collaborator

harpere commented Aug 6, 2020

I don't think we're settled on calling cmpSuccess() on cmpuishown, at least by default, because then bidders would never have the opportunity to get the user's preferences before the auction begins. But if gdprApplies is false, we don't need the user's preferences.

@chrispaterson
Copy link

chrispaterson commented Aug 6, 2020

Here's how I think about it:

function v2CmpResponseCallback(tcData, success) {

  utils.logInfo('Received a response from CMP', tcData);

  if (success) {

    /**
     * First, did the CMP self-report success.  It's not well defined what
     * success=false actually means and when a CMP is to respond that way other
     * than it's a way to enable a CMP to respond without hanging calling
     * scripts if it's unable to perform the action for any reason.
     */

    if (tcData.gdprApplies === false) {

      // GDPR doesn't apply. Success! Nothing more to do.  God bless America!

      cmpSuccess(tcData, hookConfig);

    } else if (tcData.gdprApplies === true) {

      // GDPR does apply.  More to do...

      switch (tcData.eventStatus) {

        case 'tcloaded':

          /**
           * The CMP does not need to ask the user, because the user already
           * has valid consent/ transparency.  This is hopefully the majority
           * case of responses because the CMP does not ask for new consent every
           * page.  Worst case (safari or other cookie blocking browsers) the
           * CMP would ask every session.
           */
          cmpSuccess(tcData, hookConfig);

          break;
        case 'cmpuishown':

          /**
           * In this case the TC string from the CMP is a TCString with no consent 
           * values and only legitimate interest established values.  Most Vendors 
           * won't have the legal basis they need to process, but gdprApplies so they 
           * can't lawfully process.  Proportionally, this should be a very low volume.
           */
          cmpSuccess(tcData, hookConfig);

          break;
        case 'useractioncomplete':

          /**
           * You know the TC Data is valid and fresh here but this happens in
           * human-scale time because the user has to click accept or adjust
           * their preferences.  As an aside and one that you may already
           * know, something like 98%+ of users will click an "accept all".
           */
          cmpSuccess(tcData, hookConfig);

          break;
        default:

          /**
           * unspecified weirdness: handled.
           */
          cmpError('CMP unable to register callback function.  Please check CMP setup.', hookConfig);

      }

    } else {

      /**
       * GDPR applies is not defined for some reason... This should not happen,
       * but in a universe composed of infinite probability and unknown CMP
       * implementers it could.
       */

      cmpError('CMP unable to register callback function.  Please check CMP setup.', hookConfig);

    }

  } else {

    /**
     * No soup for you...
     */

    cmpError('CMP unable to register callback function.  Please check CMP setup.', hookConfig);

  }

}

@akiselicki-liveramp
Copy link
Contributor Author

Ok, so for now it makes sense to go with suggestion from @andyblackwell to only add new case if GDPR doesn't apply and we'll open a new issue to address the purposeOneTreatment.

@jsnellbaker
Copy link
Collaborator

To help comment, there was some internal discussion with our legal contact on this issue and what Prebid.js should be listening to event-wise. The consensus was that Prebid.js probably doesn't need to listen to the cmpuishown event at all, but it should listen to the tcloaded and useractioncomplete events.

Prebid.js cares about collecting 'consent' information about the user and providing that to all of the bidders in a timely and correct manner. To copy a snippet from the conversation that explains this best:

“cmpuishown” is designed to indicate the purgatory between a user being shown purposes (establishing legitimate interest) but before the user has made any consent elections. This state was surfaced for vendors relying solely on legitimate interest interacting with the CMP JS API. Prebid itself is not such a vendor, as it will always wait for the user’s consent choices to be known, by either ‘tcloaded’ or ‘useractioncomplete’.

If everyone else agrees, I think we can drop that particular if condition entirely and keep the rest of the checks that were implemented here (the other 2 statuses and gdprApplies check).

@akiselicki-liveramp @chrispaterson @harpere please let me know your thoughts on this suggestion.

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

From some additional conversation, these current changes around gdprApplies are fine so we can address some immediate concerns that were raised in lieu of Aug 15 deadline.

In regards to the previous comment of cmpuishown, we will create another PR to address that later.

@mkendall07 mkendall07 merged commit ef00b3f into prebid:master Aug 13, 2020
@chrispaterson
Copy link

@jsnellbaker that makes the most sense to me, especially since purpose 1 (device storage and access) requires consent in almost all jurisdictions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants