1047130 - Implement desktop backend to login to FxA via OAuth for Loop

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

This is for the backend glue to allow logging in to FxA via FxAccountsOAuthClient.jsm from Loop code (e.g. MozLoopService.jsm). This bug won't expose any UI to login but will implement the backend and tests making it possible.

This will lead to the login/registration flow opening and will end when the oauth code/state is passed back to the Loop code.

There is a proof of concept that covers this in bug 1022064 attachment 8461282 [details] [diff] [review].

Out of scope for this bug:
* Persisting login state between browser sessions
* UI Changes

Comment 1

[Vlad Filippov (:vladikoff)]

Attachment: DEMO-Add-Loop-to-FxA-OAuth.patch

Attached proof of concept demo code

Comment 2

[Matthew N. [:MattN]]

Bug 1047617 then handles passing the code to the Loop server to complete the login on the Loop side.

Comment 3

[Matthew N. [:MattN]]

Attachment: WIP 1 - param & code retrieval plus basic and stub tests

Does this make sense overall? I have some TODOs with questions.
Do you have ideas for other tests I should write or other failures I need to handle? I have written function names for some tests I plan on writing before landing.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8470354 [details] [diff] [review]
WIP 1 - param & code retrieval plus basic and stub tests

Oops, I meant just feedback?

Comment 5

[Vlad Filippov (:vladikoff)]

Comment on attachment 8470354 [details] [diff] [review]
WIP 1 - param & code retrieval plus basic and stub tests

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

::: browser/components/loop/MozLoopService.jsm
@@ +471,5 @@
> +   */
> +  promiseFxAOAuthClient: Task.async(function* () {
> +    // We must make sure to have only a single client
> +    // otherwise they will have different states and multiple channels.
> +    if (gFxAOAuthClientPromise) { // TODO: when would this happen in practice? Can we be stricter and throw/warn too?

This means the user is clicking "Sign In" multiple times. This could be an unpredictable situation, such as user switching browser windows and hitting "Sign In" again. The state that was generated on the server (via promiseFxAOAuthParameters()) stays the same and is reused.

@@ +477,5 @@
> +    }
> +
> +    gFxAOAuthClientPromise = this.promiseFxAOAuthParameters().then(
> +      parameters => {
> +        console.log("parameters", parameters);

Nit: is there some Loop logging that can be used here?

@@ +502,5 @@
> +  logInToFxA: function() {
> +    let deferred = Promise.defer();
> +    this.promiseFxAOAuthClient().then(
> +      client => {
> +        client.onComplete = this._fxAOAuthComplete.bind(this, deferred);

Is this sufficient or should we still discuss changing onComplete to be a Promise?

@@ +521,5 @@
> +   * @param {Object} tokenData (with code and state)
> +   */
> +  _fxAOAuthComplete: function(deferred, tokenData) {
> +    gFxAOAuthClientPromise = null;
> +    // TODO: what about gFxAOAuthClient? It has .parameters which could be useful later still. e.g. getting the profile_uri.

Yeah if the browser is going to be making profile requests, then the Loop service should have the "profile_uri" stored somewhere.

Comment 6

[Matthew N. [:MattN]]

Thanks for the quick feedback.

(In reply to Vlad Filippov from comment #5)
> Comment on attachment 8470354 [details] [diff] [review]
> WIP 1 - param & code retrieval plus basic and stub tests
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +471,5 @@
> > +   */
> > +  promiseFxAOAuthClient: Task.async(function* () {
> > +    // We must make sure to have only a single client
> > +    // otherwise they will have different states and multiple channels.
> > +    if (gFxAOAuthClientPromise) { // TODO: when would this happen in practice? Can we be stricter and throw/warn too?
> 
> This means the user is clicking "Sign In" multiple times. This could be an
> unpredictable situation, such as user switching browser windows and hitting
> "Sign In" again. The state that was generated on the server (via
> promiseFxAOAuthParameters()) stays the same and is reused.

I see. For some reason I didn't think of this simple case. I guess I was assuming we were going to disable the button after the first click but didn't think of the multiple window case. It seems like it's fine as-is then but I will give this example in the comment to clarify.

> @@ +477,5 @@
> > +    }
> > +
> > +    gFxAOAuthClientPromise = this.promiseFxAOAuthParameters().then(
> > +      parameters => {
> > +        console.log("parameters", parameters);
> 
> Nit: is there some Loop logging that can be used here?

I didn't see any yet but this was just for my own testing and I planned to remove this. I was considering adding logging for the module though.

> @@ +502,5 @@
> > +  logInToFxA: function() {
> > +    let deferred = Promise.defer();
> > +    this.promiseFxAOAuthClient().then(
> > +      client => {
> > +        client.onComplete = this._fxAOAuthComplete.bind(this, deferred);
> 
> Is this sufficient or should we still discuss changing onComplete to be a
> Promise?

This is sufficient but not ideal IMO. I think it would make sense for onComplete to return a promise. Tim, what do you think?

> @@ +521,5 @@
> > +   * @param {Object} tokenData (with code and state)
> > +   */
> > +  _fxAOAuthComplete: function(deferred, tokenData) {
> > +    gFxAOAuthClientPromise = null;
> > +    // TODO: what about gFxAOAuthClient? It has .parameters which could be useful later still. e.g. getting the profile_uri.
> 
> Yeah if the browser is going to be making profile requests, then the Loop
> service should have the "profile_uri" stored somewhere.

I was thinking about keeping the reference to the last gFxAOAuthClient alive (like was already done) so I can access .parameters.profile_uri on it at a later time.

Comment 7

[Matthew N. [:MattN]]

Attachment: v.1 With FxAOAuth change to better handle state mismatch plus more tests

I'm not sure why we're enforcing the Internal vs. public separation so much but it makes it harder to test individual methods so I poked a hole for DEBUG builds to get around this. Mark, do you have a better suggestion?

I made some changes to the FxA module so it doesn't silently ignore invalid states and instead treats it as a failure. I think switching that code to use promises instead of a callback would make it easier to understand as for now I'm using null as the argument to the callback to indicate failure and the object with code and state otherwise. Rejecting for the cases where I callback with null would be more clear.

I finished implementing the tests that I could think of. I also spun off one thing into bug 1052247 for better security.

Comment 8

[Matthew N. [:MattN]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=478119105b93

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8471387 [details] [diff] [review]
v.1 With FxAOAuth change to better handle state mismatch plus more tests

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

I think you could probably just expose promiseFxAOAuthClient rather than the full MozLoopServiceInternal, and then have the tests run in opt mode as well. We've exposed other options before for test functionality, so I think that's reasonable to do here.

If you want a review from a Loop perspective, I'm going to see if I can pass you across to Mike, as I've got a few too many things to look at in detail at the moment.

Comment 10

[Matthew N. [:MattN]]

(In reply to Mark Banner (:standard8) from comment #9)
> I think you could probably just expose promiseFxAOAuthClient rather than the
> full MozLoopServiceInternal, and then have the tests run in opt mode as
> well. We've exposed other options before for test functionality, so I think
> that's reasonable to do here.

Do you know why we're doing this internal/public separation in the first place?

Note that it will be needing access to more methods in bug 1047617 too so I'm not sure adding them all to the public interface is scalable and it would probably defeat the point of the separation.

Comment 11

[Chris Karlof [:ckarlof]]

Comment on attachment 8471387 [details] [diff] [review]
v.1 With FxAOAuth change to better handle state mismatch plus more tests

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

Overall looks good. r+, but I'd like to some info on how errors are being handled in the UI and how the Loop client is capturing the profile_uri from the Loop server.

::: browser/components/loop/MozLoopService.jsm
@@ +479,5 @@
> +    gFxAOAuthClientPromise = this.promiseFxAOAuthParameters().then(
> +      parameters => {
> +        try {
> +          gFxAOAuthClient = new FxAccountsOAuthClient({
> +            parameters: parameters,

Passing the parameters wholesale here is fine, but there is one (profile_uri) that the FxAccountsOauthClient doesn't need. It's for the Loop Client so it knows the URI of the profile server. We discussed having a default profile URI and just overriding it if the Loop server specifies something different. Is this being handled elsewhere, e.g., by a separate request the Loop server?

@@ +508,5 @@
> +      client => {
> +        client.onComplete = this._fxAOAuthComplete.bind(this, deferred);
> +        client.launchWebFlow();
> +      },
> +      error => {

Notes: These errors need to be handled in the UI, e.g., if the FxA Web page never opens due to lack of network connectivity and failure to even retrieve the OAuth params from the server.

@@ +529,5 @@
> +    // Note: The state was already verified in FxAccountsOAuthClient.
> +    if (tokenData) {
> +      deferred.resolve(tokenData);
> +    } else {
> +      deferred.reject("Invalid token data");

Note: This rejection needs UI treatment (Error, log in again?). This code path means an invalid state, correct?

Comment 12

[Matthew N. [:MattN]]

(In reply to Chris Karlof [:ckarlof] from comment #11)
> Comment on attachment 8471387 [details] [diff] [review]
> v.1 With FxAOAuth change to better handle state mismatch plus more tests
> 
> Review of attachment 8471387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good. r+, but I'd like to some info on how errors are being
> handled in the UI and how the Loop client is capturing the profile_uri from
> the Loop server.

I was that the Loop client would access the profile_uri by accessing gFxAOAuthClient.parameters but that may have to change depending on your answer to the below question. I could instead persist the params as a separate global object.

> ::: browser/components/loop/MozLoopService.jsm
> @@ +479,5 @@
> > +    gFxAOAuthClientPromise = this.promiseFxAOAuthParameters().then(
> > +      parameters => {
> > +        try {
> > +          gFxAOAuthClient = new FxAccountsOAuthClient({
> > +            parameters: parameters,
> 
> Passing the parameters wholesale here is fine, but there is one
> (profile_uri) that the FxAccountsOauthClient doesn't need. It's for the Loop
> Client so it knows the URI of the profile server. We discussed having a
> default profile URI and just overriding it if the Loop server specifies
> something different. Is this being handled elsewhere, e.g., by a separate
> request the Loop server?

Right, I figured that they weren't all needed but also thought this was to keep things flexible so we can add new params on the server and have them pass to the webflow since this is what Vlad had in his PoC. Should I add a blacklist or whitelist for the parameters passed to the OAuth client?

> @@ +508,5 @@
> > +      client => {
> > +        client.onComplete = this._fxAOAuthComplete.bind(this, deferred);
> > +        client.launchWebFlow();
> > +      },
> > +      error => {
> 
> Notes: These errors need to be handled in the UI, e.g., if the FxA Web page
> never opens due to lack of network connectivity and failure to even retrieve
> the OAuth params from the server.

Right. These initial bugs are just implementing the backend without UI but I'm trying to make sure that the relevant rejections are accessible by the callers of logInToFxA. In this case I reject with the error so it is accessible. I haven't yet decided on how to make different errors show different UI & localized strings yet. I may want to switch to making all errors be in the format returned by Hawk Requests (with an error code/errno) which makes it easier to do mappings. I think it should be fine to defer that to bug 1047164 as long as I'm not swallowing any errors before then.

> @@ +529,5 @@
> > +    // Note: The state was already verified in FxAccountsOAuthClient.
> > +    if (tokenData) {
> > +      deferred.resolve(tokenData);
> > +    } else {
> > +      deferred.reject("Invalid token data");
> 
> Note: This rejection needs UI treatment (Error, log in again?). This code
> path means an invalid state, correct?

That's correct.

Comment 13

[Chris Karlof [:ckarlof]]

(In reply to Matthew N. [:MattN] from comment #12)
> I was (ck edit: thinking?) that the Loop client would access the profile_uri by accessing
> gFxAOAuthClient.parameters but that may have to change depending on your
> answer to the below question. I could instead persist the params as a
> separate global object.

Yeah, I forgot that parameters was a public property of the client. Seems ok for now at least.

> Right, I figured that they weren't all needed but also thought this was to
> keep things flexible so we can add new params on the server and have them
> pass to the webflow since this is what Vlad had in his PoC. Should I add a
> blacklist or whitelist for the parameters passed to the OAuth client?

I don't think that's necessary at this point. It's a little dirty using client.parameters as a potential dumping ground for whatever the server end point returns, but let's run with it for now.
 

> Right. These initial bugs are just implementing the backend without UI but
> I'm trying to make sure that the relevant rejections are accessible by the
> callers of logInToFxA. In this case I reject with the error so it is
> accessible. I haven't yet decided on how to make different errors show
> different UI & localized strings yet. I may want to switch to making all
> errors be in the format returned by Hawk Requests (with an error code/errno)
> which makes it easier to do mappings. I think it should be fine to defer
> that to bug 1047164 as long as I'm not swallowing any errors before then.

Np. These notes were more of a reminder/note than something actionable at this point.

Cool. r+ with no qualifications.

Comment 14

[Vlad Filippov (:vladikoff)]

Comment on attachment 8471387 [details] [diff] [review]
v.1 With FxAOAuth change to better handle state mismatch plus more tests

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

::: browser/components/loop/MozLoopService.jsm
@@ +706,5 @@
>        return null;
>      }
>    },
>  
> +  logInToFxA: function() {

nit: Missing comments before `logInToFxA` ( /** ... */ )

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +86,5 @@
> +  };
> +  yield promiseOAuthParamsSetup(BASE_URL, params);
> +  let loginPromise = MozLoopService.logInToFxA();
> +  yield loginPromise.catch((error) => {
> +    ok(true, "The login promie should be rejected due to invalid state");

Nit: error is not used. Assert error message or just remove it?

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +167,5 @@
>                  code: data.code,
>                  state: data.state
> +              };
> +            } else {
> +              log.debug("state doesn't match");

Nit: before "log.debug("OAuth flow completed.");" meant that everything went well. Now it will log even if state doesn't match. maybe add log.debug("OAuth flow failed. State didn't match");. If state matches have a success message?

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8471387 [details] [diff] [review]
v.1 With FxAOAuth change to better handle state mismatch plus more tests

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

::: browser/components/loop/MozLoopService.jsm
@@ +459,5 @@
> +   * @return {Promise} resolved with the body of the hawk request for OAuth parameters.
> +   */
> +  promiseFxAOAuthParameters: function() {
> +    return this.hawkRequest("/fxa-oauth/params", "POST").then(response => {
> +      return JSON.parse(response.body);

Json.parse can throw.

What about rejection handling (request error, Loop server down, etc?)

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +10,5 @@
> +const BASE_URL = "http://mochi.test:8888/browser/browser/components/loop/test/mochitest/loop_fxa.sjs?";
> +
> +add_task(function* setup() {
> +  Services.prefs.setCharPref("loop.server", BASE_URL);
> +  Services.prefs.setCharPref("services.push.serverURL", "ws://localhost/");

You can put these pref values in `testing/profiles/prefs_general.js` to be 100% sure that we never phone home.

@@ +30,5 @@
> +  };
> +  yield promiseOAuthParamsSetup(BASE_URL, params);
> +  let client = yield MozLoopService.internal.promiseFxAOAuthClient();
> +  for (let key of Object.keys(params)) {
> +    ise(client.parameters[key], params[key], "Check " + key + " was passed to the OAuth client");

nit: please use `Assert.*` assertion methods, as is common with Loop unit tests.

@@ +43,5 @@
> +
> +add_task(function* sameOAuthClientForTwoCalls() {
> +  MozLoopService.resetFxA();
> +  let client1 = yield MozLoopService.internal.promiseFxAOAuthClient();
> +  let client2 = yield MozLoopService.internal.promiseFxAOAuthClient();

I actually kinda like this solution.

::: browser/components/loop/test/mochitest/loop_fxa.sjs
@@ +20,5 @@
>        return;
>      case "/fxa-oauth/params":
>        params(request, response);
>        return;
> +    case "%2Foauth%2Fauthorization":

nit: you can also use `encodeURIComponent("/oauth/authorization")` here. Your pick.

Comment 16

[Matthew N. [:MattN]]

(In reply to Mike de Boer [:mikedeboer] (PTO from Fri Aug 15. to Sept 1.) from comment #15)
> Review of attachment 8471387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +459,5 @@
> > +   * @return {Promise} resolved with the body of the hawk request for OAuth parameters.
> > +   */
> > +  promiseFxAOAuthParameters: function() {
> > +    return this.hawkRequest("/fxa-oauth/params", "POST").then(response => {
> > +      return JSON.parse(response.body);
> 
> Json.parse can throw.

The promise will reject and that's fine.
 
> What about rejection handling (request error, Loop server down, etc?)

That will be handled in bug 1047164 with UI.

> ::: browser/components/loop/test/mochitest/browser_fxa_login.js
> @@ +10,5 @@
> > +const BASE_URL = "http://mochi.test:8888/browser/browser/components/loop/test/mochitest/loop_fxa.sjs?";
> > +
> > +add_task(function* setup() {
> > +  Services.prefs.setCharPref("loop.server", BASE_URL);
> > +  Services.prefs.setCharPref("services.push.serverURL", "ws://localhost/");
> 
> You can put these pref values in `testing/profiles/prefs_general.js` to be
> 100% sure that we never phone home.

I considered that but since those are used outside Loop and the existing Loop test didn't do this, I didn't either as I don't want to break other tests.

> @@ +30,5 @@
> > +  };
> > +  yield promiseOAuthParamsSetup(BASE_URL, params);
> > +  let client = yield MozLoopService.internal.promiseFxAOAuthClient();
> > +  for (let key of Object.keys(params)) {
> > +    ise(client.parameters[key], params[key], "Check " + key + " was passed to the OAuth client");
> 
> nit: please use `Assert.*` assertion methods, as is common with Loop unit
> tests.

They are longer to type (e.g strictEqual), less familiar to me, and I don't think the disagreement from m.d.platform was resolved yet.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8471387 [details] [diff] [review]
v.1 With FxAOAuth change to better handle state mismatch plus more tests

It's unclear to me why Mike gave r- so I'm requesting additional review from Jared since Mike is on PTO. See comment 16 for my replies to Mike's review.

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] from comment #17)
> It's unclear to me why Mike gave r- so I'm requesting additional review from
> Jared since Mike is on PTO. See comment 16 for my replies to Mike's review.

Sorry for commenting during my PTO...

> I considered that but since those are used outside Loop and the existing Loop test didn't do this,
> I didn't either as I don't want to break other tests.

That's most likely a bug. If you want, you can file it to fix this for the other unit tests and this one in one go.

> They are longer to type (e.g strictEqual), less familiar to me, and I don't think the disagreement from
> m.d.platform was resolved yet.

IMHO, this isn't about my 'battle' with m.d.platform at all, but to ensure uniformity across Loop unit tests. I might personally like the Assert.* style better, but that's not why I wrote this down during review.

Tentatively, r=me.

Comment 19

[Matthew N. [:MattN]]

(In reply to Mike de Boer [:mikedeboer] (PTO from Fri Aug 15. to Sept 1.) from comment #18)
> (In reply to Matthew N. [:MattN] from comment #17)
> > It's unclear to me why Mike gave r- so I'm requesting additional review from
> > Jared since Mike is on PTO. See comment 16 for my replies to Mike's review.
> 
> Sorry for commenting during my PTO...

No need to apologize… I was actually hoping you would reply.

> > I considered that but since those are used outside Loop and the existing Loop test didn't do this,
> > I didn't either as I don't want to break other tests.
> 
> That's most likely a bug. If you want, you can file it to fix this for the
> other unit tests and this one in one go.

I filed bug 1055258.

> > They are longer to type (e.g strictEqual), less familiar to me, and I don't think the disagreement from
> > m.d.platform was resolved yet.
> 
> IMHO, this isn't about my 'battle' with m.d.platform at all, but to ensure
> uniformity across Loop unit tests. I might personally like the Assert.*
> style better, but that's not why I wrote this down during review.

I understand but I don't think we should be writing new tests using the API if we're talking about changing it since it's create a maintenance issue. Specifically, I thought the plan was for Assert.equal to use === and changing that would require updating all of the callers of .equal and .strictEqual.

Pushed: https://hg.mozilla.org/integration/fx-team/rev/135772a1b830

Comment 20

[Matthew N. [:MattN]]

Attachment: v.2 Landed patch. r=vladikoff,ckarlof,mikedeboer

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/135772a1b830