1047617 - Register logged in user with the Loop server

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

Server API: http://docs.services.mozilla.com/loop/apis.html

(Quoting Adam Roach's email)
In short, the steps to register an FxA email address are:

    1) Get a Simple Push endpoint using the MozLoopPushHandler class, which is already handled here:
    http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/MozLoopService.jsm?rev=af885bfee5f3#144

    2) Then you need to get an FxA OAuth token, which (if I understand correctly), is the subject of the POC patch at https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1022064&attachment=8466057.

    3) And then you need to POST to /registration with the FxA OAuth token in an Authorization header and the Simple Push endpoint in the "SimplePushURL" field of the JSON body.


So, in terms of the work to be done here: you probably want to create a function similar to hawkRequest (http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/MozLoopService.jsm?rev=af885bfee5f3#175) -- say, "fxaRequest" -- which ensures that you have a valid FxA assertion, and then sends the request in question with an OAuth Authorization header.

Then, do something like renaming the existing registerWithLoopServer (http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/MozLoopService.jsm?rev=af885bfee5f3#243) to be something like "hawkRegisterWithLoopServer"; create a similar "fxaRegisterWithLoopServer," and create a replacement "registerWithLoopServer" that:

    1) Always calls the hawkRegisterWithLoopServer method, and then
    2) Checks whether the user has an OAuth token; and, if so, calls the fxaRegisterWithLoopServer method.

Comment 1

[Matthew N. [:MattN]]

The new plan is for the Loop server to associate the FxA Oauth token with the user's existing Loop session. The Loop client will only use the FxA Oauth token to talk to FxA directly (e.g. for profile information) and never use the token for Loop registration.

After validating the response to the oauth login flow, the browser would need to exchange the code from the oauth flow for a token using a loop server endpoint e.g. /token. The server would then associate the FxA username with the existing push registration in the Loop session. The Oauth token would be returned for profile information queries on the client.

See also: https://github.com/mozilla/fxa-oauth-server/wiki/How-do-I-integrate-with-Firefox-Accounts%3F

Comment 2

[Matthew N. [:MattN]]

Attachment: promiseFxAOAuthToken function WIP

Here is a function modified from Vlad's PoC which may be useful to implement this.

Comment 3

[Marco Mucci [:MarcoM]]

Added to Iteration 34.2

Comment 4

[u279076]

CCing James since this possibly falls into his area of testing expertise. James, is this something you can help test once resolved?

Comment 5

[James Bonacci [:jbonacci]]

:ashughes yea I think the whole Services QA team can come up with something to look at the flows at one end, and the server-side activity at the other end.

Comment 6

[Matthew N. [:MattN]]

Attachment: v.1 Exchange code for token

Error UI will be implemented in bug 1047164 using the promise rejections from logInToFxA.

Some things I'd like reviewers to look for:
* Do I need to handle any hawk/server responses specially?

Comment 7

[Matthew N. [:MattN]]

Attachment: v.1.1 Now with s/scopes/scope/ per #fxa

We are unifying on "scope" instead of a mix of "scopes" and "scope".

Comment 8

[Vlad Filippov (:vladikoff)]

Comment on attachment 8478485 [details] [diff] [review]
v.1.1 Now with s/scopes/scope/ per #fxa

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

::: browser/components/loop/MozLoopService.jsm
@@ +59,5 @@
>  let gLocalizedStrings =  null;
>  let gInitializeTimer = null;
>  let gFxAOAuthClientPromise = null;
>  let gFxAOAuthClient = null;
> +let gFxAOauthTokenData = null;

"gFxAOauthTokenData" should be "gFxAOAuthTokenData" (capital "A" in "Oauth")

@@ +583,5 @@
>      return MozLoopServiceInternal;
>    },
>  
> +  get gFxAOauthTokenData() {
> +    return gFxAOauthTokenData;

"gFxAOauthTokenData" should be "gFxAOAuthTokenData" (capital "A" in "Oauth")

Comment 9

[Vlad Filippov (:vladikoff)]

Comment on attachment 8478485 [details] [diff] [review]
v.1.1 Now with s/scopes/scope/ per #fxa

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

r+ with the fixed "gFxAOauthTokenData" name

::: browser/components/loop/MozLoopService.jsm
@@ +590,4 @@
>    resetFxA: function() {
>      gFxAOAuthClientPromise = null;
> +    gFxAOAuthClient = null;
> +    gFxAOauthTokenData = null;

See above.

@@ +771,5 @@
>    logInToFxA: function() {
>      return MozLoopServiceInternal.promiseFxAOAuthAuthorization().then(response => {
>        return MozLoopServiceInternal.promiseFxAOAuthToken(response.code, response.state);
> +    }).then(tokenData => {
> +      gFxAOauthTokenData = tokenData;

See above.

@@ +775,5 @@
> +      gFxAOauthTokenData = tokenData;
> +      return tokenData;
> +    },
> +    error => {
> +      gFxAOauthTokenData = null;

See above.

Comment 10

[Chris Karlof [:ckarlof]]

Comment on attachment 8478485 [details] [diff] [review]
v.1.1 Now with s/scopes/scope/ per #fxa

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

::: browser/components/loop/MozLoopService.jsm
@@ +537,5 @@
> +      code: code,
> +      state: state,
> +    };
> +    return this.hawkRequest("/fxa-oauth/token", "POST", payload).then(response => {
> +      return JSON.parse(response.body);

I don't think you need to handle errors here, but the callers does. The errors that can happen here need to be documented, and there are probably two interesting classes:

4xx: Start the login flow over. I'm not sure what would cause these, but they could happen.

5xx or no network: Probably show message "try again later", and clear state so user can try the login again later. This class is the more likely error users would encounter. 

Another set of errors can occur when using the token at the profile server, which I assume will be handled by another patch. See https://github.com/mozilla/fxa-profile-server/blob/master/docs/API.md#errors

There are 4xx errors, particularly a 403, which should force the user to re-login, and 500 errors which you should try again later and/or rely on cached data.

Comment 11

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

Comment on attachment 8478485 [details] [diff] [review]
v.1.1 Now with s/scopes/scope/ per #fxa

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

Looks fine to me.

::: browser/components/loop/MozLoopService.jsm
@@ +538,5 @@
> +      state: state,
> +    };
> +    return this.hawkRequest("/fxa-oauth/token", "POST", payload).then(response => {
> +      return JSON.parse(response.body);
> +    });

It is probably worthwhile catching and logging an error here - sadly the returned error object doesn't have a good toString() method (there is another sync bug on this), but logging, say, .code and .error on the error object would be useful.

Something somewhat unrelated to this patch is that I notice there doesn't seem to be any handling for "backoff" requests from the hawk server.

Comment 12

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

(In reply to Mark Hammond [:markh] from comment #11)
> It is probably worthwhile catching and logging an error here - sadly the
> returned error object doesn't have a good toString() method (there is
> another sync bug on this), but logging, say, .code and .error on the error
> object would be useful.

Actually, it's possibly better to do that directly in .hawkRequest()

Comment 13

[Matthew N. [:MattN]]

Thanks for the quick reviews!

Pushed: https://hg.mozilla.org/integration/fx-team/rev/356d27a0d25d


(In reply to Vlad Filippov from comment #8)
> "gFxAOauthTokenData" should be "gFxAOAuthTokenData" (capital "A" in "Oauth")

Fixed.

(In reply to Chris Karlof [:ckarlof] from comment #10)
> I don't think you need to handle errors here, but the callers does. The
> errors that can happen here need to be documented, and there are probably
> two interesting classes:

I added brief documentation to the JSDoc header to mention that these errors need to be handled by the caller.

(In reply to Mark Hammond [:markh] from comment #12)
> (In reply to Mark Hammond [:markh] from comment #11)
> > It is probably worthwhile catching and logging an error here - sadly the
> > returned error object doesn't have a good toString() method (there is
> > another sync bug on this), but logging, say, .code and .error on the error
> > object would be useful.
> 
> Actually, it's possibly better to do that directly in .hawkRequest()

Done with console.error which properly handles objects.

Comment 14

[Vlad Filippov (:vladikoff)]

(In reply to Matthew N. [:MattN] from comment #13)

Amazing work Matt! I'm really excited for the Loop FxA integration :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/356d27a0d25d

Comment 16

[u279076]

James, can you make sure this is tested when you get back?

Comment 17

[James Bonacci [:jbonacci]]

Well, if this were not working (code went out 6+ weeks ago), we would know about it by now.