1215509 - Allow signUp in FxAccountsClient to use ?key=true

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P1)

Description

[Fernando Jiménez Moreno [:ferjm]]

According to [1] POST /v1/account/create accepts a ?key=true query parameter to obtain the keyFetchToken.

We need this badly for Firefox Sync on FxOS, otherwise the UX for the case where we the user creates a FxA through the FxSync flow would be as crappy as asking the user to sign out and sign in again.

[1] https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountcreate

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Chris, do you mind taking a look at this patch, please? Thanks!

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Ugh, actually, this causes the following error after verifying the new account:

Full Message: TypeError: str is undefined
Full Stack: hexToBytes@resource://services-common/utils.js:230:21
task@resource://gre/modules/FxAccounts.jsm:739:1
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:1

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

It seems that the problem is that we are not generating and storing unwrapBKey. I'll be uploading a new patch in a few minutes.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Chris, I wasn't sure about the retrying part of the signUp step. I left it as it was (no retry).

Comment 6

[Ryan Kelly [:rfkelly]]

Also /cc :markh who has review powers over this code, and may have more bandwidth than :ckarlof

Comment 7

[Ryan Kelly [:rfkelly]]

Comment on attachment 8674909 [details] [diff] [review]
v2

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

FWIW this looks OK to me, and disallowing email-case-retries on /account/create seems like the right call.  But I don't think I have the power to grant an r+ in this neck of FxA woods...

::: services/fxaccounts/FxAccountsClient.jsm
@@ +90,5 @@
> +   *          sessionToken: a session token (hex)
> +   *          uid: the user's unique ID (hex)
> +   *          unwrapBKey: used to unwrap kB, derived locally from the
> +   *                      password (not revealed to the FxA server)
> +   *          verified: flag indicating verification status of the email

docs nit: the call to /account/create won't return a "verified" property (but if it did, it'd always be false)

@@ +152,5 @@
>     *        Returns a promise that resolves to an object:
>     *        {
>     *          uid: the user's unique ID (hex)
>     *          sessionToken: a session token (hex)
>     *          keyFetchToken: a key fetch token (hex)

docs nit: this now returns some additional properties like `unwrapBKey`.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Thank you Ryan!

Mark, could you take a look at this patch, please? Thanks!

Comment 9

[Chris Karlof [:ckarlof]]

Sorry :ferjm, yeah, I think that markh will have more bandwidth to review this ATM.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8674909 [details] [diff] [review]
v2

No worries. Thanks anyway, Chris.

Comment 11

[Chris Karlof [:ckarlof]]

Comment on attachment 8674909 [details] [diff] [review]
v2

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

I looks like a straightforward refactor, but I'll leave it to mark to give the r+.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +93,5 @@
> +   *                      password (not revealed to the FxA server)
> +   *          verified: flag indicating verification status of the email
> +   *        }
> +   */
> +  signInSignUpCommon: function(path, email, password, getKeys=false,

This feels like a leaky abstraction in the sense that the path doesn't feel like the right way to signal whether the call wants to sign in or sign up. Perhaps a different magic string, e.g., "signin" and "signup"? Otherwise, it feels like we might be signaling this param is just a configuration option.

Comment 12

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

Comment on attachment 8674909 [details] [diff] [review]
v2

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

Looks fine to me. It's probably worth pinging ckarlof to clarify what he meant there.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +93,5 @@
> +   *                      password (not revealed to the FxA server)
> +   *          verified: flag indicating verification status of the email
> +   *        }
> +   */
> +  signInSignUpCommon: function(path, email, password, getKeys=false,

We should name this with a leading _ to flag it's an internal method and not designed to be used externally.

I also don't really understand ckarlof's comment - this function doesn't seem to take any different action based on the path value.

@@ +103,5 @@
> +      };
> +      let keys = getKeys ? "?keys=true" : "";
> +
> +      return this._request(path + keys, "POST", null, data).then(
> +        // Include the canonical capitalization of the email in the response so

This comment and block confused me for a while given we are supposedly handling different case as an error below - but I see it was previously only used for creation, where we may normalize what was entered. I think it is worth clarifying that this normalization is mainly for account creation and that it is expected to be identical for signin as that enters the "retry" block below.

@@ +112,5 @@
> +
> +          return result;
> +        },
> +        error => {
> +          log.debug("Error: " + JSON.stringify(error));

This should read something like |log.debug("Signing/Signup request failed", error);| - the log module will do the right thing re formatting the error (which it didn't do back when this was originally created :)

@@ +152,5 @@
>     *        Returns a promise that resolves to an object:
>     *        {
>     *          uid: the user's unique ID (hex)
>     *          sessionToken: a session token (hex)
>     *          keyFetchToken: a key fetch token (hex)

IMO these docs are error prone - it would be more accurate to say "returns the object obtained from the server - it probably has fields like .... but possibly has even more" :)

That's not really a suggestion, just a rant ;)

@@ +185,5 @@
>     *                      password (not revealed to the FxA server)
>     *          verified: flag indicating verification status of the email
>     *        }
>     */
>    signIn: function signIn(email, password, getKeys=false, retryOK=true) {

I think we can remove retryOK from this function - it's not really part of the API but as just an implementation detail and isn't necessary now (ie, just unconditionally pass |true| to signInSignUpCommon()

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Thank you Mark and Chris! I *think* what Chris meant is that it is not clear if the caller is asking for a signin or a signup just giving the path. i.e: | this.signInSignUpCommon("/account/create", email, ...) |

And an easy solution could be saving "/account/create" and "/account/login" as SIGNUP and SIGNIN consts and renaming "signInSingUpCommon" to "_createSession". So the calls would be like:

this._createSession(SIGNUP, email, ...);
this._createSession(SIGNIN, email, ...);

What do you think?

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17fe1c4044f3

Comment 15

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

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #13)
> Thank you Mark and Chris! I *think* what Chris meant is that it is not clear
> if the caller is asking for a signin or a signup just giving the path. i.e:
> | this.signInSignUpCommon("/account/create", email, ...) |
> 
> And an easy solution could be saving "/account/create" and "/account/login"
> as SIGNUP and SIGNIN consts and renaming "signInSingUpCommon" to
> "_createSession". So the calls would be like:
> 
> this._createSession(SIGNUP, email, ...);
> this._createSession(SIGNIN, email, ...);
> 
> What do you think?

SGTM, although I don't see much benefit TBH - the function would only need that flag to change the URL and no other logic or behaviour, so if the patch was initially written that way I'd probably have suggested just passing the path directly ;) But it is a minor detail that is fine either way IMO.

Comment 16

[Chris Karlof [:ckarlof]]

Now you guys are going to make me get all academic. :)

I meant that it's a abstraction violation to treat the first param (the path) as both:

1) the literal path to use
2) a signal as to whether it's a signup or signin flow

These kind of abstraction violations make it harder to vary the implementation going forward without changing the abstraction. As implemented, the flow is nearly identical for signin and signup, but suppose we wanted to change the behavior for signup flows going forward. Given the current function signature, we can only reasonably distinguish this via the path, but only if the caller uses the paths that we expect. This may cause to then need to change the signature. 

Also, I don't see any good reason to override the path of the request, so it's not clear why we're exposing it (or requiring the caller to know the right path the use) when we could instead expose an "action" param with only two magic strings as possible values (e.g., "signup" and "signin"). This is similar to what :fergm is suggesting, except that I wouldn't use the path strings as the magic strings.

Perhaps this patch is also just following conventions from other FxA functions in the code, which would be sad, but there you go. 

Lastly, since I've delegated review to mark, I'm not blocking the r+ with my comment. It's just my 2c. I'm happy to continue the discussion or just get out of the way.

Comment 17

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

(In reply to Chris Karlof [:ckarlof] from comment #16)
> I meant that it's a abstraction violation to treat the first param (the
> path) as both:
> 
> 1) the literal path to use
> 2) a signal as to whether it's a signup or signin flow

Fair enough - I guess my point is that no such signal is currently needed, so adding one "just in case" doesn't offer much value and adds a (trivial in this case) complexity cost. This is particularly true because the signature is already abstracted to avoid the distinction - eg, retryOK only makes sense for signIn and not signUp (although that will be hard to fix as that param is also used to guard against infinite retry via recursion)

> These kind of abstraction violations make it harder to vary the
> implementation going forward without changing the abstraction.

That's partly why I like it - this isn't really an abstraction per-se, it's an internal implementation strategy for exactly 2 callers. For all we know, the very next requirement will indeed be allowing the paths to be overridden ;)

But it sounds like we can all agree on comment 13, so let's do that :)

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d037bb0980a

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4a97d6111ac549d6a99f3ffa66934daa0aaca5
Bug 1215509 - Allow signUp in FxAccountsClient to use ?key=true. r=markh

Comment 20

[Chris Karlof [:ckarlof]]

> But it sounds like we can all agree on comment 13, so let's do that :)

I do agree that change makes the function invocations easier to read. I also agree that since it's an internal helper function, it's probably not worth too much further debate.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b4a97d6111a