All users were logged out of Bugzilla on October 13th, 2018

Create a test server for /fxa-oauth/params

RESOLVED FIXED in Firefox 34

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

unspecified
mozilla34
Points:
2
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox34 fixed)

Details

(URL)

Attachments

(1 attachment)

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
Created attachment 8470233 [details] [diff] [review]
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
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
Last Resolved: 4 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)
status-firefox34: --- → fixed
You need to log in before you can comment on or make changes to this bug.