1048530 - Create a Loop test server for exchanging an OAuth code for a token

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

For testing the FxA Loop integration, we should have a mock Loop server to post the Oauth code to in order to register FxA login with the server and return the FxA Oauth token to be used for profile information.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 SJS addition plus tests

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

This patch depends on bug 1048526.

Comment 2

[Chris Karlof [:ckarlof]]

Comment on attachment 8471997 [details] [diff] [review]
v.1 SJS addition plus tests

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

::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
@@ +68,5 @@
> +  yield promiseOAuthParamsSetup(BASE_URL, params);
> +  let request = yield promise_token("my_code", params.state);
> +  ise(request.status, 200, "Check token response status");
> +  ise(request.response.access_token, "my_code_access_token", "Check access_token");
> +  ise(request.response.scopes, "", "Check scopes");

I think this should probably be a scope of "profile" for now.

Comment 3

[Vlad Filippov (:vladikoff)]

Comment on attachment 8471997 [details] [diff] [review]
v.1 SJS addition plus tests

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

This looks good! I couldn't find any issues :)

Comment 4

[Vlad Filippov (:vladikoff)]

(In reply to Chris Karlof [:ckarlof] from comment #2)
> Comment on attachment 8471997 [details] [diff] [review]
> v.1 SJS addition plus tests
> 
> Review of attachment 8471997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
> @@ +68,5 @@
> > +  yield promiseOAuthParamsSetup(BASE_URL, params);
> > +  let request = yield promise_token("my_code", params.state);
> > +  ise(request.status, 200, "Check token response status");
> > +  ise(request.response.access_token, "my_code_access_token", "Check access_token");
> > +  ise(request.response.scopes, "", "Check scopes");
> 
> I think this should probably be a scope of "profile" for now.

Agreed with this one.

Comment 5

[Matthew N. [:MattN]]

(In reply to Vlad Filippov from comment #4)
> (In reply to Chris Karlof [:ckarlof] from comment #2)
> > I think this should probably be a scope of "profile" for now.
> 
> Agreed with this one.

OK, thanks, I'll fix this after the last review.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8471997 [details] [diff] [review]
v.1 SJS addition plus tests

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

r=me if you want to take it...

::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
@@ +66,5 @@
> +    state: "my_state",
> +  };
> +  yield promiseOAuthParamsSetup(BASE_URL, params);
> +  let request = yield promise_token("my_code", params.state);
> +  ise(request.status, 200, "Check token response status");

nit: please use `Assert.*` assertion methods here, as that's what the other loop mochitests use. Under the hood `ise` proxies to `Assert.equal` but hey, who knows that? ;-)

@@ +103,5 @@
>  
>    return deferred.promise;
>  }
> +
> +function promise_token(code, state) {

nit: in Loop unit tests we generally use camelCase for function names like these. Again, style preference, but ok.

::: browser/components/loop/test/mochitest/loop_fxa.sjs
@@ +114,5 @@
> + */
> +function token(request, response) {
> +  let params = JSON.parse(getSharedState("/fxa-oauth/params") || "{}");
> +  let body = NetUtil.readInputStreamToString(request.bodyInputStream,
> +                                             request.bodyInputStream.available());

nit: what happens with the indentation here? Looks odd in Splinter...

@@ +118,5 @@
> +                                             request.bodyInputStream.available());
> +  let queryParams = new URLSearchParams(body);
> +  if (!params.state || params.state !== queryParams.get("state")) {
> +    response.setStatusLine(request.httpVersion, 400, "State mismatch");
> +    response.write("State mismatch");

You'll probably want to `return;` here as the request has ended.

@@ +127,5 @@
> +    scopes: "",
> +    token_type: "bearer",
> +  };
> +  response.setHeader("Content-Type", "application/json; charset=utf-8", false);
> +  response.write(JSON.stringify(tokenData, null, 4));

nit: indentation of '2' would be fine as well, no?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8471997 [details] [diff] [review]
v.1 SJS addition plus tests

(In reply to Mike de Boer [:mikedeboer] (PTO from Fri Aug 15. to Sept 1.) from comment #6)
> Review of attachment 8471997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you want to take it...

Thanks

> ::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
> @@ +66,5 @@
> > +    state: "my_state",
> > +  };
> > +  yield promiseOAuthParamsSetup(BASE_URL, params);
> > +  let request = yield promise_token("my_code", params.state);
> > +  ise(request.status, 200, "Check token response status");
> 
> nit: please use `Assert.*` assertion methods here, as that's what the other
> loop mochitests use. Under the hood `ise` proxies to `Assert.equal` but hey,
> who knows that? ;-)

I'm waiting for the changes (mainly equal using ===) discussed in m.d.platform to be made before using Assert.jsm to avoid churn and so I don't have to type "strictEqual" every time. Btw., from looking at browser-test.js, I don't see ise using Assert.jsm at all.

> @@ +103,5 @@
> >  
> >    return deferred.promise;
> >  }
> > +
> > +function promise_token(code, state) {
> 
> nit: in Loop unit tests we generally use camelCase for function names like
> these. Again, style preference, but ok.

Fixed

> ::: browser/components/loop/test/mochitest/loop_fxa.sjs
> @@ +114,5 @@
> > + */
> > +function token(request, response) {
> > +  let params = JSON.parse(getSharedState("/fxa-oauth/params") || "{}");
> > +  let body = NetUtil.readInputStreamToString(request.bodyInputStream,
> > +                                             request.bodyInputStream.available());
> 
> nit: what happens with the indentation here? Looks odd in Splinter...

Looks fine to me.

> @@ +118,5 @@
> > +                                             request.bodyInputStream.available());
> > +  let queryParams = new URLSearchParams(body);
> > +  if (!params.state || params.state !== queryParams.get("state")) {
> > +    response.setStatusLine(request.httpVersion, 400, "State mismatch");
> > +    response.write("State mismatch");
> 
> You'll probably want to `return;` here as the request has ended.

Good catch.

> @@ +127,5 @@
> > +    scopes: "",
> > +    token_type: "bearer",
> > +  };
> > +  response.setHeader("Content-Type", "application/json; charset=utf-8", false);
> > +  response.write(JSON.stringify(tokenData, null, 4));
> 
> nit: indentation of '2' would be fine as well, no?

Sure, changed.

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9ae608b82e8