✨ Consent: Pass consent string and metadata to amp-iframe by nerdbeere · Pull Request #29636 · ampproject/amphtml

nerdbeere · 2020-08-03T16:19:03Z

As discussed in #28719 it is necessary for us to retrieve the consent data within amp-iframe.

Implemented consent string and consent metadata forwarding to amp-iframe
Added documentation on how to retrieve consent data within the iframe
Added test page for amp-iframe with multiple CMPs
There are no tests yet, if this is implemented in the right spots I'm happy to add tests
Not implemented for amp-video-iframe because the implementation would probably look different

resolves #28719
related: #26938

zhouyx · 2020-08-06T17:31:21Z

👀

zhouyx · 2020-08-06T18:52:54Z

Thanks @nerdbeere for the Pull Request and my apology for the delay.

I agree with you that an API similar to send-interactions is desired. However one caveat is AMP currently doesn't support a consent update handler today. So in theory you would expect a postMessage whenever the consent changes, to be called whenever the consent changes, but in reality it will only give you the realtime consent information when send-consent-data is received.

I can think of a few approaches to address this
Option 1. In stead of supporting the send-consent-data API, have the <amp-iframe> pass in the initial consent information instead. This would be similar with the ad iframe.https://github.com/ampproject/amphtml/blob/master/3p/ampcontext.js#L349
This is preferred if you don't need following consent information update during one page visit.

Option 2. Go ahead with your current design. In future we can post all consent changes via the API.

Let me know if you think caveat we have today is fine with you, and we can proceed with option 2. Thanks.

nerdbeere · 2020-08-06T19:40:54Z

thanks for your feedback @zhouyx!
After some consideration and discussing it with my colleagues I think it would make sense to stick to the current design for two reasons:

It's closer to the current api that exists (postMessage). Passing it initially would probably mean to introduce an entirely new contract like amp context as you mentioned which I'm not sure is desired.
We'd like to help with the update mechanism and implement this not only for amp-iframe but also for amp-ad and amp-video-iframe if help is needed there. In this light I think it makes sense to introduce now a future proof contract. Ideally it behaves exactly like the intersection updates as consent can be revoked as well.

If the implementation in general is fine then can you maybe tell me what we should add/do in order to get this PR merged?

Thanks in advance!

zhouyx · 2020-08-06T20:57:51Z

extensions/amp-iframe/0.1/amp-iframe.js

+      this.subscriptionApiConsent_.send(
+        'consent-data',
+        dict({
+          'consentMetadata': consents[0],


Do you also want to pass the consentState. Asking because it's possible that no consentString or consentMetadata will be set (when there's <amp-consent> but no CMP)

Good thing about the object is that we can always add more field in the future : )

zhouyx · 2020-08-06T20:59:11Z

extensions/amp-iframe/amp-iframe.md


+## Iframe & Consent Data
+
+Iframes can send a `send-consent-data` message to receive consent data if a CMP is present on their parents page.


Based on the code, value will set only when data-block-on-consent attribute is added to the <amp-iframe> element. Is that expected?

Good point, that was a mistake on my end.

adjusted the test page to test with and without data-block-on-consent

use now policyId: default if data-block-on-consent not set

added some information on when to expect a consent response

zhouyx · 2020-08-06T21:00:13Z

extensions/amp-iframe/0.1/amp-iframe.js


+    this.subscriptionApiConsent_ = new SubscriptionApi(
+      iframe,
+      'send-consent-data',


nit: please declare the message type to MessageType within 3p-frame-messaging.js

Thanks for adding the messageType. Could you please also add consent-data there, and use the imported the message string.

zhouyx · 2020-08-06T23:47:02Z

examples/amp-consent/amp-consent-iframe.amp.html

+
+      <!-- This should be in alphabetical order. _ping_ should be the first -->
+
+      <!-- _ping_ example -->


I think adding the fake CMP _ping_ would be sufficient to test the feature. There's no need to add following <amp-consent> elements.
In this case, you don't need to change to app.js either.

zhouyx · 2020-08-06T23:52:06Z

extensions/amp-iframe/0.1/amp-iframe.js

      listenFor(iframe, 'embed-ready', this.activateIframe_.bind(this));
    }

+    this.subscriptionApiConsent_ = new SubscriptionApi(


Can we add a unit test to cover the send-consent-data msg? Thanks

nerdbeere · 2020-08-07T13:09:50Z

thank you for your in-depth review @zhouyx!
I addressed all your points, would be glad if you could re-check.

zhouyx · 2020-08-07T23:35:13Z

extensions/amp-iframe/0.1/amp-iframe.js

+      },
+      /*opt_is3P*/ undefined,
+      /*opt_includingNestedWindows*/ undefined,
+      /*opt_allowOpaqueOrigin*/ true


opt_is3P=false because we don't expect sentinel to match here.
opt_includingNestedWindows=false and opt_allowOpaqueOrigin=false because nested window and opaque origin are not supported with sentinel=amp.

Feel free to not include those three optional undefined variables.

zhouyx · 2020-08-07T23:37:50Z

extensions/amp-iframe/0.1/amp-iframe.js


+    this.subscriptionApiConsent_ = new SubscriptionApi(
+      iframe,
+      'send-consent-data',


Thanks for adding the messageType. Could you please also add consent-data there, and use the imported the message string.

zhouyx · 2020-08-08T00:03:57Z

extensions/amp-iframe/amp-iframe.md

+**Please note:**
+
+- The `consent-data` response will only be sent once and won't update the iframe when the consent state changes (for example when the user decides to reject consent by using the post prompt ui)
+- The response is delayed until the user interacts with the amp-consent ui in case iframe loading is not blocked by using `data-block-on-consent`


The response is not delayed until the user interact. It is delayed based on the data-block-on-consent value, and it's corresponding setting.
For example one can customize the consent service to resolve after a 2 seconds timeout. In this case, the response will resolve after 2 seconds, not wait for the user interaction.

Thanks for clarifying!

zhouyx · 2020-08-08T00:04:49Z

extensions/amp-iframe/0.1/amp-iframe.js

+   * @param {string} origin
+   * @param {JsonObject} data
+   * @private
+   * @return {Promise}


nit: doesn't return a promise. can be removed

nerdbeere · 2020-08-10T21:06:41Z

@zhouyx Thanks again for your help! Would be great if you could re-check again, ideally this would be in the next release as this is very critical for us because of the european privacy laws.

zhouyx

LGTM. Thanks for contributing!

zhouyx · 2020-08-10T23:20:43Z

to @alanorozco for owner's approval.

nerdbeere · 2020-08-17T12:38:55Z

Hi @alanorozco is something blocking this PR? Let me know if you need anything from us in order to get this merged.

alanorozco

LGTM with nits. Thanks!

alanorozco · 2020-08-25T18:46:14Z

extensions/amp-iframe/0.1/amp-iframe.js

+    const consentPolicyId = super.getConsentPolicy() || 'default';
+    const consentStringPromise = this.getConsentString_(consentPolicyId);
+    const metadataPromise = this.getConsentMetadata_(consentPolicyId);
+    const consentPolicyStatePromise = this.getConsentPolicyState_(


nit: You can get rid of these intermediate methods, just call getConsentPolicyInfo, etc. directly.

Answering for my colleague who is on vacation at the moment. These intermediate methods were introduced so that it is possible to write tests for this functionality (see mocking those functions here: https://github.com/ampproject/amphtml/pull/29636/files#diff-fd3e080d7fded1a7e2423b5ba744238bR945-R951). We've found similar ways to mock the unit-under-test within AMP within the test for send-intersections and embed-size. If we would call getConsentPolicyInfo directly the test would fail.

alanorozco · 2020-08-25T18:48:54Z

extensions/amp-iframe/amp-iframe.md

+window.addEventListener('message', function (event) {
+  if (
+    event.source != window.parent ||
+    event.origin == window.location.origin ||
+    !event.data ||
+    event.data.sentinel != 'amp' ||
+    event.data.type != 'consent-data'
+  ) {
+    return;
+  }
+  console.log(event.data.consentMetadata);
+  console.log(event.data.consentString);
+});


For sanity I would suggest naming the short-circuit clause.

Suggested change

window.addEventListener('message', function (event) {

if (

event.source != window.parent ||

event.origin == window.location.origin ||

!event.data ||

event.data.sentinel != 'amp' ||

event.data.type != 'consent-data'

) {

return;

}

console.log(event.data.consentMetadata);

console.log(event.data.consentString);

});

function isAmpMessage(event, type) {

return event.source == window.parent &&

event.origin != window.location.origin &&

event.data &&

event.data.sentinel == 'amp' &&

event.data.type == type;

}

window.addEventListener('message', function (event) {

if (!isAmpMessage(event, 'consent-data')) {

return;

}

console.log(event.data.consentMetadata);

console.log(event.data.consentString);

});

Adjusted the docs and also placed the same isAmpMessage function into the send-intersections documentation block. While adjusting the docs and doing a manual test with this isAmpMessage I've found that the sentinel = amp was not send along the consent-data message and I've fixed it with this change: f6110f9

alanorozco · 2020-08-25T18:50:06Z

extensions/amp-iframe/0.1/amp-iframe.js

+        dict({
+          'type': MessageType.CONSENT_DATA,
+          'consentMetadata': consents[0],
+          'consentString': consents[1],


How do we usually define data types for inbound messages? I wonder if it would make sense to have an intermediate consent object with these values.

What I've found: for embed-size the properties height and width are next to type (see https://github.com/ampproject/amphtml/blob/master/extensions/amp-iframe/0.1/amp-iframe.js#L467-L476). intersection event sends changes next to type (see https://github.com/ampproject/amphtml/blob/master/src/utils/intersection-observer-3p-host.js#L117-L120).

google-cla · 2020-08-26T13:21:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

klipstein · 2020-08-26T13:26:01Z

@googlebot I consent

nerdbeere · 2020-09-03T13:01:42Z

@alanorozco Hi Alan, in your opinion is this ready to go now? We'd really appreciate if this gets merged soon. Thanks!

zhouyx · 2020-09-08T19:00:08Z

I'm going to make the ads/ui team own the 3p iframe messaging related code.
@samouri @jridgewell could one of you approve the change to src/3p-frame-messaging.js in the meanwhile, so we can merge the change. Thank you!

nerdbeere · 2020-09-09T14:44:07Z

Hi @zhouyx @jridgewell any chance this can be merged now? Thanks!

zhouyx · 2020-09-09T17:59:03Z

PR Merged!

klipstein · 2020-09-10T11:56:37Z

Awesome! Thank you.

muotaz · 2020-09-15T10:01:19Z

Hi guys
When will this feature be available in the nightly or beta builds ?
Thanks,

zhouyx · 2020-09-16T18:54:27Z

Just checked, the feature should be available in nightly and beta build already.

…#29636) * Implement consent reading for 3p iframe * Add documentation for send-consent-data * Fix types * Use only _ping_ cmp on test page and revert changes in app.js * Add MessageType to 3p-frame-messaging.js * Allow retrieving consent without using data-block-on-consent * Add test and some refactoring * Fix linting in documentation * Adjust docs * Introduce and use MessageType and adjust jsdoc comment * Use fixture iframe instead of srcdoc * docs: extract isAmpMessage for receiving intersection and consent-data * Fix for missing sentinel="amp" Co-authored-by: Tobias von Klipstein <tobias@klpstn.com>

nerdbeere added 2 commits August 3, 2020 17:50

Implement consent reading for 3p iframe

d790cc7

Add documentation for send-consent-data

c14c23e

google-cla bot added the cla: yes label Aug 3, 2020

Fix types

2b84e9a

nerdbeere marked this pull request as ready for review August 3, 2020 17:30

amp-owners-bot bot requested review from estherkim and wassgha August 3, 2020 17:30

Merge branch 'master' into implement-consent-for-iframe

c72e467

kristoferbaxter requested review from alanorozco and zhouyx and removed request for wassgha August 5, 2020 15:42

zhouyx reviewed Aug 6, 2020

View reviewed changes

nerdbeere added 5 commits August 7, 2020 10:04

Merge branch 'master' into implement-consent-for-iframe

1b8248d

Use only _ping_ cmp on test page and revert changes in app.js

59d449b

Add MessageType to 3p-frame-messaging.js

b53d499

Allow retrieving consent without using data-block-on-consent

5b656d3

Add test and some refactoring

2bbc845

Fix linting in documentation

1135f05

zhouyx reviewed Aug 8, 2020

View reviewed changes

nerdbeere added 3 commits August 8, 2020 19:33

Adjust docs

0a60d7b

Introduce and use MessageType and adjust jsdoc comment

b70c60e

Use fixture iframe instead of srcdoc

6107798

zhouyx approved these changes Aug 10, 2020

View reviewed changes

zhouyx removed the request for review from estherkim August 10, 2020 23:20

bretg mentioned this pull request Aug 19, 2020

Load-Cookie: support passing AMP consent strings prebid/prebid-universal-creative#114

Closed

alanorozco approved these changes Aug 25, 2020

View reviewed changes

klipstein added 2 commits August 26, 2020 13:49

docs: extract isAmpMessage for receiving intersection and consent-data

1e8db1d

Fix for missing sentinel="amp"

f6110f9

google-cla bot added cla: no and removed cla: yes labels Aug 26, 2020

google-cla bot added cla: yes and removed cla: no labels Aug 26, 2020

jridgewell approved these changes Sep 8, 2020

View reviewed changes

zhouyx merged commit fb7291c into ampproject:master Sep 9, 2020

ampprojectbot added the PR use: In Beta / Experimental label Sep 15, 2020

klipstein mentioned this pull request Sep 24, 2020

I2I: Allow receiving TCF 2.0 consent string via TCF postmessage API #30385

Open


		## Iframe & Consent Data

		Iframes can send a `send-consent-data` message to receive consent data if a CMP is present on their parents page.


		<!-- This should be in alphabetical order. _ping_ should be the first -->

		<!-- _ping_ example -->

Conversation

nerdbeere commented Aug 3, 2020

Uh oh!

zhouyx commented Aug 6, 2020

Uh oh!

zhouyx commented Aug 6, 2020

Uh oh!

nerdbeere commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nerdbeere commented Aug 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nerdbeere commented Aug 10, 2020

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

zhouyx commented Aug 10, 2020

Uh oh!

nerdbeere commented Aug 17, 2020

Uh oh!

alanorozco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanorozco Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Aug 26, 2020

Uh oh!

klipstein commented Aug 26, 2020

Uh oh!

nerdbeere commented Sep 3, 2020

Uh oh!

zhouyx commented Sep 8, 2020

Uh oh!

nerdbeere commented Sep 9, 2020

Uh oh!

zhouyx commented Sep 9, 2020

Uh oh!

klipstein commented Sep 10, 2020

Uh oh!

muotaz commented Sep 15, 2020

Uh oh!

zhouyx commented Sep 16, 2020

Uh oh!

nerdbeere commented Aug 6, 2020 •

edited

Loading

alanorozco Aug 25, 2020 •

edited

Loading