Closed Bug 1047617 Opened 6 years ago Closed 6 years ago

Register logged in user with the Loop server

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
5

Tracking

(firefox34 verified)

VERIFIED FIXED
mozilla34
Iteration:
34.3
Tracking Status
firefox34 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

User Story

As a user, I want to be able to receive calls using my FxA email address when I'm signed in.

Attachments

(1 file, 2 obsolete files)

Blocks: 1047667
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
Attached file promiseFxAOAuthToken function WIP (obsolete) —
Here is a function modified from Vlad's PoC which may be useful to implement this.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 34.2
Points: --- → 5
Added to Iteration 34.2
CCing James since this possibly falls into his area of testing expertise. James, is this something you can help test once resolved?
: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.
QA Contact: anthony.s.hughes → jbonacci
Iteration: 34.2 → 34.3
Flags: qe-verify+
Whiteboard: [qa+]
Attached patch v.1 Exchange code for token (obsolete) — Splinter Review
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?
Attachment #8468320 - Attachment is obsolete: true
Attachment #8478330 - Flags: review?(vlad)
Attachment #8478330 - Flags: review?(mhammond)
Attachment #8478330 - Flags: review?(ckarlof)
We are unifying on "scope" instead of a mix of "scopes" and "scope".
Attachment #8478330 - Attachment is obsolete: true
Attachment #8478330 - Flags: review?(vlad)
Attachment #8478330 - Flags: review?(mhammond)
Attachment #8478330 - Flags: review?(ckarlof)
Attachment #8478485 - Flags: review?(vlad)
Attachment #8478485 - Flags: review?(mhammond)
Attachment #8478485 - Flags: review?(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
@@ +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 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.
Attachment #8478485 - Flags: review?(vlad) → review+
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.
Attachment #8478485 - Flags: review?(vlad)
Attachment #8478485 - Flags: review?(ckarlof)
Attachment #8478485 - Flags: review+
Attachment #8478485 - Flags: review?(vlad) → review+
Attachment #8478485 - Flags: review+ → review?(vlad)
Attachment #8478485 - Flags: review?(vlad) → review+
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.
Attachment #8478485 - Flags: review?(mhammond)
Attachment #8478485 - Flags: review?(ckarlof)
Attachment #8478485 - Flags: review+
(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()
Attachment #8478485 - Flags: review?(ckarlof) → review+
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.
Whiteboard: [fixed-in-fx-team]
(In reply to Matthew N. [:MattN] from comment #13)

Amazing work Matt! I'm really excited for the Loop FxA integration :)
https://hg.mozilla.org/mozilla-central/rev/356d27a0d25d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla34
James, can you make sure this is tested when you get back?
Flags: needinfo?(jbonacci)
Well, if this were not working (code went out 6+ weeks ago), we would know about it by now.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jbonacci)
You need to log in before you can comment on or make changes to this bug.