1065052 - Implement modified Loop FxA registration flow using a second Loop registration

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

Attachment: v.1 Second registration to the loop server

See bug 1047667 comment #9 and bug 1047667 comment #12.

Comment 1

[Adam Roach [:abr]]

Comment on attachment 8486612 [details] [diff] [review]
v.1 Second registration to the loop server

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

Looks good to me.

::: browser/components/loop/MozLoopAPI.jsm
@@ +337,5 @@
>      hawkRequest: {
>        enumerable: true,
>        writable: true,
>        value: function(path, method, payloadObj, callback) {
> +        // XXX Should take a sessionType parameter instead of hard-coding GUEST

Yep. Is there any reason we're not fixing that with this patch?

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +7,5 @@
>  
>  "use strict";
>  
>  const gFxAOAuthTokenData = Cu.import("resource:///modules/loop/MozLoopService.jsm", {}).gFxAOAuthTokenData;
> +const LOOP_SESSION_TYPE = Cu.import("resource:///modules/loop/MozLoopService.jsm", {}).LOOP_SESSION_TYPE;

I might be confused, but if you have to pull in multiple symbols, I think you can do something like:

const {gFxAOAuthTokenData, LOOP_SESSION_TYPE} = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});

But since these contexts are cached, I don't think it makes a lot of difference.

Comment 2

[Matthew N. [:MattN]]

(In reply to Adam Roach [:abr] from comment #1)
> ::: browser/components/loop/MozLoopAPI.jsm
> > +        // XXX Should take a sessionType parameter instead of hard-coding GUEST
> 
> Yep. Is there any reason we're not fixing that with this patch?

It's not specific to the login flow (the three callers are related to calls) and the callers may need to make other changes which I'm not sure about. I'm filing those as part of my breakdown in bug 1061711.

> ::: browser/components/loop/test/mochitest/browser_fxa_login.js
> I might be confused, but if you have to pull in multiple symbols, I think
> you can do something like:
> 
> const {gFxAOAuthTokenData, LOOP_SESSION_TYPE} =
> Cu.import("resource:///modules/loop/MozLoopService.jsm", {});

Good point about the object destructuring.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8486612 [details] [diff] [review]
v.1 Second registration to the loop server

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

::: browser/components/loop/MozLoopService.jsm
@@ +263,4 @@
>    /**
>     * Used to store a session token from a request if it exists in the headers.
>     *
> +   * @param {LOOP_SESSION_TYPE} sessionType

All other function comments explain what sessionType is, except for this one.

@@ -307,5 @@
>            }
>  
>            // Authorization failed, invalid token, we need to try again with a new token.
> -          Services.prefs.clearUserPref("loop.hawk-session-token");
> -          this.registerWithLoopServer(pushUrl, true);

Good catch about the infinite retry loop here (unreferenced noRetry), as well as removing the negation from the boolean name.

Comment 4

[Matthew N. [:MattN]]

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

Comment 5

[Matthew N. [:MattN]]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c07bee00871a
Relanded in https://hg.mozilla.org/integration/fx-team/rev/d8977af570e3 with a simple deferred check.

Comment 6

[Matthew N. [:MattN]]

mikedeboer noticed I broke call URLs in the panel since I missed forwaring the sessionType from the public service method the internal. Apparently we don't have tests on TBPL for that:
https://hg.mozilla.org/integration/fx-team/rev/e7bc94d454ee

Comment 7

[Mark Banner (:standard8)]

(In reply to Matthew N. [:MattN] from comment #6)
> mikedeboer noticed I broke call URLs in the panel since I missed forwaring
> the sessionType from the public service method the internal. Apparently we
> don't have tests on TBPL for that:

Correct, we do have functional tests in work (bug 976134 I believe), but you can only run them manually at the moment (due to complexities of integration with build system).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8977af570e3
https://hg.mozilla.org/mozilla-central/rev/e7bc94d454ee

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8486612 [details] [diff] [review]
v.1 Second registration to the loop server

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c2ae06403e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/97040a73debb
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbfa16c4d3cc

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486612 [details] [diff] [review]
v.1 Second registration to the loop server

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.