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

Backup CID in Storage for non-proxy origin #30375

Merged
merged 8 commits into from
Oct 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,7 @@ const forbiddenTermsSrcInclusive = {
'\\.setNonBoolean\\(': {
message: requiresReviewPrivacy,
allowlist: [
'src/service/cid-impl.js',
'src/service/storage-impl.js',
'extensions/amp-consent/0.1/consent-state-manager.js',
],
Expand Down
1 change: 1 addition & 0 deletions spec/amp-var-substitutions.md
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ You can pass the following arguments into the Client ID variable like a function
- `cid scope` (Required): The namespace for the Client ID.
- `amp-user-notification id` (Optional): Use this argument to make the Client ID substitution dependent on the dismissal of a user notification shown to the visitor of the page. In amp-analytics, this is the same as using the [`data-consent-notification-id`](../extensions/amp-analytics/amp-analytics.md) attribute -- you may choose to use either one for the amp-analytics component.
- `cookie name` (Optional): The name of the fallback cookie when the document is not served by an AMP proxy. If not provided, `cid scope` will be used as the cookie name.
- `disable backup` (Optional): If set to `true`, then will opt-out of storing the AMP generated CID in Storage as a backup when the document is not served by an AMP proxy. Otherwise, will default into this behavior.

#### Content Load Time

Expand Down
155 changes: 116 additions & 39 deletions src/service/cid-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,20 @@
* For details, see https://goo.gl/Mwaacs
*/

import {GoogleCidApi, TokenStatus} from './cid-api';
import {dev, rethrowAsync, user, userAssert} from '../log';
import {getCookie, setCookie} from '../cookies';
import {getServiceForDoc, registerServiceBuilderForDoc} from '../service';
import {getSourceOrigin, isProxyOrigin, parseUrlDeprecated} from '../url';
import {parseJson, tryParseJson} from '../json';

import {CacheCidApi} from './cache-cid-api';
import {GoogleCidApi, TokenStatus} from './cid-api';
import {Services} from '../services';
import {ViewerCidApi} from './viewer-cid-api';
import {base64UrlEncodeFromBytes} from '../utils/base64';
import {dev, rethrowAsync, user, userAssert} from '../log';
import {dict} from '../utils/object';
import {getCookie, setCookie} from '../cookies';
import {getCryptoRandomBytesArray} from '../utils/bytes';
import {getServiceForDoc, registerServiceBuilderForDoc} from '../service';
import {getSourceOrigin, isProxyOrigin, parseUrlDeprecated} from '../url';
import {isExperimentOn} from '../../src/experiments';
import {isIframed} from '../dom';
import {parseJson, tryParseJson} from '../json';
import {tryResolve} from '../utils/promise';

const ONE_DAY_MILLIS = 24 * 3600 * 1000;
Expand All @@ -51,6 +51,8 @@ const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout';

const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptOut';

const CID_BACKUP_STORAGE_KEY = 'amp-cid:';

/**
* Tag for debug logging.
* @const @private {string}
Expand Down Expand Up @@ -89,10 +91,14 @@ let BaseCidInfoDef;
* The "get CID" parameters.
* - createCookieIfNotPresent: Whether CID is allowed to create a cookie when.
* Default value is `false`.
* - cookieName: Name of the cookie to be used if defined for non-proxy case.
* - disableBackup: Whether CID should not be backed up in Storage.
* Default value is `false`.
* @typedef {{
* scope: string,
* createCookieIfNotPresent: (boolean|undefined),
* cookieName: (string|undefined),
* disableBackup: (boolean|undefined),
* }}
*/
let GetCidDef;
Expand Down Expand Up @@ -173,6 +179,9 @@ class Cid {

/** @private {?Object<string, string>} */
this.apiKeyMap_ = null;

/** @const {boolean} */
this.isBackupCidExpOn = isExperimentOn(this.ampdoc.win, 'amp-cid-backup');
}

/** @override */
Expand Down Expand Up @@ -382,53 +391,121 @@ function setCidCookie(win, scope, cookie) {
});
}

/**
* Sets a new CID backup in Storage
* @param {!./ampdoc-impl.AmpDoc} ampdoc
* @param {string} cookieName
* @param {string} cookie
*/
function setCidBackup(ampdoc, cookieName, cookie) {
Services.storageForDoc(ampdoc).then((storage) => {
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
const isViewerStorage = storage.isViewerStorage();
if (!isViewerStorage) {
const key = getStorageKey(cookieName);
storage.setNonBoolean(key, cookie);
}
});
}

/**
* @param {string} cookieName
* @return {string}
*/
function getStorageKey(cookieName) {
return CID_BACKUP_STORAGE_KEY + cookieName;
}

/**
* Maybe gets the CID from cookie or, if allowed, gets backup CID
* from Storage.
* @param {!Cid} cid
* @param {!GetCidDef} getCidStruct
* @return {!Promise<?string>}
*/
function maybeGetCidFromCookieOrBackup(cid, getCidStruct) {
const {ampdoc, isBackupCidExpOn} = cid;
const {win} = ampdoc;
const {disableBackup, scope} = getCidStruct;
const cookieName = getCidStruct.cookieName || scope;
const existingCookie = getCookie(win, cookieName);

if (existingCookie) {
return Promise.resolve(existingCookie);
}
if (isBackupCidExpOn && !disableBackup) {
return Services.storageForDoc(ampdoc)
.then((storage) => {
const key = getStorageKey(cookieName);
return storage.get(key, BASE_CID_MAX_AGE_MILLIS);
})
.then((backupCid) => {
if (!backupCid || typeof backupCid != 'string') {
return null;
}
return backupCid;
});
}
return Promise.resolve(null);
}

/**
* If cookie exists it's returned immediately. Otherwise, if instructed, the
* new cookie is created.
*
* @param {!Cid} cid
* @param {!GetCidDef} getCidStruct
* @param {!Promise} persistenceConsent
* @return {!Promise<?string>}
*/
function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
const {win} = cid.ampdoc;
const {scope} = getCidStruct;
const {isBackupCidExpOn, ampdoc} = cid;
const {win} = ampdoc;
const {scope, disableBackup} = getCidStruct;
const cookieName = getCidStruct.cookieName || scope;
const existingCookie = getCookie(win, cookieName);

if (!existingCookie && !getCidStruct.createCookieIfNotPresent) {
return /** @type {!Promise<?string>} */ (Promise.resolve(null));
}
return maybeGetCidFromCookieOrBackup(cid, getCidStruct).then(
(existingCookie) => {
if (!existingCookie && !getCidStruct.createCookieIfNotPresent) {
return /** @type {!Promise<?string>} */ (Promise.resolve(null));
}

if (existingCookie) {
// If we created the cookie, update it's expiration time.
if (/^amp-/.test(existingCookie)) {
setCidCookie(win, cookieName, existingCookie);
}
return /** @type {!Promise<?string>} */ (Promise.resolve(existingCookie));
}
if (existingCookie) {
// If we created the cookie, update it's expiration time.
if (/^amp-/.test(existingCookie)) {
setCidCookie(win, cookieName, existingCookie);
if (isBackupCidExpOn && !disableBackup) {
setCidBackup(ampdoc, cookieName, existingCookie);
}
}
return /** @type {!Promise<?string>} */ (Promise.resolve(
existingCookie
));
}

if (cid.externalCidCache_[scope]) {
return /** @type {!Promise<?string>} */ (cid.externalCidCache_[scope]);
}
if (cid.externalCidCache_[scope]) {
return /** @type {!Promise<?string>} */ (cid.externalCidCache_[scope]);
}

const newCookiePromise = getRandomString64(win)
// Create new cookie, always prefixed with "amp-", so that we can see from
// the value whether we created it.
.then((randomStr) => 'amp-' + randomStr);

// Store it as a cookie based on the persistence consent.
Promise.all([newCookiePromise, persistenceConsent]).then((results) => {
// The initial CID generation is inherently racy. First one that gets
// consent wins.
const newCookie = results[0];
const relookup = getCookie(win, cookieName);
if (!relookup) {
setCidCookie(win, cookieName, newCookie);
const newCookiePromise = getRandomString64(win)
// Create new cookie, always prefixed with "amp-", so that we can see from
// the value whether we created it.
.then((randomStr) => 'amp-' + randomStr);

// Store it as a cookie based on the persistence consent.
Promise.all([newCookiePromise, persistenceConsent]).then((results) => {
// The initial CID generation is inherently racy. First one that gets
// consent wins.
const newCookie = results[0];
const relookup = getCookie(win, cookieName);
if (!relookup) {
setCidCookie(win, cookieName, newCookie);
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
if (isBackupCidExpOn && !disableBackup) {
setCidBackup(ampdoc, cookieName, newCookie);
}
}
});
return (cid.externalCidCache_[scope] = newCookiePromise);
}
});
return (cid.externalCidCache_[scope] = newCookiePromise);
);
}

/**
Expand Down
16 changes: 12 additions & 4 deletions src/service/storage-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,11 @@ export class Storage {
* Returns the promise that yields the value of the property for the specified
* key.
* @param {string} name
* @param {number=} opt_duration
* @return {!Promise<*>}
*/
get(name) {
return this.getStore_().then((store) => store.get(name));
get(name, opt_duration) {
return this.getStore_().then((store) => store.get(name, opt_duration));
}

/**
Expand Down Expand Up @@ -223,12 +224,19 @@ export class Store {

/**
* @param {string} name
* @param {number|undefined} opt_duration
* @return {*|undefined}
*/
get(name) {
get(name, opt_duration) {
// The structure is {key: {v: *, t: time}}
const item = this.values_[name];
return item ? item['v'] : undefined;
const timestamp = item ? item['t'] : undefined;
const isNotExpired =
opt_duration && timestamp != undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 0 or negative value a valid opt_duration value? We need to handle them specially if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not valid values, so I don't think we need to handle them specifically.

? timestamp + opt_duration > Date.now()
: true;
const value = item && isNotExpired ? item['v'] : undefined;
return value;
}

/**
Expand Down
6 changes: 4 additions & 2 deletions src/service/url-replacements-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ export class GlobalVariableSource extends VariableSource {
}
return clientIds[scope];
},
(scope, opt_userNotificationId, opt_cookieName) => {
(scope, opt_userNotificationId, opt_cookieName, opt_disableBackup) => {
userAssert(
scope,
'The first argument to CLIENT_ID, the fallback' +
Expand All @@ -287,11 +287,13 @@ export class GlobalVariableSource extends VariableSource {
}
return Services.cidForDoc(this.ampdoc)
.then((cid) => {
opt_disableBackup = opt_disableBackup == 'true' ? true : false;
return cid.get(
{
/** @type {string} */ scope,
createCookieIfNotPresent: true,
cookieName: opt_cookieName,
cookieName: opt_cookieName || undefined,
disableBackup: opt_disableBackup,
},
consent
);
Expand Down
Loading