From 4b9718e5292d3a9e925f1fea2fa4ccd5bf11b66d Mon Sep 17 00:00:00 2001 From: Ali Ghassemi Date: Thu, 29 Jun 2017 18:35:03 -0700 Subject: [PATCH] Cid optout action (#9952) --- build-system/tasks/presubmit-checks.js | 1 + .../0.1/amp-user-notification.js | 109 ++++++++++++------ .../0.1/test/test-amp-user-notification.js | 45 ++++++++ .../amp-user-notification.md | 4 + gulpfile.js | 2 +- src/service/cid-impl.js | 61 +++++++++- test/functional/test-cid.js | 74 ++++++++++++ 7 files changed, 259 insertions(+), 37 deletions(-) diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 76c80e433698..de6334357f6e 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -397,6 +397,7 @@ var forbiddenTerms = { message: requiresReviewPrivacy, whitelist: [ 'src/services.js', + 'src/service/cid-impl.js', 'extensions/amp-user-notification/0.1/amp-user-notification.js', 'extensions/amp-app-banner/0.1/amp-app-banner.js', ], diff --git a/extensions/amp-user-notification/0.1/amp-user-notification.js b/extensions/amp-user-notification/0.1/amp-user-notification.js index 119e6fe49c88..2a10bfa0cae6 100644 --- a/extensions/amp-user-notification/0.1/amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/amp-user-notification.js @@ -86,14 +86,40 @@ export class AmpUserNotification extends AMP.BaseElement { constructor(element) { super(element); - /** @private @const {?UrlReplacements} */ - this.urlReplacements_ = null; + /** @private {?string} */ + this.ampUserId_ = null; - /** @private @const {?UserNotificationManager} */ - this.userNotificationManager_ = null; + /** @private {?string} */ + this.elementId_ = null; + + /** @private {?function()} */ + this.dialogResolve_ = null; + + /** @private {Promise} */ + this.dialogPromise_ = new Promise(resolve => { + this.dialogResolve_ = resolve; + }); - /** @const @private {?Promise} */ + /** @private {?string} */ + this.dismissHref_ = null; + + /** @private {boolean} */ + this.persistDismissal_ = false; + + /** @private {?string} */ + this.showIfHref_ = null; + + /** @private {string} */ + this.storageKey_ = ''; + + /** @private {?Promise} */ this.storagePromise_ = null; + + /** @private {?UserNotificationManager} */ + this.userNotificationManager_ = null; + + /** @private {?../../../src/service/url-replacements-impl.UrlReplacements} */ + this.urlReplacements_ = null; } /** @override */ @@ -112,30 +138,16 @@ export class AmpUserNotification extends AMP.BaseElement { 'userNotificationManager'); } - /** @private {?string} */ - this.ampUserId_ = null; - - /** @private {function()} */ - this.dialogResolve_ = null; - - /** @private {!Promise} */ - this.dialogPromise_ = new Promise(resolve => { - this.dialogResolve_ = resolve; - }); - this.elementId_ = user().assert(this.element.id, 'amp-user-notification should have an id.'); - /** @private @const {string} */ this.storageKey_ = 'amp-user-notification:' + this.elementId_; - /** @private @const {?string} */ this.showIfHref_ = this.element.getAttribute('data-show-if-href'); if (this.showIfHref_) { assertHttpsUrl(this.showIfHref_, this.element); } - /** @private @const {?string} */ this.dismissHref_ = this.element.getAttribute('data-dismiss-href'); if (this.dismissHref_) { assertHttpsUrl(this.dismissHref_, this.element); @@ -149,14 +161,15 @@ export class AmpUserNotification extends AMP.BaseElement { const persistDismissal = this.element.getAttribute( 'data-persist-dismissal'); - /** @private @const {boolean} */ + this.persistDismissal_ = ( persistDismissal != 'false' && persistDismissal != 'no'); this.userNotificationManager_ .registerUserNotification(this.elementId_, this); - this.registerAction('dismiss', this.dismiss.bind(this)); + this.registerAction('dismiss', () => this.dismiss(/*forceNoPersist*/false)); + this.registerAction('optoutOfCid', () => this.optoutOfCid_()); } /** @@ -167,12 +180,13 @@ export class AmpUserNotification extends AMP.BaseElement { * @private */ buildGetHref_(ampUserId) { - const showIfHref = dev().assert(this.showIfHref_); + const showIfHref = dev().assertString(this.showIfHref_); return this.urlReplacements_.expandAsync(showIfHref).then(href => { - return addParamsToUrl(href, { - elementId: this.elementId_, - ampUserId, + const data = /** @type {!JsonObject} */({ + 'elementId': this.elementId_, + 'ampUserId': ampUserId, }); + return addParamsToUrl(href, data); }); } @@ -200,14 +214,14 @@ export class AmpUserNotification extends AMP.BaseElement { * @return {!Promise} */ postDismissEnpoint_() { - return xhrFor(this.win).fetchJson(dev().assert(this.dismissHref_), { + return xhrFor(this.win).fetchJson(dev().assertString(this.dismissHref_), { method: 'POST', credentials: 'include', requireAmpResponseSourceOrigin: false, - body: { + body: /** @type {!JsonObject} */({ 'elementId': this.elementId_, 'ampUserId': this.ampUserId_, - }, + }), }); } @@ -232,12 +246,27 @@ export class AmpUserNotification extends AMP.BaseElement { } /** - * Get async cid service. + * Opts the user out of cid issuance and dismisses the notification. + * @private + */ + optoutOfCid_() { + return this.getCidService_() + .then(cid => cid.optOut()) + .then(() => this.dismiss(/*forceNoPersist*/false), reason => { + dev().error(TAG, + 'Failed to opt out of Cid', reason); + // If optout fails, dismiss notification without persisting. + this.dismiss(/*forceNoPersist*/true); + }); + } + + /** + * Get async cid. * @return {!Promise} * @private */ getAsyncCid_() { - return cidForDoc(this.element).then(cid => { + return this.getCidService_().then(cid => { // `amp-user-notification` is our cid scope, while we give it a resolved // promise for the 2nd argument so that the 3rd argument (the // persistentConsent) is the one used to resolve getting @@ -252,6 +281,15 @@ export class AmpUserNotification extends AMP.BaseElement { }); } + /** + * Get cid service. + * @return {!Promise} + * @private + */ + getCidService_() { + return cidForDoc(this.element); + } + /** @override */ shouldShow() { return this.isDismissed().then(dismissed => { @@ -302,20 +340,23 @@ export class AmpUserNotification extends AMP.BaseElement { /** @override */ activate() { - this.dismiss(); + this.dismiss(/*forceNoPersist*/false); } /** * Hides the current user notification and invokes the `dialogResolve_` * method. Removes the `.amp-active` class from the element. + * + * @param {boolean} forceNoPersist If true, dismissal won't be persisted + * regardless of 'data-persist-dismissal''s value */ - dismiss() { + dismiss(forceNoPersist) { this.element.classList.remove('amp-active'); this.element.classList.add('amp-hidden'); this.dialogResolve_(); this.getViewport().removeFromFixedLayer(this.element); - if (this.persistDismissal_) { + if (this.persistDismissal_ && !forceNoPersist) { // Store and post. this.storagePromise_.then(storage => { storage.set(this.storageKey_, true); @@ -348,7 +389,7 @@ export class UserNotificationManager { /** @private @const {!Object} */ this.deferRegistry_ = Object.create(null); - /** @private @const {!Viewer} */ + /** @private @const {!../../../src/service/viewer-impl.Viewer} */ this.viewer_ = viewerForDoc(this.win.document); /** @private @const {!Promise} */ diff --git a/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js b/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js index d799962612ac..1bbf25e0b590 100644 --- a/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js @@ -548,6 +548,7 @@ describes.realWin('amp-user-notification', { it('should be able to get AmpUserNotification object by ID', () => { const element = getUserNotification(); const userNotification = new AmpUserNotification(element); + userNotification.dialogResolve_(); service.registerUserNotification('n1', userNotification); return expect(service.get('n1')).to.eventually.equal(userNotification); }); @@ -582,4 +583,48 @@ describes.realWin('amp-user-notification', { expect(get().then).to.be.function; }); }); + + describe('optOutOfCid', () => { + const cidMock = { + optOut() { + return optoutPromise; + }, + }; + let dismissSpy; + let optoutPromise; + let optOutOfCidStub; + + beforeEach(() => { + optOutOfCidStub = sandbox.spy(cidMock, 'optOut'); + }); + + it('should call cid.optOut() and dismiss', () => { + const element = getUserNotification({id: 'n1'}); + const impl = element.implementation_; + impl.buildCallback(); + optoutPromise = Promise.resolve(); + impl.getCidService_ = () => { return Promise.resolve(cidMock); }; + dismissSpy = sandbox.spy(impl, 'dismiss'); + + return impl.optoutOfCid_().then(() => { + expect(dismissSpy).to.be.calledWithExactly(false); + expect(optOutOfCidStub).to.be.calledOnce; + }); + }); + + it('should dissmiss without persistence if cid.optOut() fails', () => { + const element = getUserNotification({id: 'n1'}); + const impl = element.implementation_; + impl.buildCallback(); + optoutPromise = Promise.reject('failed'); + impl.getCidService_ = () => { return Promise.resolve(cidMock); }; + dismissSpy = sandbox.spy(impl, 'dismiss'); + + return impl.optoutOfCid_().then(() => { + expect(dismissSpy).to.be.calledWithExactly(true); + expect(optOutOfCidStub).to.be.calledOnce; + }); + }); + + }); }); diff --git a/extensions/amp-user-notification/amp-user-notification.md b/extensions/amp-user-notification/amp-user-notification.md index 1463ada9feb1..0eb6ed40ac05 100644 --- a/extensions/amp-user-notification/amp-user-notification.md +++ b/extensions/amp-user-notification/amp-user-notification.md @@ -240,6 +240,10 @@ The `amp-user-notification` exposes the following actions that you can use [AMP dismiss (default) Closes the user notification; see usage for more details. + + optoutOfCid + User will be opted out of Client ID generation for all scopes. + ## Delaying Client ID generation until the notification is acknowledged diff --git a/gulpfile.js b/gulpfile.js index d7ff66b41791..4a321a05a59c 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -124,7 +124,7 @@ declareExtension('amp-slides', '0.1', false, 'NO_TYPE_CHECK'); declareExtension('amp-social-share', '0.1', true); declareExtension('amp-timeago', '0.1', false); declareExtension('amp-twitter', '0.1', false); -declareExtension('amp-user-notification', '0.1', true, 'NO_TYPE_CHECK'); +declareExtension('amp-user-notification', '0.1', true); declareExtension('amp-vimeo', '0.1', false, 'NO_TYPE_CHECK'); declareExtension('amp-vine', '0.1', false, 'NO_TYPE_CHECK'); declareExtension('amp-viz-vega', '0.1', true); diff --git a/src/service/cid-impl.js b/src/service/cid-impl.js index 38b8bcab208c..0513db1bb134 100644 --- a/src/service/cid-impl.js +++ b/src/service/cid-impl.js @@ -35,7 +35,7 @@ import { import {dict} from '../utils/object'; import {isIframed} from '../dom'; import {getCryptoRandomBytesArray} from '../utils/bytes'; -import {viewerForDoc} from '../services'; +import {viewerForDoc, storageForDoc} from '../services'; import {cryptoFor} from '../crypto'; import {parseJson, tryParseJson} from '../json'; import {timerFor} from '../services'; @@ -50,6 +50,10 @@ const BASE_CID_MAX_AGE_MILLIS = 365 * ONE_DAY_MILLIS; const SCOPE_NAME_VALIDATOR = /^[a-zA-Z0-9-_.]+$/; +const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout'; + +const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptOut'; + /** * A base cid string value and the time it was last read / stored. * @typedef {{time: time, cid: string}} @@ -107,7 +111,8 @@ export class Cid { * consent, of course). * @return {!Promise} A client identifier that should be used * within the current source origin and externalCidScope. Might be - * null if no identifier was found or could be made. + * null if user has opted out of cid or no identifier was found + * or it could be made. * This promise may take a long time to resolve if consent isn't * given. */ @@ -118,9 +123,17 @@ export class Cid { 'The CID scope and cookie name must only use the characters ' + '[a-zA-Z0-9-_.]+\nInstead found: %s', getCidStruct.scope); + return consent.then(() => { return viewerForDoc(this.ampdoc).whenFirstVisible(); }).then(() => { + // Check if user has globally opted out of CID, we do this after + // consent check since user can optout during consent process. + return isOptedOutOfCid(this.ampdoc); + }).then(optedOut => { + if (optedOut) { + return ''; + } const cidPromise = this.getExternalCid_( getCidStruct, opt_persistenceConsent || consent); // Getting the CID might involve an HTTP request. We timeout after 10s. @@ -133,6 +146,16 @@ export class Cid { }); } + /** + * User will be opted out of Cid issuance for all scopes. + * When opted-out Cid service will reject all `get` requests. + * + * @return {!Promise} + */ + optOut() { + return optOutOfCid(this.ampdoc); + } + /** * Returns the "external cid". This is a cid for a specific purpose * (Say Analytics provider X). It is unique per user, that purpose @@ -174,6 +197,40 @@ export class Cid { } } +/** + * User will be opted out of Cid issuance for all scopes. + * When opted-out Cid service will reject all `get` requests. + * + * @return {!Promise} + * @visibleForTesting + */ +export function optOutOfCid(ampdoc) { + + // Tell the viewer that user has opted out. + viewerForDoc(ampdoc)./*OK*/sendMessage(CID_OPTOUT_VIEWER_MESSAGE, dict()); + + // Store the optout bit in storage + return storageForDoc(ampdoc).then(storage => { + return storage.set(CID_OPTOUT_STORAGE_KEY, true); + }); +} + +/** + * Whether user has opted out of Cid issuance for all scopes. + * + * @param {!./ampdoc-impl.AmpDoc} ampdoc + * @return {!Promise} + * @visibleForTesting + */ +export function isOptedOutOfCid(ampdoc) { + return storageForDoc(ampdoc).then(storage => { + return storage.get(CID_OPTOUT_STORAGE_KEY).then(val => !!val); + }).catch(() => { + // If we fail to read the flag, assume not opted out. + return false; + }); +} + /** * Sets a new CID cookie for expire 1 year from now. * @param {!Window} win diff --git a/test/functional/test-cid.js b/test/functional/test-cid.js index 6f53e6b98878..ce164183b023 100644 --- a/test/functional/test-cid.js +++ b/test/functional/test-cid.js @@ -23,6 +23,8 @@ import { import { cidServiceForDocForTesting, getProxySourceOrigin, + optOutOfCid, + isOptedOutOfCid, } from '../../src/service/cid-impl'; import {installCryptoService, Crypto} from '../../src/service/crypto-impl'; import {cryptoFor} from '../../src/crypto'; @@ -61,6 +63,7 @@ describe('cid', () => { let whenFirstVisible; let trustedViewer; let shouldSendMessageTimeout; + let storageGetStub; const hasConsent = Promise.resolve(); const timer = timerFor(window); @@ -121,6 +124,7 @@ describe('cid', () => { }); installViewerServiceForDoc(ampdoc); + storageGetStub = stubServiceForDoc(sandbox, ampdoc, 'storage', 'get'); viewer = viewerForDoc(ampdoc); sandbox.stub(viewer, 'whenFirstVisible', function() { return whenFirstVisible; @@ -276,6 +280,18 @@ describe('cid', () => { 'sha384(YYYhttp://www.origin.come2)'); }); + it('should return empty if opted out', () => { + storageGetStub.withArgs('amp-cid-optout').returns(Promise.resolve(true)); + + storage['amp-cid'] = JSON.stringify({ + cid: 'YYY', + time: Date.now(), + }); + return compare( + 'e2', + ''); + }); + it('should read from viewer storage if embedded', () => { fakeWin.parent = {}; const expectedBaseCid = 'from-viewer'; @@ -742,3 +758,61 @@ describes.realWin('cid', {amp: true}, env => { expect(scopedCid).to.be.undefined; }); }); + +describes.fakeWin('cid optout:', {amp: true}, env => { + let storageGetStub; + let storageSetStub; + let viewerSendMessageStub; + let ampdoc; + + beforeEach(() => { + ampdoc = env.ampdoc; + storageSetStub = stubServiceForDoc(sandbox, ampdoc, 'storage', 'set'); + storageGetStub = stubServiceForDoc(sandbox, ampdoc, 'storage', 'get'); + viewerSendMessageStub = stubServiceForDoc(sandbox, ampdoc, + 'viewer', 'sendMessage'); + }); + + describe('optOutOfCid()', () => { + it('should send a message to viewer', () => { + return optOutOfCid(ampdoc).then(() => { + expect(viewerSendMessageStub).to.be.calledWith('cidOptOut'); + }); + }); + + it('should save bit in storage', () => { + optOutOfCid(ampdoc).then(() => { + expect(storageSetStub).to.be.calledWith('amp-cid-optout', true); + }); + }); + + it('should reject promise if storage set fails', () => { + storageSetStub.returns(Promise.reject('failed!')); + return optOutOfCid(ampdoc).should.eventually.be.rejectedWith('failed!'); + }); + }); + + describe('isOptedOutOfCid()', () => { + it('should return true if bit is set in storage', () => { + storageGetStub.withArgs('amp-cid-optout').returns(Promise.resolve(true)); + return isOptedOutOfCid(ampdoc).then(isOut => { + expect(isOut).to.be.true; + }); + }); + + it('should return false if bit is not set in storage', () => { + storageGetStub.withArgs('amp-cid-optout').returns(Promise.resolve(null)); + return isOptedOutOfCid(ampdoc).then(isOut => { + expect(isOut).to.be.false; + }); + }); + + it('should return false if storage get fails', () => { + storageGetStub.withArgs('amp-cid-optout').returns(Promise.reject('Fail')); + return isOptedOutOfCid(ampdoc).then(isOut => { + expect(isOut).to.be.false; + }); + }); + }); + +});