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

Introduces CryptoService as a wrapper around sha384 lib. #3841

Merged
merged 7 commits into from
Jun 30, 2016
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
7 changes: 7 additions & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ var forbiddenTerms = {
'extensions/amp-analytics/0.1/amp-analytics.js',
],
},
'installCryptoService': {
message: privateServiceFactory,
whitelist: [
'extensions/amp-analytics/0.1/amp-analytics.js',
'extensions/amp-analytics/0.1/crypto-impl.js',
],
},
'installDocService': {
message: privateServiceFactory,
whitelist: [
Expand Down
24 changes: 13 additions & 11 deletions extensions/amp-analytics/0.1/amp-analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,21 @@ import {assertHttpsUrl, addParamsToUrl} from '../../../src/url';
import {dev, user} from '../../../src/log';
import {expandTemplate} from '../../../src/string';
import {installCidService} from './cid-impl';
import {installCryptoService} from './crypto-impl';
import {installStorageService} from './storage-impl';
import {installActivityService} from './activity-impl';
import {installVisibilityService} from './visibility-impl';
import {isArray, isObject} from '../../../src/types';
import {sendRequest, sendRequestUsingIframe} from './transport';
import {urlReplacementsFor} from '../../../src/url-replacements';
import {userNotificationManagerFor} from '../../../src/user-notification';
import {cryptoFor} from '../../../src/crypto';
import {xhrFor} from '../../../src/xhr';
import {toggle} from '../../../src/style';
import {sha384} from '../../../third_party/closure-library/sha384-generated';

installActivityService(AMP.win);
installCidService(AMP.win);
installCryptoService(AMP.win);
installStorageService(AMP.win);
installVisibilityService(AMP.win);
instrumentationServiceFor(AMP.win);
Expand Down Expand Up @@ -66,9 +68,6 @@ export class AmpAnalytics extends AMP.BaseElement {
* @private
*/
this.predefinedConfig_ = ANALYTICS_CONFIG;

/** @private @const Instance for testing. */
this.sha384_ = sha384;
}

/** @override */
Expand Down Expand Up @@ -415,13 +414,16 @@ export class AmpAnalytics extends AMP.BaseElement {
}
const key = this.expandTemplate_(spec['sampleOn'], trigger);

return urlReplacementsFor(this.getWin()).expand(key).then(key => {
const digest = this.sha384_(key);
if (digest[0] % 100 < spec['threshold']) {
return resolve;
}
return Promise.resolve(false);
});
const keyPromise = urlReplacementsFor(this.getWin()).expand(key);
const cryptoPromise = cryptoFor(this.getWin());
return Promise.all([keyPromise, cryptoPromise])
.then(results => {
const key = results[0];
const crypto = results[1];
return crypto.sha384(key);
}).then(digest => {
return Promise.resolve(digest[0] % 100 < spec['threshold']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return here, no need to return a Promise inside a #then block.

});
}

/**
Expand Down
89 changes: 44 additions & 45 deletions extensions/amp-analytics/0.1/cid-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ import {
} from '../../../src/url';
import {timer} from '../../../src/timer';
import {viewerFor} from '../../../src/viewer';
import {
sha384Base64,
} from '../../../third_party/closure-library/sha384-generated';
import {cryptoFor} from '../../../src/crypto';
import {user} from '../../../src/log';


Expand Down Expand Up @@ -68,20 +66,17 @@ class Cid {
/** @const */
this.win = win;

/** @private @const Instance for testing. */
this.sha384Base64_ = sha384Base64;

/**
* Cached base cid once read from storage to avoid repeated
* reads.
* @private {?string}
* @private {?Promise<string>}
*/
this.baseCid_ = null;

/**
* Cache to store external cids. Scope is used as the key and cookie value
* is the value.
* @private {!Object.<string, string>}
* @private {!Object<string, !Promise<string>>}
*/
this.externalCidCache_ = Object.create(null);
}
Expand Down Expand Up @@ -143,12 +138,13 @@ function getExternalCid(cid, getCidStruct, persistenceConsent) {
if (!isProxyOrigin(url)) {
return getOrCreateCookie(cid, getCidStruct, persistenceConsent);
}
return getBaseCid(cid, persistenceConsent).then(baseCid => {
return cid.sha384Base64_(
baseCid +
getProxySourceOrigin(url) +
getCidStruct.scope);
});
return Promise.all([getBaseCid(cid, persistenceConsent), cryptoFor(cid.win)])
.then(results => {
const baseCid = results[0];
const crypto = results[1];
return crypto.sha384Base64(
baseCid + getProxySourceOrigin(url) + getCidStruct.scope);
});
}

/**
Expand Down Expand Up @@ -183,7 +179,7 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
}

if (cid.externalCidCache_[scope]) {
return Promise.resolve(cid.externalCidCache_[scope]);
return cid.externalCidCache_[scope];
}

if (existingCookie) {
Expand All @@ -194,21 +190,24 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
return Promise.resolve(existingCookie);
}

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

cid.externalCidCache_[scope] = newCookie;
// Store it as a cookie based on the persistence consent.
persistenceConsent.then(() => {
// The initial CID generation is inherently racy. First one that gets
// consent wins.
const relookup = getCookie(win, scope);
if (!relookup) {
setCidCookie(win, scope, newCookie);
}
});
return Promise.resolve(newCookie);
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, scope);
if (!relookup) {
setCidCookie(win, scope, newCookie);
}
});
return cid.externalCidCache_[scope] = newCookiePromise;
}

/**
Expand Down Expand Up @@ -236,7 +235,7 @@ export function getProxySourceOrigin(url) {
*/
function getBaseCid(cid, persistenceConsent) {
if (cid.baseCid_) {
return Promise.resolve(cid.baseCid_);
return cid.baseCid_;
}
const win = cid.win;
const stored = read(win);
Expand All @@ -247,38 +246,38 @@ function getBaseCid(cid, persistenceConsent) {
// Once per interval we mark the cid as used.
store(win, stored.cid);
}
cid.baseCid_ = stored.cid;
return Promise.resolve(stored.cid);
return cid.baseCid_ = Promise.resolve(stored.cid);
}
// If we are being embedded, try to get the base cid from the viewer.
// Note, that we never try to persist to localStorage in this case.
const viewer = viewerFor(win);
if (viewer.isIframed()) {
return viewer.getBaseCid().then(cidValue => {
return cid.baseCid_ = viewer.getBaseCid().then(cidValue => {
if (!cidValue) {
throw new Error('No CID');
}
cid.baseCid_ = cidValue;
return cidValue;
});
}

// We need to make a new one.
const seed = getEntropy(win);
const newVal = cid.sha384Base64_(seed);
cid.baseCid_ = cryptoFor(win)
.then(crypto => crypto.sha384Base64(seed));

cid.baseCid_ = newVal;
// Storing the value may require consent. We wait for the respective
// promise.
persistenceConsent.then(() => {
// The initial CID generation is inherently racy. First one that gets
// consent wins.
const relookup = read(win);
if (!relookup) {
store(win, newVal);
}
});
return Promise.resolve(newVal);
Promise.all([cid.baseCid_, persistenceConsent])
.then(results => {
// The initial CID generation is inherently racy. First one that gets
// consent wins.
const newVal = results[0];
const relookup = read(win);
if (!relookup) {
store(win, newVal);
}
});
return cid.baseCid_;
}

/**
Expand Down Expand Up @@ -386,4 +385,4 @@ export function installCidService(window) {
return getService(window, 'cid', () => {
return new Cid(window);
});
};
}
50 changes: 50 additions & 0 deletions extensions/amp-analytics/0.1/crypto-impl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright 2016 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as lib from '../../../third_party/closure-library/sha384-generated';
import {getService} from '../../../src/service';

export class Crypto {

constructor(win) {
this.win = win;
}

/**
* Returns the SHA-384 hash of the input string in an ArrayBuffer.
* @param {string} str
* @returns {!Promise<!ArrayBuffer>}
*/
sha384(str) {
Copy link
Member

Choose a reason for hiding this comment

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

Types and docs, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were commenting on an old diff :-)

return Promise.resolve(lib.sha384(str));
}

/**
* Returns the SHA-384 hash of the input string in the format of web safe
* base64 string (using -_. instead of +/=).
* @param {string} str
* @returns {!Promise<string>}
*/
sha384Base64(str) {
return Promise.resolve(lib.sha384Base64(str));
}
}

export function installCryptoService(win) {
Copy link
Member

Choose a reason for hiding this comment

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

Forbid/whitelist usage with presubmit.

return getService(win, 'crypto', () => {
return new Crypto(win);
});
}
14 changes: 11 additions & 3 deletions extensions/amp-analytics/0.1/test/test-amp-analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import {ANALYTICS_CONFIG} from '../vendors';
import {AmpAnalytics} from '../amp-analytics';
import {Crypto} from '../crypto-impl';
import {instrumentationServiceFor} from '../instrumentation';
import {
installUserNotificationManager,
Expand Down Expand Up @@ -45,6 +46,7 @@ describe('amp-analytics', function() {
let sendRequestSpy;
let configWithCredentials;
let uidService;
let crypto;

const jsonMockResponses = {
'config1': '{"vars": {"title": "remote"}}',
Expand All @@ -68,6 +70,7 @@ describe('amp-analytics', function() {
installCidService(iframe.win);
installUrlReplacementsService(iframe.win);
uidService = installUserNotificationManager(iframe.win);

resetServiceForTesting(iframe.win, 'xhr');
getService(iframe.win, 'xhr', () => {
return {fetchJson: (url, init) => {
Expand All @@ -80,6 +83,10 @@ describe('amp-analytics', function() {
return Promise.resolve(JSON.parse(jsonMockResponses[url]));
}};
});

resetServiceForTesting(iframe.win, 'crypto');
crypto = new Crypto(iframe.win);
getService(iframe.win, 'crypto', () => crypto);
const link = document.createElement('link');
link.setAttribute('rel', 'canonical');
link.setAttribute('href', './test-canonical.html');
Expand Down Expand Up @@ -719,7 +726,7 @@ describe('amp-analytics', function() {
it('allows a request through', () => {
const analytics = getAnalyticsTag(getConfig(1));

sandbox.stub(analytics, 'sha384_').returns([0, 100, 0, 100]);
sandbox.stub(crypto, 'sha384').returns(Promise.resolve([0, 100, 0, 100]));
return waitForSendRequest(analytics).then(() => {
expect(sendRequestSpy.callCount).to.equal(1);
});
Expand All @@ -733,7 +740,8 @@ describe('amp-analytics', function() {
const urlReplacements = installUrlReplacementsService(
analytics.getWin());
sandbox.stub(urlReplacements, 'getReplacement_').returns(0);
sandbox.stub(analytics, 'sha384_').withArgs('0').returns([0]);
sandbox.stub(crypto, 'sha384')
.withArgs('0').returns(Promise.resolve([0]));
return waitForSendRequest(analytics).then(() => {
expect(sendRequestSpy.callCount).to.equal(1);
});
Expand All @@ -742,7 +750,7 @@ describe('amp-analytics', function() {
it('does not allow a request through', () => {
const analytics = getAnalyticsTag(getConfig(1));

sandbox.stub(analytics, 'sha384_').returns([1, 2, 3, 4]);
sandbox.stub(crypto, 'sha384').returns(Promise.resolve([1, 2, 3, 4]));
return waitForNoSendRequest(analytics).then(() => {
expect(sendRequestSpy.callCount).to.equal(0);
});
Expand Down
Loading