935232 - create FxAccountClient.jsm module

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Brian Warner [:warner :bwarner]]

The FxAccount-specific parts of HAWK.jsm should be moved out to its own module, named FxAccountClient.jsm in services/fxaccounts/ . This should use the HAWK code from the other half of HAWK.jsm (which itself will move to services/common/ in bug 930598).

This module should take a server URL and have methods like "recoveryEmailStatus", "accountKeys", and "signCertificate", all of which should return promises.

Comment 1

[Sam Penrose]

Sketching signUp and signIn methods in zaach/warnerhack .

Comment 2

[Sam Penrose]

Attachment: paste_fxaccounts.txt

This is not a patch, but it contains the signUp and signIn methods and a est for the two of them

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Interestingly, the atob and btoa functions are undefined in xpcshell tests on b2g, though they are present on browser builds.  I have no idea how this is even possible.  Bug 643212 is about atob and btoa, so moving the discussion there.

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Firefox Accounts client, storage, and hawk

Pulled from zaach's rebase [1] of warner's fork [2] of mhammond's clone [3] of elm, with a few manual patch tweaks by jedp.  Lovingly applied against m-c.

[1] https://github.com/zaach/mozilla-central/commits/warnerhack
[2] https://github.com/warner/mozilla-central/tree/warnerhack
[3] https://github.com/mhammond/mozilla-central/tree/experiment/elm-fxaccount-sync

Built for browser so far; all services tests green on local run.

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Push to try:

https://tbpl.mozilla.org/?tree=Try&rev=bb3785ba98e4

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use request feedback, please) [:jedp, :jparsons] from comment #3)
> Interestingly, the atob and btoa functions are undefined in xpcshell tests
> on b2g, though they are present on browser builds.  I have no idea how this
> is even possible.  Bug 643212 is about atob and btoa, so moving the
> discussion there.

I cannot check it right now, but they should be there after bug 809218

Comment 7

[Chris Karlof [:ckarlof]]

This bug was originally scoped to the network API client, but the patch also seems to be introducing the FxA module/service for managing logged in state. Was that intentional?

Comment 8

[Chris Karlof [:ckarlof]]

Perhaps we should land the FxAccounts.jsm component in bug 909967?

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Chris, yes, we ought to factor this patch out into smaller, coherent bits.  If you want to re-open bug 909967 and land FxAccounts.jsm there, I'm game (as long as we land m-c first and then merge into elm afterward :)

Comment 10

[Zachary Carter [:zaach]]

Attachment: FxAccountsClient.jsm

This removes the FxAccounts.jsm bits.

Comment 12

[Chris Karlof [:ckarlof]]

Attachment: Updated existing patch to include hkdf tests from bug 930598

Added hkdf tests from duplicate bug 930598.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 831733 [details] [diff] [review]
Updated existing patch to include hkdf tests from bug 930598

Review of attachment 831733 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me apart from a few nits I've mentioned.  Given my eyes glossed over in some of the crypto related functions, I think this should also be reviewed by someone from the sec team.

::: services/common/tests/unit/head_helpers.js
@@ +36,5 @@
>  }
>  
> +function do_check_throws_message(aFunc, aResult, aStack) {
> +  if (!aStack) {
> +    try {

I notice that do_check_eq already has code to fetch the stack - but it isn't guarded by the exception handler.  I'd be inclined to assume the code in do_check_eq works, and if it fails due to a missing |Components|, fix it there.  Same goes for the existing do_check_throws I guess.

@@ +48,5 @@
> +  } catch (e) {
> +    do_check_eq(e.message, aResult, aStack);
> +    return;
> +  }
> +  do_throw("Expected result " + aResult + ", none thrown.", aStack);

I'd have thought the default message from do_check_eq would be better - the specific message being thrown here doesn't include |e.message|.  ie, I'd think just plain do_check_eq() would be enough.

::: services/crypto/modules/utils.js
@@ +149,4 @@
>      let T = "";
>      let Tn = "";
>      let iterations = Math.ceil(len/BLOCKSIZE);
> +    //assert iterations <= 255;

this line should either be removed, or some comment added indicated why we'd *like* to throw in that case but currently can not.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +31,5 @@
> +    let uuidPromise = this._request("/raw_password/account/create", "POST", null,
> +                          {email: hexEmail, password: password});
> +    let self = this;
> +
> +    uuidPromise.then(function(result) {

nit: I believe using a fat arrow function would avoid needing to use |self|

::: services/fxaccounts/tests/xpcshell/head.js
@@ +8,5 @@
> +(function initFxAccountsTestingInfrastructure() {
> +  do_get_profile();
> +
> +  let ns = {};
> +  Components.utils.import("resource://testing-common/services-common/logging.js",

Use Cu.import?

@@ +28,5 @@
> + *        Components#stack#caller.
> + */
> +function do_check_throws(func, message, stack)
> +{
> +  if (!stack)

similar to before, there should be no need to fetch a default stack here.

@@ +41,5 @@
> +    do_throw("expecting exception '" + message
> +             + "', caught '" + exc.message + "'", stack);
> +  }
> +
> +  if (message) {

this seems strange - it means someone can call do_check_throws without specifying a value for "message", and it really means "check the function doesn't fail"?  ie, the "if (message)" check here seems wrong.

@@ +46,5 @@
> +    do_throw("expecting exception '" + message + "', none thrown", stack);
> +  }
> +}
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

I'd be inclined to add this import to the top.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +11,5 @@
> +}
> +
> +function deferredStop(server) {
> +    let deferred = Promise.defer();
> +    server.stop(function () {

nit: given the result of this promise is never used, it could be written as: server.stop(deferred.resolve)

@@ +54,5 @@
> +  do_check_eq("Great Success!", result.json.msg);
> +  do_check_eq(200, result.status);
> +
> +  yield deferredStop(server);
> +  run_next_test();

I believe run_next_test should not be called from test generators (here and elsewhere in this file)

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Apparently flagging for sec-review is the best way to get feedback from the sec team.  The patch in this bug has some crypto related functions that should probably be eyeballed.

Comment 16

[Zachary Carter [:zaach]]

Attachment: FxA client as a component

Turned the client into a component after feedback from fermj, to help memory usage on b2g. Fixed nits, added more tests.

Comment 17

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

:bsmith - would you mind taking a look at the crypto parts metioned in Comment 15?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 831909 [details] [diff] [review]
FxA client as a component

Review of attachment 831909 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Zachary Carter [:zaach] from comment #16)
> Turned the client into a component after feedback from fermj, to help memory
> usage on b2g. Fixed nits, added more tests.

Hmm, I was just about to complain about this not being a JSM and then saw you changed that intentionally. I have no clue about JSMs and their memory usage on B2G but on the Desktop side of things we wouldn't ever write a component (instead of a JSM) for code that is only used by JavaScript

It seems weird to include all that XPCOM fun that we're trying to minimize on all other platforms. This code isn't really in the hot path so XPCOM wouldn't slow things down too much but having to maintain the .manifest and .idl files seems like overhead to me.

::: services/fxaccounts/FxAccountsClient.js
@@ +41,5 @@
> +  host: HOST,
> +
> +  init: function (host = HOST) {
> +    this.host = host;
> +  },

I think it's quite weird that there is a shared instance that everyone can call .init() on and change the host used in all of the code. Shouldn't this be passed to the constructor and the whole FxAccountsClients be implemented as instantiable?

@@ +120,5 @@
> +            err => {dump("HAWK.signCertificate error: "+err+"\n");
> +                    throw err;});
> +  },
> +
> +  _deriveHawkCredentials: function (tokenHex, context, size) {

Shouldn't those bits be in HAWK.jsm or something? For other sync code that needs to make HAWK requests as well?

@@ +132,5 @@
> +      id: CommonUtils.bytesAsHex(out.slice(0, 32))
> +    };
> +  },
> +
> +  _request: function hawkRequest(path, method, credentials, jsonPayload) {

Same.

@@ +134,5 @@
> +  },
> +
> +  _request: function hawkRequest(path, method, credentials, jsonPayload) {
> +    let deferred = Promise.defer();
> +    let xhr = new XMLHttpRequest({mozSystem: true});

xhr.mozBackgroundRequest = true; (to prevent error dialogs)

@@ +142,5 @@
> +    if (jsonPayload) {
> +      payload = JSON.stringify(jsonPayload);
> +    }
> +
> +    xhr.open(method, URI);

After opening we should set some flags to ensure we don't send too much information?

xhr.channel.loadFlags = Ci.nsIChannel.LOAD_ANONYMOUS |
                        Ci.nsIChannel.LOAD_BYPASS_CACHE |
                        Ci.nsIChannel.INHIBIT_CACHING;

@@ +145,5 @@
> +
> +    xhr.open(method, URI);
> +    xhr.onerror = deferred.reject;
> +    xhr.onload = function onload() {
> +      try {

We should probably check xhr.status here.

@@ +147,5 @@
> +    xhr.onerror = deferred.reject;
> +    xhr.onload = function onload() {
> +      try {
> +        let json = JSON.parse(xhr.responseText);
> +        if (json.error) {

I think we also need to check for (json.code && json.code == 200)?

@@ +148,5 @@
> +    xhr.onload = function onload() {
> +      try {
> +        let json = JSON.parse(xhr.responseText);
> +        if (json.error) {
> +          return deferred.reject(json);

Shouldn't we pass |json.error| to deferrec.reject()?

Comment 19

[Zachary Carter [:zaach]]

(In reply to Tim Taubert [:ttaubert] from comment #18)
> Comment on attachment 831909 [details] [diff] [review]
> FxA client as a component
> 
> Review of attachment 831909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Zachary Carter [:zaach] from comment #16)
> > Turned the client into a component after feedback from fermj, to help memory
> > usage on b2g. Fixed nits, added more tests.
> 
> Hmm, I was just about to complain about this not being a JSM and then saw
> you changed that intentionally. I have no clue about JSMs and their memory
> usage on B2G but on the Desktop side of things we wouldn't ever write a
> component (instead of a JSM) for code that is only used by JavaScript
> 
> It seems weird to include all that XPCOM fun that we're trying to minimize
> on all other platforms. This code isn't really in the hot path so XPCOM
> wouldn't slow things down too much but having to maintain the .manifest and
> .idl files seems like overhead to me.

I'll let fermj weigh in here.

> 
> ::: services/fxaccounts/FxAccountsClient.js
> @@ +41,5 @@
> > +  host: HOST,
> > +
> > +  init: function (host = HOST) {
> > +    this.host = host;
> > +  },
> 
> I think it's quite weird that there is a shared instance that everyone can
> call .init() on and change the host used in all of the code. Shouldn't this
> be passed to the constructor and the whole FxAccountsClients be implemented
> as instantiable?

Consumers will create a new instance of the client for each use.
> 
> @@ +120,5 @@
> > +            err => {dump("HAWK.signCertificate error: "+err+"\n");
> > +                    throw err;});
> > +  },
> > +
> > +  _deriveHawkCredentials: function (tokenHex, context, size) {
> 
> Shouldn't those bits be in HAWK.jsm or something? For other sync code that
> needs to make HAWK requests as well?

This method of deriving credentials is specific to the FXA auth protocol. Other users of Hawk may have their own way. Sync, for instance, gets the credentials from the Sync 2.0 token server in exchange for a Persona assertion.

> 
> @@ +132,5 @@
> > +      id: CommonUtils.bytesAsHex(out.slice(0, 32))
> > +    };
> > +  },
> > +
> > +  _request: function hawkRequest(path, method, credentials, jsonPayload) {
> 
> Same.

A general client would probably have a different abstraction to handle many different use cases. This one is pretty simple as it is. We could delegate to a more general client in the future when other use cases become clear, such as Sync's.

> 
> @@ +134,5 @@
> > +  },
> > +
> > +  _request: function hawkRequest(path, method, credentials, jsonPayload) {
> > +    let deferred = Promise.defer();
> > +    let xhr = new XMLHttpRequest({mozSystem: true});
> 
> xhr.mozBackgroundRequest = true; (to prevent error dialogs)

Noted!

> 
> @@ +142,5 @@
> > +    if (jsonPayload) {
> > +      payload = JSON.stringify(jsonPayload);
> > +    }
> > +
> > +    xhr.open(method, URI);
> 
> After opening we should set some flags to ensure we don't send too much
> information?
> 
> xhr.channel.loadFlags = Ci.nsIChannel.LOAD_ANONYMOUS |
>                         Ci.nsIChannel.LOAD_BYPASS_CACHE |
>                         Ci.nsIChannel.INHIBIT_CACHING;
Noted!

> 
> @@ +145,5 @@
> > +
> > +    xhr.open(method, URI);
> > +    xhr.onerror = deferred.reject;
> > +    xhr.onload = function onload() {
> > +      try {
> 
> We should probably check xhr.status here.
> 
> @@ +147,5 @@
> > +    xhr.onerror = deferred.reject;
> > +    xhr.onload = function onload() {
> > +      try {
> > +        let json = JSON.parse(xhr.responseText);
> > +        if (json.error) {
> 
> I think we also need to check for (json.code && json.code == 200)?

The auth server's response format[1] handles these cases. The presence of "error" or "errno" is enough to distinguish an error response from a successful one.

> 
> @@ +148,5 @@
> > +    xhr.onload = function onload() {
> > +      try {
> > +        let json = JSON.parse(xhr.responseText);
> > +        if (json.error) {
> > +          return deferred.reject(json);
> 
> Shouldn't we pass |json.error| to deferrec.reject()?

The json object has additional details[1] about the error that clients may use.

[1] https://github.com/mozilla/picl-idp/blob/master/docs/api.md#response-format

Comment 20

[Richard Newman [:rnewman]]

(In reply to Tim Taubert [:ttaubert] from comment #18)

> xhr.channel.loadFlags = Ci.nsIChannel.LOAD_ANONYMOUS |
>                         Ci.nsIChannel.LOAD_BYPASS_CACHE |
>                         Ci.nsIChannel.INHIBIT_CACHING;

Bear in mind here that LOAD_ANONYMOUS is a pretty big stick: for example, it removes the ability for clients to send TLS client certs, which users hit in the wild with Sync. See Bug 836602.

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Zachary Carter [:zaach] from comment #19)
> (In reply to Tim Taubert [:ttaubert] from comment #18)
> > Comment on attachment 831909 [details] [diff] [review]
> > FxA client as a component
> > 
> > Review of attachment 831909 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to Zachary Carter [:zaach] from comment #16)
> > > Turned the client into a component after feedback from fermj, to help memory
> > > usage on b2g. Fixed nits, added more tests.
> > 
> > Hmm, I was just about to complain about this not being a JSM and then saw
> > you changed that intentionally. I have no clue about JSMs and their memory
> > usage on B2G but on the Desktop side of things we wouldn't ever write a
> > component (instead of a JSM) for code that is only used by JavaScript
> > 
> > It seems weird to include all that XPCOM fun that we're trying to minimize
> > on all other platforms. This code isn't really in the hot path so XPCOM
> > wouldn't slow things down too much but having to maintain the .manifest and
> > .idl files seems like overhead to me.
> 
> I'll let fermj weigh in here.
> 

Having spoken with Tim via IRC, I agree that we can keep it as a module, but exporting *only* the client definition, so consumers can create and manage its own instances.

Sorry for the confusion Zac :(

Comment 22

[Zachary Carter [:zaach]]

Attachment: revert to a JSM

Revert to a JSM and attempt to address Tim's feedback.

Comment 23

[Chris Karlof [:ckarlof]]

The next iteration of the client should support this backoff protocol:

https://github.com/mozilla/fxa-auth-server/pull/323

FYI, it's not implemented yet in the server.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: + accountExists

Comment 25

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: +urls configurable via pref

Apply after accountExists patch

Comment 26

[Zachary Carter [:zaach]]

(In reply to Jed Parsons (use request feedback, please) [:jedp, :jparsons] from comment #25)
> Created attachment 8335003 [details] [diff] [review]
> +urls configurable via pref
> 
> Apply after accountExists patch

One note: PREFIX_NAME is not a URL and shouldn't be configurable. See also: https://github.com/mozilla/fxa-auth-server/issues/329

Comment 27

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: add contentType to hawk header

Comment 28

[Zachary Carter [:zaach]]

Attachment: incorporate various fixes

I've incorporated a few patches that add a pref for the auth server URL, a new client method, and a bug, as well as additional tests and comments.

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8336268 [details] [diff] [review]
incorporate various fixes

Review of attachment 8336268 [details] [diff] [review]:
-----------------------------------------------------------------

Quick drive-by comments

::: b2g/app/b2g.js
@@ +832,5 @@
>  // Enable Web Speech synthesis API
>  pref("media.webspeech.synth.enabled", true);
> +
> +// The URL of the Firefox Accounts auth server backend
> +pref("fxa.auth.uri", "https://api-accounts.dev.lcip.org/v1");

We've been adding prefs as identity.fxaccounts.* in bug 929386

::: b2g/components/FxAccountsManager.js
@@ +1,1 @@
> +"use strict";

There is no need for this FxAccountsManager. We are adding a similar module in bug 929386.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +5,5 @@
> +this.EXPORTED_SYMBOLS = ["FxAccountsClient"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

It seems that you are not using XPCOMUtils

@@ +11,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-crypto/utils.js");
> +
> +// Default can be changed by the preference 'fxa.idp.host'

The pref name in the comment differs from the pref that you get in line 18

Comment 30

[Zachary Carter [:zaach]]

Attachment: remove b2g component

Updated. Thanks fermj.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Daniel,
  We are hoping to land this module over the next couple of days, but it's pending a sec-review.  This module isn't going to be used directly for a while, but we want to land it so we can continue the FxAccounts work for both desktop and FxOS.  Is there any chance a review could be organized soon, or alternatively, we get the green light for it to be reviewed *after* it lands but before it is actually used by anything in central?

Comment 32

[Stephanie Ouillon [:arroway] (needinfo me)]

A security review of the implementation of FxAccounts in FxOS is under work (https://bugzilla.mozilla.org/show_bug.cgi?id=941079), so I'll be able to review this before it lands on central.

Comment 33

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Mark, we will prioritize this as Stéphanie mentions above. But having a quick skim, I think you are ok to land this though in mozilla-central since it doesn't expose anything to web content, afaict. In general I think the guidance is that we try to have security reviews completed before code hits Aurora.

Comment 34

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ps this was from chatting with Dan so I am clearing the needinfo.

Comment 35

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8336973 [details] [diff] [review]
remove b2g component

Review of attachment 8336973 [details] [diff] [review]:
-----------------------------------------------------------------

All in all, this looks much improved, thanks.  I think with the comments addressed this is fine to land.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +185,5 @@
> +                 duration: lifetime };
> +    return Promise.resolve()
> +      .then(_ => this._request("/certificate/sign", "POST", creds, body))
> +      .then(resp => resp.cert,
> +            err => {dump("HAWK.signCertificate error: "+err+"\n");

please add spaces around the '+' operators.

@@ +215,5 @@
> +        }
> +      );
> +  },
> +
> +  /**

It seems this should be in the HAWK module - can you please file a followup to move it there?

@@ +279,5 @@
> +    xhr.open(method, URI);
> +    xhr.channel.loadFlags = Ci.nsIChannel.LOAD_BYPASS_CACHE |
> +                            Ci.nsIChannel.INHIBIT_CACHING;
> +
> +    xhr.onerror = deferred.reject;

I doubt we want to pass the full XHR event object to deferred.reject.  I'd assume we'd want something very similar to what is passed in the other reject calls below - which themselves should be unified.  An error event is unlikely to have much in the way of response attributes, so maybe
xhr.onerror = function(err) {
  deferred.reject({error: "xhr failed"});
}
would be ok.

@@ +281,5 @@
> +                            Ci.nsIChannel.INHIBIT_CACHING;
> +
> +    xhr.onerror = deferred.reject;
> +    xhr.onload = function onload() {
> +      try {

see Tim's comment around this - even though the new exception handler is better and should catch most non-JSON responses, I think a check that xhr.status == 200 is worthwhile.  That might mean a little local function to perform the reject would be best - something like:
function rejectPromise(e) {
  return deferred.reject({error: e, message: xhr.statusText, code: xhr.status, errno: xhr.status});
}
if (xhr.status != 200) {
  return rejectPromise();
}
...
} catch (e) {
  return rejectPromise(e);
}

@@ +284,5 @@
> +    xhr.onload = function onload() {
> +      try {
> +        let json = JSON.parse(xhr.responseText);
> +        if (json.error) {
> +          return deferred.reject(json);

as Tim said, reject(json.error) - although the same format as below would be even better - something like deferred.reject({error: json.error})

::: services/fxaccounts/tests/xpcshell/head.js
@@ +11,5 @@
> +  do_get_profile();
> +
> +  let ns = {};
> +  Cu.import("resource://testing-common/services-common/logging.js",
> +                          ns);

strange indent of this line.

@@ +33,5 @@
> +{
> +  try {
> +    func();
> +  } catch (exc) {
> +    do_check_eq(e.message, message);

presumably this should read exc.message?  Which makes me wonder how tests that use this work?

I think that because xpcshell.ini refers to head_helpers.js, that this function isn't actually needed here.

Comment 36

[Zachary Carter [:zaach]]

Attachment: fxa-client.patch

Fixed nits, and added comments to clarify why we pass the response to the reject handler rather than just response.error (namely, the response has the stable error code and other error details, response.error is just a high level title for the error).

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/dcfc0e0e663d

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dcfc0e0e663d

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Clearing needinfo.