1048526 - Create a test server for /fxa-oauth/params

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

For testing the FxA Oauth flow, we should have a mock server to return the parameters used for passing to FxAccountsOAuthClient.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 Add a m-bc test server for OAuth params

Chris and/or Vlad, can you make sure I'm implementing the endpoint properly? I realize the path or property names may change but that is fine.

Tim, can you do a review as a browser/ peer?

Thanks

Comment 2

[Chris Karlof [:ckarlof]]

Comment on attachment 8470233 [details] [diff] [review]
v.1 Add a m-bc test server for OAuth params

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

::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
@@ +17,5 @@
> +  let params = {
> +    client_id: "my_client_id",
> +    content_uri: "https://example.com/content/",
> +    oauth_uri: "https://example.com/oauth/",
> +    profile_uri: "https://example.com/profile/",

Default versions of these URIs could ship with the browser, but we definitely want our services to be able to override them. If we add defaults to the browser, then these could be considered optional.

@@ +31,5 @@
> +});
> +
> +add_task(function* optional_setup_params() {
> +  let params = {
> +    action: "signin",

"action" isn't strictly need from the server. I think only "signin" or "signup" are valid options here, which are likely going to be driven by a client side UI decision.

Comment 3

[Vlad Filippov (:vladikoff)]

Comment on attachment 8470233 [details] [diff] [review]
v.1 Add a m-bc test server for OAuth params

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

Looks good!

::: browser/components/loop/test/mochitest/loop_fxa.sjs
@@ +62,5 @@
> +    response.setStatusLine(request.httpVersion, 405, "Method Not Allowed");
> +    response.setHeader("Allow", "POST", false);
> +
> +    // Add a button to make a POST request to make this endpoint easier to debug in the browser.
> +    response.write('<form method=POST><button type=submit>POST</button></form>');

Nit: Double quotes?

Comment 4

[Matthew N. [:MattN]]

(In reply to Chris Karlof [:ckarlof] from comment #2)
> Comment on attachment 8470233 [details] [diff] [review]
> v.1 Add a m-bc test server for OAuth params
> 
> ::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
> @@ +17,5 @@
> > +  let params = {
> > +    client_id: "my_client_id",
> > +    content_uri: "https://example.com/content/",
> > +    oauth_uri: "https://example.com/oauth/",
> > +    profile_uri: "https://example.com/profile/",
> 
> Default versions of these URIs could ship with the browser, but we
> definitely want our services to be able to override them. If we add defaults
> to the browser, then these could be considered optional.

Okay, I'll keep that in mind.

> @@ +31,5 @@
> > +});
> > +
> > +add_task(function* optional_setup_params() {
> > +  let params = {
> > +    action: "signin",
> 
> "action" isn't strictly need from the server. I think only "signin" or
> "signup" are valid options here, which are likely going to be driven by a
> client side UI decision.

Right, which is why it's in the test of "optional" parameters.

(In reply to Vlad Filippov from comment #3)
> > +    // Add a button to make a POST request to make this endpoint easier to debug in the browser.
> > +    response.write('<form method=POST><button type=submit>POST</button></form>');
> 
> Nit: Double quotes?

Yep, I'll fix that.

Comment 5

[Matthew N. [:MattN]]

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

Comment 6

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

Comment on attachment 8470233 [details] [diff] [review]
v.1 Add a m-bc test server for OAuth params

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

::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
@@ +49,5 @@
> +  }
> +});
> +
> +add_task(function* delete_setup_params() {
> +  promiseDeletedOAuthParams(BASE_URL);

shouldn't this be `yield promiseDeletedOAuthParams(BASE_URL);` ?

::: browser/components/loop/test/mochitest/loop_fxa.sjs
@@ +62,5 @@
> +    response.setStatusLine(request.httpVersion, 405, "Method Not Allowed");
> +    response.setHeader("Allow", "POST", false);
> +
> +    // Add a button to make a POST request to make this endpoint easier to debug in the browser.
> +    response.write('<form method=POST><button type=submit>POST</button></form>');

should this form have an action of "/fxa-oauth/params"? I guess it's not entirely necessary since the page it is located on is already /fxa-oauth/params.

@@ +73,5 @@
> +
> +  // Warn if required parameters are missing.
> +  for (let paramName of REQUIRED_PARAMS) {
> +    if (!(paramName in params)) {
> +      dump("Warning: " + paramName + " is a required parameter\n");

If these are required, we should probably do more than just warn.

Comment 7

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8470233 [details] [diff] [review]
> v.1 Add a m-bc test server for OAuth params
> 
> Review of attachment 8470233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/test/mochitest/browser_loop_fxa_server.js
> @@ +49,5 @@
> > +  }
> > +});
> > +
> > +add_task(function* delete_setup_params() {
> > +  promiseDeletedOAuthParams(BASE_URL);
> 
> shouldn't this be `yield promiseDeletedOAuthParams(BASE_URL);` ?

Good catch.

> ::: browser/components/loop/test/mochitest/loop_fxa.sjs
> @@ +62,5 @@
> > +    response.setStatusLine(request.httpVersion, 405, "Method Not Allowed");
> > +    response.setHeader("Allow", "POST", false);
> > +
> > +    // Add a button to make a POST request to make this endpoint easier to debug in the browser.
> > +    response.write('<form method=POST><button type=submit>POST</button></form>');
> 
> should this form have an action of "/fxa-oauth/params"? I guess it's not
> entirely necessary since the page it is located on is already
> /fxa-oauth/params.

Right, it's using the default action which is the same path. Hard-coding that makes this harder to maintain.

> @@ +73,5 @@
> > +
> > +  // Warn if required parameters are missing.
> > +  for (let paramName of REQUIRED_PARAMS) {
> > +    if (!(paramName in params)) {
> > +      dump("Warning: " + paramName + " is a required parameter\n");
> 
> If these are required, we should probably do more than just warn.

Since this is just a test server and I want to be able to write tests to see what happens when some of these are missing, I think warning is sufficient. I don't want tests to fail when I'm testing the lack of required parameters.

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4068cffc03fc

Comment 10

[Mike de Boer [:mikedeboer]]

Matt, the function `setSharedState()` in loop_fxa.sjs in not defined anywhere, so how does that work?

Comment 11

[Matthew N. [:MattN]]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> Matt, the function `setSharedState()` in loop_fxa.sjs in not defined
> anywhere, so how does that work?

See https://developer.mozilla.org/en-US/docs/Httpd.js/HTTP_server_for_unit_tests#Storing_information_across_requests_%28not_available_in_Gecko.C2.A01.9.0%29