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

ConsentManagement: detect __cmp in window.top #2626

Merged
merged 4 commits into from
May 29, 2018

Conversation

benjaminclot
Copy link
Contributor

@benjaminclot benjaminclot commented May 25, 2018

Type of change

  • Feature
  • Code style update (formatting, local variables)

Description of change

At least one CMP (QuantCast, not to name it) doesn't create the __cmpLocator iFrame, so that when prebid is loaded from inside an iFrame it can't locate the CMP. This PR tries to detect if window.top.__cmp exists (after checking if window.top is accessible) and use it. If not, an error is returned.

Other information

While I'm aware that this should be fixed on the CMP level, adding this detection isn't too much of a hassle.

@YOzaz
Copy link

YOzaz commented May 25, 2018

I would actually replace this to:

var __cmpFunc = window.__cmp || window.top.__cmp;
if ( __cmpFunc ) ...

@benjaminclot
Copy link
Contributor Author

benjaminclot commented May 25, 2018

@YOzaz your example would throw an exception in an unfriendly iframe.

@YOzaz
Copy link

YOzaz commented May 25, 2018

Oh come on:

var __cmpFunc = null;

try {
   __cmpFunc = window.__cmp || window.top.__cmp;
} catch(e) { /** Silence is golden **/ }

if (utils.isFn(__cmpFunc)) ...

You can actually do window.parent traveling upwards if it's nested iframe as well here.

@mkendall07
Copy link
Member

Shouldn't QuantCast just become compliant with the CMP spec?

@YOzaz
Copy link

YOzaz commented May 25, 2018

@mkendall07 good point!

@benjaminclot
Copy link
Contributor Author

benjaminclot commented May 25, 2018

@YOzaz may be simple, but still worth mentioning.

@mkendall07 that's what I said in "other information" but considering it's an easy enough modification, I think Prebid.js should cover it, just in case. Because right now publishers/ad networks have to create a proxy for the window.top.__cmp function.

@YOzaz
Copy link

YOzaz commented May 25, 2018

@mkendall07 but even official example on Prebid.og docs doesn't inject an iframe :) from my point of view, this PR is quite logical, but I'd rewrite it like this:

let cmpFunc = null;
try {
   cmpFunc = window.__cmp || window.top.__cmp;
} catch(e) {}

if (utils.isFn(cmpFunc)) {
   cmpFunc('getConsentData', null, callbackHandler.consentDataCallback);
   cmpFunc('getVendorConsents', null, callbackHandler.vendorConsentsCallback);
} else if (inASafeFrame() && typeof window.$sf.ext.cmp === 'function') {
   callCmpWhileInSafeFrame('getConsentData', callbackHandler.consentDataCallback);
   callCmpWhileInSafeFrame('getVendorConsents', callbackHandler.vendorConsentsCallback);
} else {
   // find the CMP frame
   let f = window;
   let cmpFrame;
   while (!cmpFrame) {
      try {
         if (f.frames['__cmpLocator']) cmpFrame = f;
      } catch (e) {}
      if (f === window.top) break;
      f = f.parent;
   }

   callCmpWhileInIframe('getConsentData', cmpFrame, callbackHandler.consentDataCallback);
   callCmpWhileInIframe('getVendorConsents', cmpFrame, callbackHandler.vendorConsentsCallback);
}

Difference: no duplicated code.

@mkendall07
Copy link
Member

@YOzaz
Which prebid.org example are you referring to?

@YOzaz
Copy link

YOzaz commented May 25, 2018

@mkendall07 sorry, my bad. It's there now.

@benjaminclot
Copy link
Contributor Author

PR updated.

@jsnellbaker
Copy link
Collaborator

@benjaminclot and all, thanks for the conversation/feedback here.

I think the updated code may be more ideal here, I'll start testing it out in a bit. @benjaminclot Can you please take a look at/fix the lint errors in the Travis job that failed in the meantime?

@benjaminclot
Copy link
Contributor Author

@jsnellbaker fixed.

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.

LGTM

@jsnellbaker jsnellbaker added needs 2nd review Core module updates require two approvals from the core team LGTM labels May 25, 2018
@YOzaz
Copy link

YOzaz commented May 25, 2018

@mkendall07 actually, it's not there - I was referring to this example: http://prebid.org/dev-docs/modules/consentManagement.html#publishers-not-using-an-iab-compliant-cmp.
In a situation, where stub would be loaded in a main window, and Prebid.js in DFP async mode (friendly IFRAME) as a postbid process - without this PR, ConsentManagement module would fail.
I've put a PR to accomodate IFRAME creation process, and other IAB mandatory requirements Prebid.js may (or may not) use in the future: prebid/prebid.github.io#792. It may be an overkill, though.

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@jsnellbaker jsnellbaker merged commit b199353 into prebid:master May 29, 2018
Pupis pushed a commit to adform/Prebid.js that referenced this pull request Jun 7, 2018
* ConsentManagement: detect __cmp in window.top

* Lighter modification

Credits to @YOzaz

* typo

* fixed spaces
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* ConsentManagement: detect __cmp in window.top

* Lighter modification

Credits to @YOzaz

* typo

* fixed spaces
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* ConsentManagement: detect __cmp in window.top

* Lighter modification

Credits to @YOzaz

* typo

* fixed spaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM 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.

5 participants