Closed
Bug 943521
Opened 11 years ago
Closed 11 years ago
Update desktop FXAccountsClient to use 'onepw' protocol
Categories
(Firefox :: Firefox Accounts, defect)
Firefox
Firefox Accounts
Tracking
()
RESOLVED
FIXED
Firefox 30
People
(Reporter: spenrose, Assigned: jedp)
References
Details
(Whiteboard: [qa-])
Attachments
(3 files, 10 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jedp
:
review+
ckarlof
:
feedback+
lsblakk
:
approval-mozilla-aurora-
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The FxA Server API is going to change, in large part to better support us. Conform to the result.
Updated•11 years ago
|
Whiteboard: [qa+]
Assignee | ||
Comment 1•11 years ago
|
||
fxa server issue: https://github.com/mozilla/fxa-auth-server/issues/344
Reporter | ||
Comment 2•11 years ago
|
||
Looks like this is going to be https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol.
Reporter | ||
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
+ more Android and desktop client devs
Comment 5•11 years ago
|
||
Is this bug specifically for Desktop or Android or both?
Comment 6•11 years ago
|
||
I assumed this was for desktop. For Android, see Bug 955808. Updating titles.
Updated•11 years ago
|
Summary: Update FXAccountsClient for new server API per https://github.com/mozilla/fxa-auth-server/issues/344 → Update desktop FXAccountsClient for new server API per https://github.com/mozilla/fxa-auth-server/issues/344
Comment 7•11 years ago
|
||
This is primarily to support login to FxOS devices.
Reporter | ||
Comment 8•11 years ago
|
||
The API, "onepw", is here:
https://github.com/mozilla/fxa-auth-server/tree/onepw
and will be rolled out to:
https://api-accounts.dev.lcip.org/v1
very soon, which will break all server connectivity. I will start work on updating FxAccountsClient.jsm now.
Assignee: nobody → spenrose
Reporter | ||
Comment 9•11 years ago
|
||
Prefing "identity.fxaccounts.auth.uri" to "https://api-accounts-legacy.dev.lcip.org" should get around this breakage for now.
Reporter | ||
Comment 10•11 years ago
|
||
The correct custom preference is:
user_pref("identity.fxaccounts.auth.uri", "https://api-accounts-legacy.dev.lcip.org/v1");
apologies for leaving off the "v1".
Assignee | ||
Comment 12•11 years ago
|
||
Sam, if you like, I can take this. I'm working on some FxAccountsClient bugs at the moment, so I'm already in this code.
Flags: needinfo?(spenrose)
Reporter | ||
Comment 13•11 years ago
|
||
We'll look at this one together tomorrow.
Flags: needinfo?(spenrose)
Assignee | ||
Updated•11 years ago
|
Assignee: spenrose → jparsons
Assignee | ||
Comment 14•11 years ago
|
||
This implements the onepw protocol in the FxAccountsClient. It follows the implementation of fxa-js-client closely.
Patch includes:
- credentials tests, incl warner's test vectors for the onepw protocol
- updated tests for FxAccountsClient
- some tweaks to CryptoUtils
Many thanks to :ckarlof for holding my hand for two hours today and helping me stumble past my clumsiness.
Mark, do you mind taking a look? Thanks!
j
Attachment #8370520 -
Flags: review?(mhammond)
Assignee | ||
Updated•11 years ago
|
Summary: Update desktop FXAccountsClient for new server API per https://github.com/mozilla/fxa-auth-server/issues/344 → Update desktop FXAccountsClient to use 'onepw' protocol
Comment 15•11 years ago
|
||
Comment on attachment 8370520 [details] [diff] [review]
Use onepw protocol in fxa client
Review of attachment 8370520 [details] [diff] [review]:
-----------------------------------------------------------------
Looking reasonable to me, but I think we'd also want review from someone who understand some of the crypto bits and who can also verify the tests are covering the important stuff.
::: services/common/utils.js
@@ +196,5 @@
> return [String.fromCharCode(byte) for each (byte in bytes)].join("");
> },
>
> + stringAsHex: function stringAsHex(str) {
> + return CommonUtils.bytesAsHex(CommonUtils.encodeUTF8(str));
I think |this| would be clearer here (and below)
@@ +203,2 @@
> bytesAsHex: function bytesAsHex(bytes) {
> + return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2)
nit: trailing space
::: services/fxaccounts/FxAccountsClient.jsm
@@ +71,5 @@
> * sessionToken: a session token
> * }
> */
> + signUp: function(email, password) {
> + return credentials.setup(email, password)
This file's style is already all over the place WRT promises, but I don't like this indentation - it makes it harder for my brain to read. I tend to prefer the .this(() => {\n type style, meaning each nested promise just gets a single indent level.
@@ +90,5 @@
> + }
> + );
> + },
> + (error) => {
> + throw error;
I don't think this error handler adds any value - IIUC, the behaviour would be the same without this (ie, it might make sense if you logged the error before throwing etc). Please correct me if I'm wrong about this :)
@@ +113,5 @@
> */
> signIn: function signIn(email, password) {
> + let hexEmail = CommonUtils.stringAsHex(email);
> + return credentials.setup(email, password)
> + .then(
ditto here re indentation and final throw.
@@ +179,5 @@
> accountKeys: function (keyFetchTokenHex) {
> let creds = this._deriveHawkCredentials(keyFetchTokenHex, "keyFetchToken");
> let keyRequestKey = creds.extra.slice(0, 32);
> let morecreds = CryptoUtils.hkdf(keyRequestKey, undefined,
> + kw("account/keys"), 3 * 32);
see comment in credentials.js about the naming of |kw| etc.
::: services/fxaccounts/credentials.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
I think this should be named Credentials.jsm (FxAccountsCommon.js sucks 'cos it doesn't use .jsm)
@@ +36,5 @@
> + HMAC_ALGORITHM: HMAC_ALGORITHM,
> + HMAC_LENGTH: HMAC_LENGTH,
> +};
> +
> +this.kw = function(context) {
can we give these better names? The names were slightly more palatable when the function was local to the module that used it, but now that fxAccountsClient calls a function called "kw" it's not at all obvious where it comes from. Another alternative that might be better still would be to have an object exported from here called, say, "CredentialsUtils" (or even directly on the credentials object below) - then seeing Credentials.kw(...) might not make much sense, but at least you know where it came from!
@@ +62,5 @@
> +this.kwe = function kwe(name, email) {
> + return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
> +};
> +
> +this.credentials = {
should be Credentials
@@ +95,5 @@
> + CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, kw("unwrapBkey"), hkdfLength);
> +
> + deferred.resolve(result);
> + } catch(err) {
> + deferred.reject(err);
What exceptions are expected here? It looks like it might make sense to log this before throwing it, probably via Cu.reportError("fail to blah: " + err);
::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +8,3 @@
>
> const FAKE_SESSION_TOKEN = "a0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebf";
> +const CU = CommonUtils;
not that keen on this name - too close to Cu. Is it even used?
@@ +122,4 @@
> let body = CommonUtils.readBytesFromInputStream(request.bodyInputStream);
> let jsonBody = JSON.parse(body);
> + // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#wiki-test-vectors
> + do_check_eq(jsonBody.email, "andré@example.org");
I don't understand these tests (but that's almost certainly my fault). I was expecting to see the addition of tests rather then just modifying existing ones.
::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-crypto/utils.js");
> +
> +const CU = CommonUtils;
ditto here re "CU" vs "Cu". If it is only used below, something like:
let {hexToBytes: h2b, hexAsString: h2s, ...} = CommonUtils;
should work (untested, but I *think* the order of the object-literal-like-thing is correct :)
@@ +172,5 @@
> + "quickStretchedPW is wrong");
> +
> + do_check_eq(expected.authPW, b2h(results.authPW),
> + "authPW is wrong");
> +
another space nit :)
Attachment #8370520 -
Flags: review?(mhammond) → feedback+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #15)
> Comment on attachment 8370520 [details] [diff] [review]
> Use onepw protocol in fxa client
>
> Review of attachment 8370520 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looking reasonable to me, but I think we'd also want review from someone who
> understand some of the crypto bits and who can also verify the tests are
> covering the important stuff.
Mark, thank you as always for your thorough reviews. I shall ask :warner for review of the crypto components, having modified the patch according to your feedback.
(:warner returns from PTO next Monday, I think)
> ::: services/common/utils.js
> @@ +196,5 @@
> > return [String.fromCharCode(byte) for each (byte in bytes)].join("");
> > },
> >
> > + stringAsHex: function stringAsHex(str) {
> > + return CommonUtils.bytesAsHex(CommonUtils.encodeUTF8(str));
>
> I think |this| would be clearer here (and below)
Hrm. This (no pun intended) will require restructuring this file a bit. I agree that it's kind of gross as it is, but I'm hesitant to make the change.
> @@ +203,2 @@
> > bytesAsHex: function bytesAsHex(bytes) {
> > + return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2)
>
> nit: trailing space
Sorry. Thanks.
> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +71,5 @@
> > * sessionToken: a session token
> > * }
> > */
> > + signUp: function(email, password) {
> > + return credentials.setup(email, password)
>
> This file's style is already all over the place WRT promises, but I don't
> like this indentation - it makes it harder for my brain to read. I tend to
> prefer the .this(() => {\n type style, meaning each nested promise just gets
> a single indent level.
I agree. I've updated this to be less of a Pyramid Of Doom. I hope the revised version is OK.
> @@ +90,5 @@
> > + }
> > + );
> > + },
> > + (error) => {
> > + throw error;
>
> I don't think this error handler adds any value - IIUC, the behaviour would
> be the same without this (ie, it might make sense if you logged the error
> before throwing etc). Please correct me if I'm wrong about this :)
You're right. I don't see the point of throwing this. And I don't see what error could occur. I'm removing the error handler.
> @@ +113,5 @@
> > */
> > signIn: function signIn(email, password) {
> > + let hexEmail = CommonUtils.stringAsHex(email);
> > + return credentials.setup(email, password)
> > + .then(
>
> ditto here re indentation and final throw.
Thank you. Yes.
> @@ +179,5 @@
> > accountKeys: function (keyFetchTokenHex) {
> > let creds = this._deriveHawkCredentials(keyFetchTokenHex, "keyFetchToken");
> > let keyRequestKey = creds.extra.slice(0, 32);
> > let morecreds = CryptoUtils.hkdf(keyRequestKey, undefined,
> > + kw("account/keys"), 3 * 32);
>
> see comment in credentials.js about the naming of |kw| etc.
Adjusted ...
> ::: services/fxaccounts/credentials.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
>
> I think this should be named Credentials.jsm (FxAccountsCommon.js sucks 'cos
> it doesn't use .jsm)
Better watch out or ima rewrite this whole thing in Dart. Just kidding. I've updated it to be a fresh jsm.
> @@ +36,5 @@
> > + HMAC_ALGORITHM: HMAC_ALGORITHM,
> > + HMAC_LENGTH: HMAC_LENGTH,
> > +};
> > +
> > +this.kw = function(context) {
>
> can we give these better names? The names were slightly more palatable when
> the function was local to the module that used it, but now that
> fxAccountsClient calls a function called "kw" it's not at all obvious where
> it comes from. Another alternative that might be better still would be to
> have an object exported from here called, say, "CredentialsUtils" (or even
> directly on the credentials object below) - then seeing Credentials.kw(...)
> might not make much sense, but at least you know where it came from!
I confess I took these names straight from the spec. And I double-confess that I initially found them confusing as well. I have followed your advice by both extending the names and placing the methods on the Credentials object. I hope it's clearer now.
> @@ +62,5 @@
> > +this.kwe = function kwe(name, email) {
> > + return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
> > +};
> > +
> > +this.credentials = {
>
> should be Credentials
Fixed.
> @@ +95,5 @@
> > + CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, kw("unwrapBkey"), hkdfLength);
> > +
> > + deferred.resolve(result);
> > + } catch(err) {
> > + deferred.reject(err);
>
> What exceptions are expected here? It looks like it might make sense to log
> this before throwing it, probably via Cu.reportError("fail to blah: " + err);
Yeah, I don't think we're expecting anything to blow up here. I've removed the error handler.
> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +8,3 @@
> >
> > const FAKE_SESSION_TOKEN = "a0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebf";
> > +const CU = CommonUtils;
>
> not that keen on this name - too close to Cu. Is it even used?
So right. And so not used. Removed.
> @@ +122,4 @@
> > let body = CommonUtils.readBytesFromInputStream(request.bodyInputStream);
> > let jsonBody = JSON.parse(body);
> > + // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#wiki-test-vectors
> > + do_check_eq(jsonBody.email, "andré@example.org");
>
> I don't understand these tests (but that's almost certainly my fault). I
> was expecting to see the addition of tests rather then just modifying
> existing ones.
In this case, we are replacing the /raw_password endpoints with new endpoints, but the overall logic remains the same. I've added some extra comments and checks in the test to make the flow clearer (especially as /accounts/login gets hit twice, once each by different checks).
I don't think we actually want new tests here; we just want the endpoints replaced and the checks for payload data to change a bit.
> ::: services/fxaccounts/tests/xpcshell/test_credentials.js
> @@ +5,5 @@
> > +Cu.import("resource://gre/modules/Promise.jsm");
> > +Cu.import("resource://services-common/utils.js");
> > +Cu.import("resource://services-crypto/utils.js");
> > +
> > +const CU = CommonUtils;
>
> ditto here re "CU" vs "Cu". If it is only used below, something like:
>
> let {hexToBytes: h2b, hexAsString: h2s, ...} = CommonUtils;
You rule. Done.
> should work (untested, but I *think* the order of the
> object-literal-like-thing is correct :)
>
> @@ +172,5 @@
> > + "quickStretchedPW is wrong");
> > +
> > + do_check_eq(expected.authPW, b2h(results.authPW),
> > + "authPW is wrong");
> > +
>
> another space nit :)
Argh. Sorry.
Thanks again for your comments and feedback. Revision forthcoming.
Cheers
j
Assignee | ||
Comment 17•11 years ago
|
||
f=markh
Hi, Brian, and welcome back! How's this look?
thanks
j
Attachment #8370520 -
Attachment is obsolete: true
Attachment #8370571 -
Flags: review?(warner-bugzilla)
Comment 18•11 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #16)
> > I think |this| would be clearer here (and below)
>
> Hrm. This (no pun intended) will require restructuring this file a bit. I
> agree that it's kind of gross as it is, but I'm hesitant to make the change.
I'm surprised by this (but I'm always surprised by |this| in javascript :) How are they called in a manner that screws this? But yeah, no big deal if it is a PITA.
Reporter | ||
Comment 19•11 years ago
|
||
Jed, and Mark, thank you so much for the excellent and timely work. Chris, can you think of an alternate reviewer? This bug is blocking a lot of progress for FxA on FxOS.
Flags: needinfo?(ckarlof)
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
Minor revisions forthcoming - some comment cleanup and normative pbkdf2 tests moved to common/crypto where they belong.
Comment 22•11 years ago
|
||
Comment on attachment 8370571 [details] [diff] [review]
Use onepw protocol in fxa client
Review of attachment 8370571 [details] [diff] [review]:
-----------------------------------------------------------------
Test coverage of /account/create would be nice, as well as tests for correct handling of error conditions for /account/create and /account/login: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md
However, I don't think the lack of tests should block these from landing. However, there are a few issues which need be fixed, e.g., accountExists is broken. r+ with the inline issues addressed and we need to have warner review this when he gets back.
::: services/common/utils.js
@@ +200,5 @@
> + return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2)
> + for (byte in bytes)].join("");
> + },
> +
> + stringAsHex: function stringAsHex(str) {
I assume this is new. Is there a test for this?
@@ +212,5 @@
> }
> return String.fromCharCode.apply(String, bytes);
> },
>
> + hexAsString: function hexAsString(hex) {
I assume this is new. Is there a test for this?
::: services/crypto/modules/utils.js
@@ +170,5 @@
> *
> * The output is an octet string of length dkLen, which you
> * can encode as you wish.
> */
> + pbkdf2Generate : function pbkdf2Generate(P, S, c, dkLen, hmacAlg="SHA1", hmacLen=20) {
It would be nice if we could default this to nsICryptoHMAC.SHA1 instead of this magic string.
::: services/fxaccounts/Credentials.jsm
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
Link to our protocol/API docs would be helpful here.
@@ +23,5 @@
> +const PBKDF2_ROUNDS = 1000;
> +const STRETCHED_PW_LENGTH_BYTES = 32;
> +const HKDF_SALT = h2b("00");
> +const HKDF_LENGTH = 32;
> +const HMAC_ALGORITHM = "SHA256";
It would be nicer if this was nsICryptoHMAC.SHA256
@@ +37,5 @@
> + HMAC_LENGTH: HMAC_LENGTH,
> +};
> +
> +this.Credentials = {
> + keyWrap: function(context) {
I'm not completely sure here, but I don't think "KW" stands for "keyWrap". I think it's "keyword".
@@ +44,5 @@
> + // Firefox Accounts API. For this reason, it is not exposed as a pref.
> + //
> + // See:
> + // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#creating-the-account
> + return h2b(s2h(PROTOCOL_VERSION + context));
s2b might be useful.
@@ +59,5 @@
> + * @return {bitArray} the salt combination with the namespace
> + *
> + * Exported for testing
> + */
> + keyWrapExtend: function(name, email) {
I'm not completely sure here, but I don't think "KWE" stands for "keyWrapExtend". I think it's "keyword encoding" or "keyword extend".
@@ +60,5 @@
> + *
> + * Exported for testing
> + */
> + keyWrapExtend: function(name, email) {
> + return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
s2b might be useful.
::: services/fxaccounts/FxAccountsClient.jsm
@@ +77,5 @@
> + email: creds.emailUTF8,
> + authPW: b2h(creds.authPW),
> + };
> + return this._request("/account/create", "POST", null, data).then((response) => {
> + return this.signIn(email, password)
You don't need to do this signIn call anymore in signUp. You get the session and keyFetch tokens directly back from the /account/create call now: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountcreate
@@ +103,5 @@
> * verified: flag indicating verification status of the email
> * }
> */
> signIn: function signIn(email, password) {
> + let hexEmail = CommonUtils.stringAsHex(email);
email isn't hex encoded in the API anymore. I think this line can be deleted because you seem to be doing the right thing below.
@@ +230,5 @@
> * @return Promise
> * The promise resolves to true if the account exists, or false
> * if it doesn't. The promise is rejected on other errors.
> */
> accountExists: function (email) {
accountsExists is going to fail because /auth/start is no longer part of the API. We don't have an "accountExists" API, so you'll have to change this /auth/start -> /account/login with the (non-hex encoded) email and an empty password. If you get a 102 the account doesn't exist and a 103 means that the account exists. This isn't ideal because too many hits against /account/login with the incorrect password will hit some throttling at some point.
::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +87,5 @@
> + // Simple test vectors from the interwebs
> + let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 20);
> + do_check_eq(b2h(result), "0c60c80f961f0e71f3a9b524af6012062fe037a6");
> +
> + let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 32, "SHA256", 32);
nsICryptoHMAC.SHA256 here and other places would be nicer.
Attachment #8370571 -
Flags: review?(warner-bugzilla) → review+
Comment 23•11 years ago
|
||
I reviewed on behalf of warner. We can land because it's greenfield and low risk, but he need to have warner review when he returns from vacation.
Flags: needinfo?(ckarlof)
Updated•11 years ago
|
Attachment #8370571 -
Flags: feedback?(warner-bugzilla)
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Chris Karlof [:ckarlof] from comment #22)
> Comment on attachment 8370571 [details] [diff] [review]
> Use onepw protocol in fxa client
>
> Review of attachment 8370571 [details] [diff] [review]:
> -----------------------------------------------------------------
This was a very helpful review. Thanks, Chris.
> Test coverage of /account/create would be nice, as well as tests for correct
> handling of error conditions for /account/create and /account/login:
> https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md
> However, I don't think the lack of tests should block these from landing.
> However, there are a few issues which need be fixed, e.g., accountExists is
> broken. r+ with the inline issues addressed and we need to have warner
> review this when he gets back.
All sounds good.
> ::: services/common/utils.js
> @@ +200,5 @@
> > + return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2)
> > + for (byte in bytes)].join("");
> > + },
> > +
> > + stringAsHex: function stringAsHex(str) {
>
> I assume this is new. Is there a test for this?
I've added a slew of services/common tests for these functions.
> @@ +212,5 @@
> > }
> > return String.fromCharCode.apply(String, bytes);
> > },
> >
> > + hexAsString: function hexAsString(hex) {
>
> I assume this is new. Is there a test for this?
Added.
> ::: services/crypto/modules/utils.js
> @@ +170,5 @@
> > *
> > * The output is an octet string of length dkLen, which you
> > * can encode as you wish.
> > */
> > + pbkdf2Generate : function pbkdf2Generate(P, S, c, dkLen, hmacAlg="SHA1", hmacLen=20) {
>
> It would be nice if we could default this to nsICryptoHMAC.SHA1 instead of
> this magic string.
Agreed. Fixed.
> ::: services/fxaccounts/Credentials.jsm
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> Link to our protocol/API docs would be helpful here.
Good point. Added it.
> @@ +23,5 @@
> > +const PBKDF2_ROUNDS = 1000;
> > +const STRETCHED_PW_LENGTH_BYTES = 32;
> > +const HKDF_SALT = h2b("00");
> > +const HKDF_LENGTH = 32;
> > +const HMAC_ALGORITHM = "SHA256";
>
> It would be nicer if this was nsICryptoHMAC.SHA256
Ditto.
> @@ +37,5 @@
> > + HMAC_LENGTH: HMAC_LENGTH,
> > +};
> > +
> > +this.Credentials = {
> > + keyWrap: function(context) {
>
> I'm not completely sure here, but I don't think "KW" stands for "keyWrap". I
> think it's "keyword".
I wasn't sure either. I couldn't remember, and it's not unpacked in the docs. I've gone for keyWord and keyWordExtended. If it's wrong, warner can tell us and we can patch it in a follow-up, I guess.
> @@ +44,5 @@
> > + // Firefox Accounts API. For this reason, it is not exposed as a pref.
> > + //
> > + // See:
> > + // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#creating-the-account
> > + return h2b(s2h(PROTOCOL_VERSION + context));
>
> s2b might be useful.
Yes indeed. Added.
> @@ +59,5 @@
> > + * @return {bitArray} the salt combination with the namespace
> > + *
> > + * Exported for testing
> > + */
> > + keyWrapExtend: function(name, email) {
>
> I'm not completely sure here, but I don't think "KWE" stands for
> "keyWrapExtend". I think it's "keyword encoding" or "keyword extend".
Ditto as above.
> @@ +60,5 @@
> > + *
> > + * Exported for testing
> > + */
> > + keyWrapExtend: function(name, email) {
> > + return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
>
> s2b might be useful.
Ditto as above.
> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +77,5 @@
> > + email: creds.emailUTF8,
> > + authPW: b2h(creds.authPW),
> > + };
> > + return this._request("/account/create", "POST", null, data).then((response) => {
> > + return this.signIn(email, password)
>
> You don't need to do this signIn call anymore in signUp. You get the session
> and keyFetch tokens directly back from the /account/create call now:
> https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-
> v1accountcreate
Oh great! Simplified.
> @@ +103,5 @@
> > * verified: flag indicating verification status of the email
> > * }
> > */
> > signIn: function signIn(email, password) {
> > + let hexEmail = CommonUtils.stringAsHex(email);
>
> email isn't hex encoded in the API anymore. I think this line can be deleted
> because you seem to be doing the right thing below.
Yes, that was an oversight. Removed.
> @@ +230,5 @@
> > * @return Promise
> > * The promise resolves to true if the account exists, or false
> > * if it doesn't. The promise is rejected on other errors.
> > */
> > accountExists: function (email) {
>
> accountsExists is going to fail because /auth/start is no longer part of the
> API. We don't have an "accountExists" API, so you'll have to change this
> /auth/start -> /account/login with the (non-hex encoded) email and an empty
> password. If you get a 102 the account doesn't exist and a 103 means that
> the account exists. This isn't ideal because too many hits against
> /account/login with the incorrect password will hit some throttling at some
> point.
Ah, ok. I've rewritten accountExists and revised the tests.
> ::: services/fxaccounts/tests/xpcshell/test_credentials.js
> @@ +87,5 @@
> > + // Simple test vectors from the interwebs
> > + let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 20);
> > + do_check_eq(b2h(result), "0c60c80f961f0e71f3a9b524af6012062fe037a6");
> > +
> > + let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 32, "SHA256", 32);
>
> nsICryptoHMAC.SHA256 here and other places would be nicer.
Ditto, as above.
As an extra detail, I've moved the pbkdf2 tests to the services/crypto tests, and filled them out with ietf test vectors for both sha1 and sha256. Sorting that out yesterday was too laborious not to leave a coherent test trail behind :)
Thanks for such a thorough read, Chris, and for all your time working together yesterday. Revised patch is building against inbound on my machine atm. I'll update it and check it in when it's done and all tests are still green.
Cheers
j
Assignee | ||
Comment 25•11 years ago
|
||
f=markh; r=ckarlof
Attachment #8370571 -
Attachment is obsolete: true
Attachment #8370571 -
Flags: feedback?(warner-bugzilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8371062 -
Flags: feedback?(warner-bugzilla)
Comment 26•11 years ago
|
||
Comment on attachment 8371062 [details] [diff] [review]
Use onepw protocol in fxa client
Review of attachment 8371062 [details] [diff] [review]:
-----------------------------------------------------------------
::: services/fxaccounts/FxAccountsClient.jsm
@@ +225,5 @@
> * The promise resolves to true if the account exists, or false
> * if it doesn't. The promise is rejected on other errors.
> */
> accountExists: function (email) {
> + return this.signIn(email, "").then(
I assume passing an empty string here doesn't cause anything to blow up?
Comment 27•11 years ago
|
||
I spoke with bsmith last night. He told me there was some magic we could perform using the PBKDF2 implementation in NSS that will allow us to get the resulting key out. This would yield a perf improvement, I believe.
Comment 28•11 years ago
|
||
Comment on attachment 8371062 [details] [diff] [review]
Use onepw protocol in fxa client
Review of attachment 8371062 [details] [diff] [review]:
-----------------------------------------------------------------
Hey Jed, I hope you don't mind me coming along and doing a drive-by review. I didn't really check any of the behavior, I just have a few style suggestions from a quick glance on the code.
::: services/fxaccounts/Credentials.jsm
@@ +18,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://services-crypto/utils.js");
> +Cu.import("resource://services-common/utils.js");
> +
> +let {stringToBytes: s2b, hexToBytes: h2b} = CommonUtils;
Nit: I think it would be better to keep the original names here instead of shortening them. The current code makes grepping or using MXR just harder when searching of occurrences of these somewhat basic functions.
@@ +31,5 @@
> +
> +this.constants = {
> + PROTOCOL_VERSION: PROTOCOL_VERSION,
> + PBKDF2_ROUNDS: PBKDF2_ROUNDS,
> + STRETCHED_PW_LENGTH_BYTES: STRETCHED_PW_LENGTH_BYTES,
Wouldn't it be nicer to define those in the "Credentials" object? Modules exporting only one symbol are a little easier to use and clients could just use |Credentials.HKDF_SALT| etc. It probably doesn't make sense to have the consts here and then maintain another list, just define them in the object once?
@@ +38,5 @@
> + HMAC_ALGORITHM: HMAC_ALGORITHM,
> + HMAC_LENGTH: HMAC_LENGTH,
> +};
> +
> +this.Credentials = {
Nit: this.Credentials = Object.freeze({
});
@@ +104,5 @@
> + CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, this.keyWord("unwrapBkey"), hkdfLength);
> +
> + deferred.resolve(result);
> +
> + return deferred.promise;
Nit: you don't actually need the deferred here. You can just do |return Promise.resolve(result)|.
::: services/fxaccounts/FxAccountsClient.jsm
@@ +14,5 @@
> Cu.import("resource://services-crypto/utils.js");
> Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +Cu.import("resource://gre/modules/Credentials.jsm");
> +
> +let b2h = CommonUtils.bytesAsHex;
Nit: same comment about the shortened function name.
::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +8,5 @@
> +
> +let {hexToBytes: h2b,
> + hexAsString: h2s,
> + stringAsHex: s2h,
> + bytesAsHex: b2h} = CommonUtils;
I don't mind so much doing that in a test but if the first thing in a file using those methods is to establish short names for them then maybe just rename the functions?
Comment 29•11 years ago
|
||
> Hey Jed, I hope you don't mind me coming along and doing a drive-by review.
Thanks Tim!
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to Chris Karlof [:ckarlof] from comment #26)
> Comment on attachment 8371062 [details] [diff] [review]
> Use onepw protocol in fxa client
>
> Review of attachment 8371062 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +225,5 @@
> > * The promise resolves to true if the account exists, or false
> > * if it doesn't. The promise is rejected on other errors.
> > */
> > accountExists: function (email) {
> > + return this.signIn(email, "").then(
>
> I assume passing an empty string here doesn't cause anything to blow up?
It doesn't blow anything up in my accountExists tests in test_client.js. I assume the server is just going to treat it as an ill-formed password?
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #28)
> Comment on attachment 8371062 [details] [diff] [review]
> Use onepw protocol in fxa client
>
> Review of attachment 8371062 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Hey Jed, I hope you don't mind me coming along and doing a drive-by review.
> I didn't really check any of the behavior, I just have a few style
> suggestions from a quick glance on the code.
I am always grateful for your feedback. Thanks for taking a look.
> ::: services/fxaccounts/Credentials.jsm
> @@ +18,5 @@
> > +Cu.import("resource://gre/modules/Promise.jsm");
> > +Cu.import("resource://services-crypto/utils.js");
> > +Cu.import("resource://services-common/utils.js");
> > +
> > +let {stringToBytes: s2b, hexToBytes: h2b} = CommonUtils;
>
> Nit: I think it would be better to keep the original names here instead of
> shortening them. The current code makes grepping or using MXR just harder
> when searching of occurrences of these somewhat basic functions.
That's a very good point. I'm glad you pointed that out. Fixed.
> @@ +31,5 @@
> > +
> > +this.constants = {
> > + PROTOCOL_VERSION: PROTOCOL_VERSION,
> > + PBKDF2_ROUNDS: PBKDF2_ROUNDS,
> > + STRETCHED_PW_LENGTH_BYTES: STRETCHED_PW_LENGTH_BYTES,
>
> Wouldn't it be nicer to define those in the "Credentials" object? Modules
> exporting only one symbol are a little easier to use and clients could just
> use |Credentials.HKDF_SALT| etc. It probably doesn't make sense to have the
> consts here and then maintain another list, just define them in the object
> once?
Agreed. Fixed.
> @@ +38,5 @@
> > + HMAC_ALGORITHM: HMAC_ALGORITHM,
> > + HMAC_LENGTH: HMAC_LENGTH,
> > +};
> > +
> > +this.Credentials = {
>
> Nit: this.Credentials = Object.freeze({
>
> });
Thanks. Done.
> @@ +104,5 @@
> > + CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, this.keyWord("unwrapBkey"), hkdfLength);
> > +
> > + deferred.resolve(result);
> > +
> > + return deferred.promise;
>
> Nit: you don't actually need the deferred here. You can just do |return
> Promise.resolve(result)|.
Ah yes. Fixed.
> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +14,5 @@
> > Cu.import("resource://services-crypto/utils.js");
> > Cu.import("resource://gre/modules/FxAccountsCommon.js");
> > +Cu.import("resource://gre/modules/Credentials.jsm");
> > +
> > +let b2h = CommonUtils.bytesAsHex;
>
> Nit: same comment about the shortened function name.
Fixed.
> ::: services/fxaccounts/tests/xpcshell/test_credentials.js
> @@ +8,5 @@
> > +
> > +let {hexToBytes: h2b,
> > + hexAsString: h2s,
> > + stringAsHex: s2h,
> > + bytesAsHex: b2h} = CommonUtils;
>
> I don't mind so much doing that in a test but if the first thing in a file
> using those methods is to establish short names for them then maybe just
> rename the functions?
My inclination is to let this stand as it is. I think the substitute names keep the lines short and more legible in the tests. But I don't feel comfortable just changing all the names in CommonUtils in this patch, as most of them are as I found them. Still, they could be shorter, more consistent (fooAsBar vs fooToBar), and in cases clearer ("Bytes" are not what I'd think of as bytes).
Perhaps such a clean-up would be appropriate for a follow-up bug? I'd be glad to do it. I just didn't want to muddy the waters any more than I have with this patch!
Many thanks again for your feedback.
Cheers,
j
Assignee | ||
Comment 32•11 years ago
|
||
I rejoice in seeing so many people giving helpful feedback and criticism on the FxA bugs. Thanks to all who have been watching this product and chipping in with advice. It's a healthy sign, and it makes it a pleasure to work on it.
Assignee | ||
Comment 33•11 years ago
|
||
Updated according to feedback from :ttaubert
Attachment #8371062 -
Attachment is obsolete: true
Attachment #8371062 -
Flags: feedback?(warner-bugzilla)
Attachment #8371134 -
Flags: feedback?(warner-bugzilla)
Comment 34•11 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #31)
> Perhaps such a clean-up would be appropriate for a follow-up bug? I'd be
> glad to do it. I just didn't want to muddy the waters any more than I have
> with this patch!
Yes, this would not warrant being handled here but rather in a follow-up. I actually don't think we should do that and keep the more legible function names. It's totally fine to shorten those in tests if it helps there. Thank you for addressing all the feedback!
Status: NEW → ASSIGNED
Assignee | ||
Comment 35•11 years ago
|
||
Testing this on a 2.3GHz laptop, with a CPU dedicated to the test, 1000 rounds of pbkdf2 take a whopping 3.5 seconds to compute (presumably due to the overhead of shoving data back and forth so many times across the XPCOM blood-brain barrier between the JavaScript PBKDF2 function on the one side and its C++ HMAC hasher on the other). This is going to be horrid on b2g.
ckarlof has spoken to bsmith about this, and bsmith says he can implement PBKDF2 HMAC SHA256 in NSS for us next week. I've opened Bug 968567 to track this.
Sam, given that this is a huge blocker for b2g, how do you want to proceed regarding the present patch?
a. Land it and just suck it up on b2g for a week? (slow dev, but unblocked)
b. Revise the patch to include an all-js implementation of pbkdf2 (e.g., sjcl), not landing it, but using it in dev to tide us over until Bug 968567 lands?
c. Try it first on b2g before passing judgment?
d. Something else?
Flags: needinfo?(spenrose)
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 37•11 years ago
|
||
Comment on attachment 8371134 [details] [diff] [review]
Use onepw protocol in fxa client
Review of attachment 8371134 [details] [diff] [review]:
-----------------------------------------------------------------
::: services/fxaccounts/Credentials.jsm
@@ +72,5 @@
> + * Note that PROTOCOL_VERSION does not refer in any way to the version of the
> + * Firefox Accounts API.
> + */
> + keyWordExtended: function(name, email) {
> + return stringToBytes(PROTOCOL_VERSION + name + ':' + email);
s/stringToBytes/CommonUtils.stringToBytes
Updated•11 years ago
|
Keywords: checkin-needed
Comment 38•11 years ago
|
||
I am testing this patch in b2g and I cannot create an account.
With the dev server I am getting:
Gecko I 1391692658109 FirefoxAccounts DEBUG Got content msg {"id":"9460b072-5032-3569-5db8-229eea1df6cf","data":{"method":"queryAccount","accountId":"ggggg@ggggg"}}
Gecko I 1391692658203 FirefoxAccounts DEBUG queryAccount ggggg@ggggg
GeckoConsole E Content JS WARN at app://system.gaiamobile.org/shared/js/l10n.js:79 in consoleWarn: [l10n] #fxa-connecting-to-firefox is undefined.
Gecko I 1391692660668 FirefoxAccounts DEBUG (Response) code: 400 - Status text: Bad Request
Gecko I 1391692660673 FirefoxAccounts DEBUG Clock offset vs https://api-accounts.dev.lcip.org/v1: -673
Gecko I 1391692660693 FirefoxAccounts DEBUG
Gecko I 1391692660694 FirefoxAccounts DEBUG Chrome event {"id":"9460b072-5032-3569-5db8-229eea1df6cf","data":{"registered":false}}
Gecko I 1391692666680 FirefoxAccounts DEBUG Got content msg {"id":"b44667f5-c8ed-7495-a428-c9617e929cbd","data":{"method":"signUp","accountId":"ggggg@ggggg","password":"11111111"}}
GeckoConsole E Content JS WARN at app://system.gaiamobile.org/shared/js/l10n.js:79 in consoleWarn: [l10n] #fxa-registering is undefined.
Gecko I 1391692669040 FirefoxAccounts DEBUG (Response) code: 200 - Status text: OK
Gecko I 1391692669041 FirefoxAccounts DEBUG Clock offset vs https://api-accounts.dev.lcip.org/v1: -1041
Gecko I 1391692669063 FirefoxAccounts ERROR INTERNAL_ERROR_INVALID_USER
Gecko I 1391692669071 FirefoxAccounts DEBUG Chrome event {"id":"b44667f5-c8ed-7495-a428-c9617e929cbd","error":{"error":"INTERNAL_ERROR_INVALID_USER","details":{"user":{"uid":"e34599af6d1a4e9bab366a95d01f1675"}}}}
With the legacy server:
Gecko I 1391693169403 FirefoxAccounts DEBUG Clock offset vs https://api-accounts-legacy.dev.lcip.org/v1: -403
Gecko I *************************
Gecko I A coding exception was thrown in a Promise rejection callback.
Gecko I Full message: ReferenceError: err is not defined
Gecko I See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Gecko I Full stack: this.FxAccountsClient.prototype.accountExists/<@resource://gre/modules/FxAccountsClient.jsm:240
Gecko I Handler.prototype.process@resource://gre/modules/Promise.jsm:770
Gecko I this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm:531
Gecko I *************************
GeckoConsole E Full Stack: JS frame :: resource://gre/modules/FxAccountsManager.jsm :: this.FxAccountsManager._serverError :: line 75
GeckoConsole E JS frame :: resource://gre/modules/FxAccountsManager.jsm :: this.FxAccountsManager.queryAccount/< :: line 306
GeckoConsole E JS frame :: resource://gre/modules/Promise.jsm :: Handler.prototype.process :: line 770
GeckoConsole E JS frame :: resource://gre/modules/Promise.jsm :: this.PromiseWalker.walkerLoop :: line 531
GeckoConsole E native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"]
Gecko I 1391693206193 FirefoxAccounts DEBUG (Response) code: 400 - Status text: Bad Request
Gecko I 1391693206194 FirefoxAccounts DEBUG Clock offset vs https://api-accounts-legacy.dev.lcip.org/v1: -194
Gecko I 1391693206197 FirefoxAccounts ERROR UNKNOWN_ERROR
Gecko I 1391693206204 FirefoxAccounts DEBUG Chrome event {"id":"31b3cec3-0869-b716-ab03-ab2c6313e336","error":{"error":"UNKNOWN_ERROR","details":{"code":400,"error":"Bad Request","message":"the value of email must match the RegExp /^(?:[a-fA-F0-9]{2})+40(?:[a-fA_F0-9]{2})+$/, the value of srp is not allowed to be undefined, the value of passwordStretching is not allowed to be undefined, the key (authPW) is not allowed","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format","validation":{"source":"payload","keys":["email","srp","passwordStretching","authPW"]},"errno":999}}}
GeckoConsole E [JavaScript Error: "TypeError: l10nKeys is undefined" {file: "app://system.gaiamobile.org/fxa/js/fxam_errors.js" line: 55}]
Comment 39•11 years ago
|
||
We *really* need mochitests :(
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #35)
> Testing this on a 2.3GHz laptop, with a CPU dedicated to the test, 1000
> rounds of pbkdf2 take a whopping 3.5 seconds to compute (presumably due to
> the overhead of shoving data back and forth so many times across the XPCOM
> blood-brain barrier between the JavaScript PBKDF2 function on the one side
> and its C++ HMAC hasher on the other). This is going to be horrid on b2g.
>
> ckarlof has spoken to bsmith about this, and bsmith says he can implement
> PBKDF2 HMAC SHA256 in NSS for us next week. I've opened Bug 968567 to track
> this.
>
Is this going to run in the main thread?
Assignee | ||
Comment 40•11 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #37)
> Comment on attachment 8371134 [details] [diff] [review]
> Use onepw protocol in fxa client
>
> Review of attachment 8371134 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: services/fxaccounts/Credentials.jsm
> @@ +72,5 @@
> > + * Note that PROTOCOL_VERSION does not refer in any way to the version of the
> > + * Firefox Accounts API.
> > + */
> > + keyWordExtended: function(name, email) {
> > + return stringToBytes(PROTOCOL_VERSION + name + ':' + email);
>
> s/stringToBytes/CommonUtils.stringToBytes
Holy cow. How did that even pass any of the tests??? kwe is repeatedly tested. Ugh.
Assignee | ||
Comment 41•11 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #39)
> We *really* need mochitests :(
Yes, we do.
> (In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from
> comment #35)
> > Testing this on a 2.3GHz laptop, with a CPU dedicated to the test, 1000
> > rounds of pbkdf2 take a whopping 3.5 seconds to compute (presumably due to
> > the overhead of shoving data back and forth so many times across the XPCOM
> > blood-brain barrier between the JavaScript PBKDF2 function on the one side
> > and its C++ HMAC hasher on the other). This is going to be horrid on b2g.
> >
> > ckarlof has spoken to bsmith about this, and bsmith says he can implement
> > PBKDF2 HMAC SHA256 in NSS for us next week. I've opened Bug 968567 to track
> > this.
> >
>
> Is this going to run in the main thread?
Good point. I will fix this.
Assignee | ||
Comment 42•11 years ago
|
||
ferjm, thanks for all this testing, and for catching the 'err' error in comment 38. afaik, this isn't going to be used by desktop, only b2g. does that mean we really want marionette tests?
Flags: needinfo?(ferjmoreno)
Comment 43•11 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #38)
> I am testing this patch in b2g and I cannot create an account.
>
> With the dev server I am getting:
Going forward, I would recommend using the prod FxA API server (https://api.accounts.firefox.com/v1). The role of the dev servers is being reconsidered.
-chris
Comment 44•11 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #42)
> ferjm, thanks for all this testing, and for catching the 'err' error in
> comment 38. afaik, this isn't going to be used by desktop, only b2g. does
> that mean we really want marionette tests?
I would start with mochitests for services/fxaccounts as suggested in Bug 967508. We should be able to mock the server behavior with server-side JavaScript [1]. Marionette tests would also be great for FxAccounts and the mozId API. We can ask the Gaia QA team to help with this [2]. We already have UI tests for the mozId API, we just need to update them again with the FxAccounts bits that were backed out.
[1] https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F
[2] https://groups.google.com/d/msg/mozilla.dev.b2g/f_LCBGv_InY/IO0sDFIZGv4J
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 45•11 years ago
|
||
WIP - updating tests
- revised test_client.js (xpcshell)
- separated all api tests
- one test per client api call
- exercise all error paths for each api call
working on mochitests now ...
Attachment #8371134 -
Attachment is obsolete: true
Attachment #8371134 -
Flags: feedback?(warner-bugzilla)
Assignee | ||
Comment 46•11 years ago
|
||
Oh - and all that pbkdf2 stuff happens on a separate thread now, per Comment 39
Assignee | ||
Comment 47•11 years ago
|
||
Verified with spenrose that the updated patch works on the production server, but using the desktop emulator.
I'm building for my hamachi now; if anyone has a build ready to go, and can throw this patch on, please pref identity.fxaccounts.loglevel="DEBUG" and try to sign in.
I want to know how long pbkdf2 takes on the device. grep for the string "Credentials set up after" to see how many ms it took for 1000 rounds of pbkdf2 with the current (slow) implementation.
Thanks, j
Comment 48•11 years ago
|
||
Comment on attachment 8371751 [details] [diff] [review]
Use onepw protocol in fxa client (WIP)
Review of attachment 8371751 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks Jed! I'll try to test this again.
::: services/fxaccounts/Credentials.jsm
@@ +129,5 @@
> + deferred.resolve(result);
> + }
> +
> + Services.tm.currentThread.dispatch(runnable,
> + Ci.nsIThread.DISPATCH_NORMAL);
If I am not wrong, 'currentThread' is still the main thread in this case, so you are just queuing the task on the main thread (i.e setTimeout). I am not sure if the ThreadManager can be used safely to create a new thread from JS :\, so we might need a chrome worker instead.
Comment 49•11 years ago
|
||
From [1]:
"Please note that as of Firefox 4, nsIThread.dispatch does not accept nsIRunnable-s implemented in Javascript and created on a different thread than the thread you're trying to dispatch the nsIRunnable to. This limitation was implemented to avoid crashes caused by changes especially to the Javascript strings implementation. This effectively means that javascript extensions cannot use the nsIThread API anymore to execute own jobs on different threads than the main thread. Consider Web/ChromeWorker as a replacement, which are severely limited in what you can do with them, or just don't use threads."
So, yeah, we need a chrome worker.
[1] https://wiki.mozilla.org/Performance/Addons/BestPractices#Consider_using_web_workers_and.2For_nsIThread
Reporter | ||
Comment 50•11 years ago
|
||
Can we break the chrome worker issue into a separate bug, or does it need to block the rest of the patch?
Assignee | ||
Comment 51•11 years ago
|
||
D'oh! Thanks, ferjm. Chrome worker it is.
Assignee | ||
Comment 52•11 years ago
|
||
(In reply to Sam Penrose from comment #50)
> Can we break the chrome worker issue into a separate bug, or does it need to
> block the rest of the patch?
I think it looks pretty straightforward to implement. I'm grabbing breakfast and coffee, and then I'll take a stab at it.
Assignee | ||
Comment 53•11 years ago
|
||
I take that back. This isn't straightforward at all.
XPCOM is not exposed to ChromeWorkers, so I see no way to use CryptoUtils in a worker.
Assignee | ||
Comment 54•11 years ago
|
||
Bug 608156 Comment 8 seems relevant:
> From the results outlined in bug 624485 comment 1, this is likely more trouble
> than it's worth. Because we decrypt one record per call, we're talking about
> thousands of small calls, none of which are long enough on their own to block
> the main thread for any meaningful amount of time. Without a bulk API in NSS,
> using ChromeWorker for these ops likely add even more overhead with little benefit.
> Instead, we're going to focus on making the crypto pieces much more efficient
> with the intent of reducing overall time spent in crypto.
Comment 55•11 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #54)
> Bug 608156 Comment 8 seems relevant:
Quite so.
> Without a bulk API in NSS,
> using ChromeWorker for these ops likely add even more overhead with little benefit.
Consider also serialization overhead from main thread to worker.
Workers only make sense inasmuch as you can put most of the feature in a worker -- reading directly from databases inside the worker, getting access to crypto from inside the worker, etc.
Otherwise you're just offloading a small chunk of work, and paying for it in messaging overhead.
See also Bug 649154 / Bug 721193, Bug 850711.
Assignee | ||
Comment 56•11 years ago
|
||
ferjm, spenrose, and I have spent the morning working on this.
Of this patch, we've concluded:
- Account creation and signin work properly on emulator and device
- Performance is ok, taking a little over a second on hamachi
- that means hamachi is 3x faster than tests on my mac! xpcshell-test overhead?
- Crypto work doesn't block UI
- Crypto performance should be solved the right way with Bug 968567
Consensus is that we should land this to unblock progress towards b2g 1.4.
Assignee | ||
Comment 57•11 years ago
|
||
Added a few more tests for pbkdf2 results because I guess I just love hex strings.
Attachment #8371751 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 58•11 years ago
|
||
In testing this patch, :ferjm discovered a problem with signing in while fxa was still polling for a different user's email. Sadly, there is a test in test_accounts.js to test this very case, but it appears not to be sufficient.
I have opened Bug 969546 for this issue.
Comment 59•11 years ago
|
||
Thanks for all the work that you've done here Jed!
Let's wait for the try push before pushing to inbound: https://tbpl.mozilla.org/?tree=Try&rev=5c918162e609
Assignee | ||
Comment 60•11 years ago
|
||
And thanks for all your input and help, as always, Fernando.
I hope the try push won't be much different from when I tried on Comment 20 (the patch hasn't changed much, in essence, since then, and all local tests pass). But you are wise to be cautious.
Awaiting the outcome.
Keywords: checkin-needed
Comment 61•11 years ago
|
||
Flags: in-testsuite+
Whiteboard: [qa+] → [qa+][fixed-in-fx-team]
Comment 62•11 years ago
|
||
Backed out for xpcshell failures.
https://hg.mozilla.org/integration/fx-team/rev/8e247297c978
https://tbpl.mozilla.org/php/getParsedLog.php?id=34313398&tree=Fx-Team
Assignee | ||
Comment 63•11 years ago
|
||
s/bang/whimper/
Assignee | ||
Comment 64•11 years ago
|
||
The report in comment 62 is:
14:32:06 WARNING - TEST-UNEXPECTED-FAIL | C:/slave/test/build/tests/xpcshell/tests/services/fxaccounts/tests/xpcshell/test_client.js | null == 102 - See following stack:
14:32:06 INFO - JS frame :: C:/slave/test/build/tests/xpcshell/tests/services/fxaccounts/tests/xpcshell/test_client.js :: test_accountExists :: line 481
Fantastic. I can't reproduce this failure locally on my mac.
Updated•11 years ago
|
Whiteboard: [qa+][fixed-in-fx-team] → [qa+]
Updated•11 years ago
|
OS: Gonk (Firefox OS) → All
Comment 65•11 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #64)
> Fantastic. I can't reproduce this failure locally on my mac.
Yeah, this failure is Windows-only.
Assignee | ||
Comment 66•11 years ago
|
||
Google search for "site:bugzilla.mozilla.org WindowsError: [Error 6] The handle is invalid." returns 23,000 results. Just sayin.
Comment 67•11 years ago
|
||
A local run on my Windows VM seems to confirm that there might be too many open connections. It isn't tied to the specific 102 return code test, everything after those requests fails.
Assignee | ||
Comment 68•11 years ago
|
||
Refactored accountExists test so we don't have multiple connections to the same server. Hoping this addresses the mysterious Windows failure.
Attachment #8372476 -
Attachment is obsolete: true
Assignee | ||
Comment 69•11 years ago
|
||
I suppose it would have helped had I run 'hg qrefresh' first.
Attachment #8372691 -
Attachment is obsolete: true
Assignee | ||
Comment 70•11 years ago
|
||
Yay, attachment 8372700 [details] [diff] [review] is looking good.
The "fix" was to break the accountExists test into three separate tests, each of which only uses a test http server once.
Profound thanks to ttaubert for helping debug a windows build at 3am localtime.
tbpl:
https://tbpl.mozilla.org/?tree=Try&rev=54f0fcfb301d
Assignee | ||
Comment 71•11 years ago
|
||
Oh bother. Still some windows tests failing.
Assignee | ||
Comment 72•11 years ago
|
||
It looks like the same failure pattern in test_accountKeys, where the http server is used for four successive requests.
Assignee | ||
Comment 73•11 years ago
|
||
Refactored accountKeys test into four separate tests to try to fix intermittent orange on Windows that appears to occur when http servers get too many requests.
https://tbpl.mozilla.org/?tree=Try&rev=f5ef41601a2e
Attachment #8372700 -
Attachment is obsolete: true
Assignee | ||
Comment 74•11 years ago
|
||
Bug 967372 reports an intermittent error with the test_hawk.js suite that looks like it's failing with the same pattern. (A test file with lots of separate http servers, Windows eventually complaining NS_ERROR_NOT_AVAILABLE, and then the test failing.)
Unfortunately, that test suite looks very much like my "fixed" suite in this current patch. That is, each server fields at most two requests. So I'm wondering if whether the solution is instead to ensure that the tests run sequentially, not concurrently as they do now.
Comment 75•11 years ago
|
||
The test also fails on Mac for me, although it succeeds in 20% of all runs. To make it fail all I need is test_accountKeys() and test_accountExists(). Removing one of them make the test succeed, so it looks like they somehow interfere with each other...
Too many requests in any of those two tests doesn't seem to be the issue, I can e.g. increase the number of client.accountExists() calls by 100x and it still succeeds.
Comment 76•11 years ago
|
||
Oh, and test_client.js does fail in sequential mode. I think we don't split up sub-tests anyway, only by file.
Comment 77•11 years ago
|
||
This seems all caused by multiple instances of:
add_task(function () {
yield deferredStop(server);
run_next_test();
});
add_task() does already call run_next_test() [1] so calling it again probably messes up the test harness somehow.
[1] https://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#1364
Comment 78•11 years ago
|
||
Re-pushed the original patch with those minor fixes as described in comment #77:
https://hg.mozilla.org/integration/fx-team/rev/6e77dfcf5c6e
Running in parallel, sequential and standalone mode all succeeded on the Mac and Windows VM.
Assignee | ||
Comment 79•11 years ago
|
||
w00t!
Thanks for digging in more, running the tests so many times, and pushing the patch with the fixes, Tim. This is great news!
Good catch in comment 77; I'll look for that in my other tests as well.
I noticed last night that the failure-case tests in this patch aren't all guaranteed to be executed, since they are in try-catch blocks, and if there's no exception, the test will just flow through them happily. In fact one of them was broken in this way. I fixed that in attachment 8372775 [details] [diff] [review], but forgot to mention it in comments. I'll file a follow-up bug to fix them.
Finally, as ckarlof said in Comment 23, we should ask warner to take a look at this when he's back from PTO next week.
Thanks, everyone, for all your comments and feedback!
Flags: needinfo?(warner-bugzilla)
Comment 80•11 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #79)
> I noticed last night that the failure-case tests in this patch aren't all
> guaranteed to be executed, since they are in try-catch blocks, and if
> there's no exception, the test will just flow through them happily. In fact
> one of them was broken in this way. I fixed that in attachment 8372775 [details] [diff] [review]
> [details] [diff] [review], but forgot to mention it in comments. I'll file
> a follow-up bug to fix them.
Oh, sorry. I didn't know that there's some more fixes in the patch than just splitting tests into multiple ones. Thanks for filing the follow-up!
Assignee | ||
Comment 81•11 years ago
|
||
I've opened Bug 969892 for the test suite adjustments
Comment 82•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
Component: Identity → FxA
Comment 83•11 years ago
|
||
Comment on attachment 8372775 [details] [diff] [review]
Use onepw protocol in fxa client
Review of attachment 8372775 [details] [diff] [review]:
-----------------------------------------------------------------
Crypto looks fine to me: it'd be nice to fix those two salt values, but it's not a blocker. So sorry I took so long on this!
::: services/fxaccounts/Credentials.jsm
@@ +23,5 @@
> +
> +const PROTOCOL_VERSION = "identity.mozilla.com/picl/v1/";
> +const PBKDF2_ROUNDS = 1000;
> +const STRETCHED_PW_LENGTH_BYTES = 32;
> +const HKDF_SALT = CommonUtils.hexToBytes("00");
Does this mean the default salt is a single NUL byte (length=1)? The spec calls for an empty string (length=0). It turns out this is fine (as verified by your test vectors), because HMAC pads short keys (anything shorter than the hash size) with zero bytes, so key="" and key="\x00" and key="\x00\x00\x00" will all behave the same way. But it might be nice to change this to hexToBytes(""), if that's legal, for the benefit of future readers.
@@ +88,5 @@
> + *
> + * Note that PROTOCOL_VERSION does not refer in any way to the version of the
> + * Firefox Accounts API.
> + */
> + keyWordExtended: function(name, email) {
It doesn't matter, but the "E" in "KWE" actually stands for "email" :)
::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +52,5 @@
> + let authKeyInfo = Credentials.keyWord('authPW');
> + do_check_eq(b2h(authKeyInfo), "6964656e746974792e6d6f7a696c6c612e636f6d2f7069636c2f76312f617574685057");
> +
> + // derive auth password
> + let hkdfSalt = h2b("00");
ditto: h2b("") would be more precise
@@ +100,5 @@
> +
> + do_check_eq(expected.quickStretchedPW, b2h(results.quickStretchedPW),
> + "quickStretchedPW is wrong");
> +
> + do_check_eq(expected.authPW, b2h(results.authPW),
excellent test vectors!
Attachment #8372775 -
Flags: review+
Comment 84•11 years ago
|
||
Comment on attachment 8372775 [details] [diff] [review]
Use onepw protocol in fxa client
oops, set the wrong flag
Attachment #8372775 -
Flags: review+
Flags: needinfo?(warner-bugzilla)
Comment 85•11 years ago
|
||
This bug is blocking me from getting code changes related to TPS on bug 966434 landed on Aurora. Can we please get this backported?
I also request blocking 29.0 for that given that it will not allow us any testing.
Comment 86•11 years ago
|
||
Sorry, it looks like this is a complete change of the underlying protocol and which may not make it into Firefox 29.0. If that is the case I will have to update my patches to work with the older authentication protocol.
Updated•11 years ago
|
Comment 87•11 years ago
|
||
Also asking info from jed. Would it be possible to backport or is it too dangerous and/or not wanted?
Flags: needinfo?(jparsons)
Comment 88•11 years ago
|
||
Henrik, the "old protocol" isn't supported anymore in the server, so "updating your patches to work with the older auth protocol isn't going to work". I had forgotten this patch wasn't uplifted to aurora. At the didn't impact the Sync 29 effort (but now it does).
Flags: needinfo?(ckarlof)
Comment 89•11 years ago
|
||
Thank you Chris. Given that both bugs I have referenced are blockers for Firefox 29, this should also become one. Asking for tracking firefox-29.
status-firefox29:
--- → affected
tracking-firefox29:
--- → ?
Updated•11 years ago
|
Assignee | ||
Comment 90•11 years ago
|
||
Comment on attachment 8372775 [details] [diff] [review]
Use onepw protocol in fxa client
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 943521
User impact if declined:
Firefox Accounts and Sync won't work
Sorrow and gnashing of teeth
Testing completed (on m-c, etc.): fx-team, m-i, and m-c
Risk to taking this patch (and alternatives if risky):
Risk is low; it's been in use with active QA for a month
No alternatives. The Firefox Accounts auth servers have all
been updated to use the protocol this patch supports
String or IDL/UUID changes made by this patch: none
Attachment #8372775 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(jparsons)
Comment 91•11 years ago
|
||
This patch didn't apply for me on aurora: https://bugzilla.mozilla.org/show_bug.cgi?id=981921#c19
Assignee | ||
Comment 92•11 years ago
|
||
Drat. Ok, getting a fresh aurora checkout. Will upload a rebased patch.
Assignee | ||
Comment 93•11 years ago
|
||
Think I got it sorted out. Building now in order to test.
Assignee | ||
Comment 94•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8372775 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 95•11 years ago
|
||
Comment on attachment 8391487 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 943521
User impact if declined:
Firefox Accounts and Sync won't work
Sorrow and gnashing of teeth
Testing completed (on m-c, etc.): fx-team, m-i, and m-c
Risk to taking this patch (and alternatives if risky):
Risk is low; it's been in use with active QA for a month
No alternatives. The Firefox Accounts auth servers have all
been updated to use the protocol this patch supports
String or IDL/UUID changes made by this patch: none
The attachment whose title begins with [aurora rebase] is the one we want
Attachment #8391487 -
Flags: approval-mozilla-aurora?
Comment 96•11 years ago
|
||
Jed, this bug is marked as tracking firefox-29. So no approval request of the attachment is necessary. But you most likely should ask for review before getting it landed. Thanks for fixing it that quickly. If it doesn't regress anything I will backport my patches most likely on Sunday.
Comment 97•11 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #96)
> Jed, this bug is marked as tracking firefox-29. So no approval request of
> the attachment is necessary.
That's not true. Requesting approval is the correct course of action.
Assignee | ||
Comment 98•11 years ago
|
||
Comment on attachment 8391487 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth
Oh! Good to know. I asked around and got the impression that this was enough. :rnewman, can I ask you for a sanity-check review of this patch?
Thanks
j
Attachment #8391487 -
Flags: review?(rnewman)
Comment 99•11 years ago
|
||
Comment on attachment 8391487 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth
Review of attachment 8391487 [details] [diff] [review]:
-----------------------------------------------------------------
Skimming, this looks fine; I recommend manual testing, though!
rs=me
::: services/fxaccounts/FxAccountsClient.jsm
@@ +154,5 @@
> * @return Promise
> * Returns a promise that resolves to an object:
> * {
> + * kA: an encryption key for recevorable data (bytes)
> + * wrapKB: an encryption key that requires knowledge of the
Pah trailing whitespace.
::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +217,5 @@
> + do_throw("Expected an error");
> + } catch(expectedError) {
> + do_check_eq(101, expectedError.errno);
> + }
> +
ws
@@ +221,5 @@
> +
> + yield deferredStop(server);
> + run_next_test();
> +});
> +
ws
@@ +509,5 @@
> + response.bodyOutputStream.write(errorMessage, errorMessage.length);
> + return;
> + },
> + });
> +
ws
Attachment #8391487 -
Flags: review?(rnewman) → review+
Comment 100•11 years ago
|
||
I agree with rnewman, this should get a manual test of FxA Sync. gavin had similar concerns.
Assignee | ||
Comment 101•11 years ago
|
||
r=rnewman
Attachment #8391487 -
Attachment is obsolete: true
Attachment #8391487 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 102•11 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #99)
> Comment on attachment 8391487 [details] [diff] [review]
> [aurora rebase] use onepw protocol for auth
>
> Review of attachment 8391487 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Skimming, this looks fine; I recommend manual testing, though!
>
> rs=me
Thanks, Richard,
Sorry about the whitespace. My text editor is doing something wonky.
Testing manually now.
Assignee | ||
Comment 103•11 years ago
|
||
I tested as follows:
- Go to about:accounts and create a new account
- Verify account
- Manage sync
- Disconnect
- Sign in again
And a sync:
- Configure logOnSuccess to true
- Open some new tabs
- Choose Tools -> Sync Now
- See success logs in about:sync-log as expected
Assignee | ||
Comment 104•11 years ago
|
||
Comment on attachment 8391600 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 943521
User impact if declined:
Firefox Accounts and Sync won't work
Sorrow and gnashing of teeth
Testing completed (on m-c, etc.):
fx-team, m-i, and m-c
Confirmed with manual testing as described in Comment 103
Risk to taking this patch (and alternatives if risky):
Risk is low; it's been in use with active QA for a month
No alternatives. The Firefox Accounts auth servers have all
been updated to use the protocol this patch supports
String or IDL/UUID changes made by this patch:
none
The attachment whose title begins with [aurora rebase] is the one we want
rs=rnewman
Attachment #8391600 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8391600 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 105•11 years ago
|
||
Jed, I hope you are ok that I pushed this for you. We have a hard time-line and I want to get my stuff landed today before the merge.
https://hg.mozilla.org/releases/mozilla-aurora/rev/e85435556aaa
Comment 106•11 years ago
|
||
Hm, Jed any reason why the whole retry logic for SignIn() hasn't been implemented in the backport patch? I have seen this now while trying to get my patches to work.
https://hg.mozilla.org/mozilla-central/file/25cfa01ba054/services/fxaccounts/FxAccountsClient.jsm#l105
https://hg.mozilla.org/releases/mozilla-aurora/file/default/services/fxaccounts/FxAccountsClient.jsm#l103
Is that really wanted? I see loss of functionality here.
Flags: needinfo?(jparsons)
Comment 107•11 years ago
|
||
Also when you apply attachment 8387572 [details] [diff] [review] on top of your patch, it will fail with:
"ASSERTION FAILED! Weave status OK; expected "success.status_ok", got "service.client_not_configured"
And this *after* the 'fxaccounts:onlogin' and 'weave:service:setup-complete' observer notifications have been sent. On m-c it is working fine. Do we still have a core bug in here?
Comment 108•11 years ago
|
||
Revoking tracking flag for now, given that a follow-up or back-out and new fix seem to be necessary.
Comment 109•11 years ago
|
||
As result the 'weave:engine:start-tracking' observer notification is not send, which we are waiting for in tps to start the tests.
Comment 110•11 years ago
|
||
So the problem here is that result.email is undefined (not returned by the server) and on m-c we set it to data.email. To keep the API definition we should get this updated. I have a patch in a bit for review.
Comment 111•11 years ago
|
||
This updates the signIn() method and makes it conform to our API as used on m-c. Tests for that change, and the jsdoc updates are included in my patch on bug 981921 which is waiting for this patch to be landed first.
Attachment #8392184 -
Flags: review?(ckarlof)
Assignee | ||
Comment 112•11 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #106)
> Hm, Jed any reason why the whole retry logic for SignIn() hasn't been
> implemented in the backport patch? I have seen this now while trying to get
> my patches to work.
>
> https://hg.mozilla.org/mozilla-central/file/25cfa01ba054/services/fxaccounts/
> FxAccountsClient.jsm#l105
> https://hg.mozilla.org/releases/mozilla-aurora/file/default/services/
> fxaccounts/FxAccountsClient.jsm#l103
Yes, I asked :ckarlof about this in IRC on Friday, namely whether we should push for uplift of Bug 963835, which implements the retry logic on incorrect email case capitalization. He said no, it's not needed for Firefox 29, adding that this patch really benefits FirefoxOS anyway.
Flags: needinfo?(jparsons)
Assignee | ||
Comment 113•11 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #110)
> So the problem here is that result.email is undefined (not returned by the
> server) and on m-c we set it to data.email. To keep the API definition we
> should get this updated. I have a patch in a bit for review.
I see
Comment 114•11 years ago
|
||
Ok, this implements only the updating piece for the email address, but leaves out the capitalization correction, which is too large to be backported at this stage. Please notice that the jsdoc will be updated with my other patch.
Attachment #8392184 -
Attachment is obsolete: true
Attachment #8392184 -
Flags: review?(ckarlof)
Attachment #8392334 -
Flags: review?(jparsons)
Attachment #8392334 -
Flags: feedback?(ckarlof)
Updated•11 years ago
|
Attachment #8392334 -
Flags: feedback?(ckarlof) → feedback+
Assignee | ||
Comment 115•11 years ago
|
||
Comment on attachment 8392334 [details] [diff] [review]
[beta] Make signIn() method API conform v2
Looks good!
I also did manual testing on latest desktop aurora, just out of extra paranoia :)
Attachment #8392334 -
Flags: review?(jparsons) → review+
Comment 116•11 years ago
|
||
Comment on attachment 8392334 [details] [diff] [review]
[beta] Make signIn() method API conform v2
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Code missed in last backport patch
User impact if declined: Only automation is affected
Testing completed (on m-c, etc.): yes, for a long time.
Risk to taking this patch (and alternatives if risky): None, fixes the API compared to m-c
String or IDL/UUID changes made by this patch: None
Attachment #8392334 -
Flags: approval-mozilla-aurora?
Comment 117•11 years ago
|
||
Comment on attachment 8392334 [details] [diff] [review]
[beta] Make signIn() method API conform v2
[Triage Comment]
Post-merge, we're now needing this on Beta for 29.
Attachment #8392334 -
Flags: approval-mozilla-beta+
Attachment #8392334 -
Flags: approval-mozilla-aurora?
Attachment #8392334 -
Flags: approval-mozilla-aurora-
Updated•11 years ago
|
Attachment #8392334 -
Attachment description: [aurora] Make signIn() method API conform v2 → [beta] Make signIn() method API conform v2
Comment 118•11 years ago
|
||
I'm not around today, so could someone please land this on beta? Thanks.
Keywords: checkin-needed
Comment 119•11 years ago
|
||
Landed follow-up patch on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/32b0612b72e7
Keywords: checkin-needed
Comment 120•11 years ago
|
||
Is any additional manual testing required here? If that is the case, could you please let me know if the steps specified in Comment 103 still represent a viable way of verifying the fix?
Flags: needinfo?(jparsons)
Comment 121•11 years ago
|
||
This code is in use in our tps framework to run the sync tests. All is working fine across branches. So I don't think we need manual testing here.
Comment 122•11 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #121)
> This code is in use in our tps framework to run the sync tests. All is
> working fine across branches. So I don't think we need manual testing here.
Thank you for following up on this, Henrik! Marking it as [qa-] based on Comment 121 - Jed, please flip this back if you disagree.
Whiteboard: [qa+] → [qa-]
Comment 124•10 years ago
|
||
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #46)
> Oh - and all that pbkdf2 stuff happens on a separate thread now, per Comment
> 39
I'm looking at bug 1073639 and found myself here. Note that there isn't a different thread here - Services.tm.currentThread.dispatch simply waits for an event-loop spin before running the function on the same thread. AFAIK, the only way for JS to get a new thread is to use a worker. The log message:
0:05.75 Identity.FxAccounts DEBUG Dispatched thread for credentials setup crypto work
is also misleading in this case.
It sounds like a new bug is warranted - but I'm not sure if it is "*really* use a thread" or "make the log message less misleading" :) JedP, do you mind finding out what followup makes sense?
Flags: needinfo?(jed+bmo)
Reporter | ||
Comment 125•10 years ago
|
||
Hey Mark --
Jed has left the building. I have inherited responsibility for this code, and honestly I have no idea what the cross-platform implications of introducing a thread here are. My guess is that we should try it but it would be nice to get insight from someone who has handled memory- and CPU-intensive work on platform before. Any nominations?
Flags: needinfo?(jed+bmo)
Updated•7 years ago
|
Product: Core → Firefox
Updated•7 years ago
|
Target Milestone: mozilla30 → Firefox 30
You need to log in
before you can comment on or make changes to this bug.
Description
•