Closed Bug 1048530 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
2

Tracking

(firefox34 fixed)

RESOLVED FIXED
mozilla34
Iteration:
34.2
Tracking Status
firefox34 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

Attachments

(1 file)

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.
Flags: firefox-backlog+
Whiteboard: [qa-]
Points: --- → 2
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 34.2
Summary: Create a test server for /fxa-oauth/code → Create a Loop test server for exchanging an OAuth code for a token
Try push: https://tbpl.mozilla.org/?tree=Try&rev=478119105b93

This patch depends on bug 1048526.
Attachment #8471997 - Flags: review?(vlad)
Attachment #8471997 - Flags: review?(ttaubert)
Attachment #8471997 - Flags: review?(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.
Attachment #8471997 - Flags: review?(ckarlof) → review+
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 :)
Attachment #8471997 - Flags: review?(vlad) → review+
(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.
(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 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 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.
Attachment #8471997 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/e9ae608b82e8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][fixed-in-fx-team] → [qa-]
Target Milestone: --- → mozilla34
Flags: qe-verify-
QA Contact: anthony.s.hughes
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.