Closed Bug 1215509 Opened 4 years ago Closed 4 years ago

Allow signUp in FxAccountsClient to use ?key=true

Categories

(Firefox :: Firefox Accounts, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox44 --- fixed

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → ferjmoreno
Target Milestone: --- → FxOS-S9 (16Oct)
Attached patch v1 (obsolete) — Splinter Review
Chris, do you mind taking a look at this patch, please? Thanks!
Attachment #8674874 - Flags: review?(ckarlof)
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
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.
Attachment #8674874 - Flags: review?(ckarlof)
Attached patch v2Splinter Review
Attachment #8674874 - Attachment is obsolete: true
Attachment #8674909 - Flags: review?(ckarlof)
Chris, I wasn't sure about the retrying part of the signUp step. I left it as it was (no retry).
Also /cc :markh who has review powers over this code, and may have more bandwidth than :ckarlof
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`.
Target Milestone: FxOS-S9 (16Oct) → FxOS-S10 (30Oct)
Priority: -- → P1
Attachment #8674909 - Flags: review?(markh)
Thank you Ryan!

Mark, could you take a look at this patch, please? Thanks!
Sorry :ferjm, yeah, I think that markh will have more bandwidth to review this ATM.
Comment on attachment 8674909 [details] [diff] [review]
v2

No worries. Thanks anyway, Chris.
Attachment #8674909 - Flags: review?(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.
Attachment #8674909 - Flags: feedback+
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()
Attachment #8674909 - Flags: review?(markh) → review+
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?
Flags: needinfo?(ckarlof)
(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.
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.
Flags: needinfo?(ckarlof)
(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 :)
> 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.
https://hg.mozilla.org/mozilla-central/rev/6b4a97d6111a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [partner-cherry-pick]
Whiteboard: [partner-cherry-pick]
Product: Core → Firefox
Target Milestone: FxOS-S10 (30Oct) → ---
You need to log in before you can comment on or make changes to this bug.