Closed Bug 1048526 Opened 5 years ago Closed 5 years ago

Create a test server for /fxa-oauth/params

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 Oauth flow, we should have a mock server to return the parameters used for passing to FxAccountsOAuthClient.
Flags: firefox-backlog+
Whiteboard: [qa-]
Points: --- → 2
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 34.2
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
Attachment #8470233 - Flags: review?(vlad)
Attachment #8470233 - Flags: review?(ttaubert)
Attachment #8470233 - Flags: review?(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.
Attachment #8470233 - Flags: review?(ckarlof) → review+
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?
Attachment #8470233 - Flags: review?(vlad) → review+
(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 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.
Attachment #8470233 - Flags: review?(ttaubert) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/4068cffc03fc
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-]
Matt, the function `setSharedState()` in loop_fxa.sjs in not defined anywhere, so how does that work?
Flags: needinfo?(MattN+bmo)
(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
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.