Introduces CryptoService as a wrapper around sha384 lib. #3841
Conversation
…eparation for a later refactory to browser native crypto API.
| const keyPromise = urlReplacementsFor(this.getWin()).expand(key); | ||
| const cryptoPromise = cryptoFor(this.getWin()); | ||
| return Promise.all([keyPromise, cryptoPromise]) | ||
| .then(([key, crypto]) => { |
There was a problem hiding this comment.
Yes. Seems working, at least under chrome. Is it something new?
On Jun 29, 2016 5:09 PM, "Malte Ubl" notifications@github.com wrote:
In extensions/amp-analytics/0.1/amp-analytics.js
#3841 (comment):@@ -415,13 +414,14 @@ 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(([key, crypto]) => {Wow, this works?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/pull/3841/files/ad4493954bc4e6803beefa696a9284258ecd4f00#r69050419,
or mute the thread
https://github.com/notifications/unsubscribe/ANd2kIU8IoiJL-4whBCC0Pyo8dEMUOxKks5qQwlMgaJpZM4JBrfz
.You received this message because you are subscribed to the Google Groups
"amphtml-eng-github" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to amphtml-eng-github+unsubscribe@google.com.
To post to this group, send email to amphtml-eng-github@google.com.
To view this discussion on the web visit
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/3841/r69050419%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/3841/r69050419%40github.com?utm_medium=email&utm_source=footer
.
There was a problem hiding this comment.
the output this will produce is pretty bad btw, closure compiler will start using Symbol's and iterator protocol. (even babel will use sliceIterator, unless in loose mode). any destructuring will produce this
There was a problem hiding this comment.
There was a problem hiding this comment.
Good to know. Fixed. Thanks!
| * @param {!Window} window | ||
| * @return {!Promise<!Crypto>} | ||
| */ | ||
| export function cryptoFor(window) { |
There was a problem hiding this comment.
Forbid/whitelist usage with presubmit.
There was a problem hiding this comment.
What type of presubmit check for the usage of 'cryptoFor'?
There was a problem hiding this comment.
There was a problem hiding this comment.
check for installCryptoService is already added.
I thought you were also asking for a check for cryptoFor()
|
Nice! |
lannka
changed the title
|
Ready for review now. PTAL |
| }); | ||
| return Promise.resolve(newCookie); | ||
| Promise.all([newCookiePromise, persistenceConsent]) | ||
| .then(([newCookie]) => { |
There was a problem hiding this comment.
Here and elsewhere are some more instances of the magic de-composition. We should catch this in the linter if the generated code sucks. Really sad.
|
LGTM |
| const crypto = results[1]; | ||
| return crypto.sha384(key); | ||
| }).then(digest => { | ||
| return Promise.resolve(digest[0] % 100 < spec['threshold']); |
There was a problem hiding this comment.
Just return here, no need to return a Promise inside a #then block.
This is a preparation for a later refactory to browser native crypto API. #3690