Closed Bug 727210 Opened 8 years ago Closed 8 years ago

Implement token server client for Sync

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla14

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa+])

Attachments

(1 file, 2 obsolete files)

As part of BID support, we'll need a token server client to talk to the token server.

The attached patch contains a generic token server client. Ideally, I would land this in toolkit, but I'm not sure it is appropriate at this juncture since things are still being flushed out.

As for implementation, the API could be better, particularly around error handling. Currently, there is no programmatic way to differentiate error conditions. Assuming we want that, please specify how you would like it. Error-derived types? >2 arguments to callback? etc.
Attachment #597140 - Flags: feedback?(rnewman)
There's one minor thing we need to dicuss : for single box set ups, the service_entry might not be provided as a full url in the response, but as a relative url.

In that case, the client needs to put in service_entry the root value of the token server itself + the relative url provided by the value. 

Let me know if this works for you on the client side.
(In reply to Tarek Ziadé (:tarek) from comment #1)
> There's one minor thing we need to dicuss : for single box set ups, the
> service_entry might not be provided as a full url in the response, but as a
> relative url.
> 
> In that case, the client needs to put in service_entry the root value of the
> token server itself + the relative url provided by the value. 
> 
> Let me know if this works for you on the client side.

22:32 < telliott> the old client used to understand relative paths. Since we're building a new client, let's not build that into it or support it in the server
Comment on attachment 597140 [details] [diff] [review]
Implement token server client for Sync

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

Thumbs up.

wrt error handling: no additional arguments. I'd be inclined to use Error subclasses for the three error cases -- malformed response, bad content type, fetch failed.

Note that the failure cases -- and the recovery strategies -- can essentially be split into "server did something wrong", "network not working", "client did something wrong". At the risk of getting a bit Javaeseque, having the error hierarchy reflect that will simplify calling code that wishes to thoroughly recover from failures.

::: services/sync/modules/auth.js
@@ +12,5 @@
> +
> +const EXPORTED_SYMBOLS = ["TokenServerClient"];
> +
> +/**
> + * Represents a client to the Token Server.

Link to the spec would be super awesome.

@@ +70,5 @@
> +    function getTokenFromBrowserIDAssertion(app, assertion, cb) {
> +
> +    if (!cb) {
> +      throw new Error("cb argument is not defined.");
> +    }

If the other two args are required -- and it looks like they are, unless you want your URI to include "null" or "undefined" -- you should throw for those, too.

@@ +104,5 @@
> +  _processTokenResponse: function processTokenResponse(response, error, cb) {
> +    this._log.debug("Got token response.");
> +
> +    if (error) {
> +      cb(error, null);

Can't we just do this check in the onResponse handler, and avoid the error argument to _processTokenResponse entirely?

@@ +110,5 @@
> +    }
> +    if (!response.success) {
> +      this._log.info("Non-200 response code to token request: " +
> +                     response.status);
> +      cb(new Error("Non 2xx response code: " + response.status), null);

Standardize on 2xx or 200, and if you really mean 200, you should use (200 != response.status) instead of !response.success.

(I have no idea what the spec says.)

@@ +115,5 @@
> +      return;
> +    }
> +
> +    let ct = response.headers["content-type"];
> +    if (ct != "application/json") {

Woah, nelly. Don't tell me you're going to reject charset parameters?

@@ +120,5 @@
> +      cb(new Error("Unsupported media type: " + ct), null);
> +      return;
> +    }
> +
> +    let result = JSON.parse(response.body);

Can JSON.parse throw?
Attachment #597140 - Flags: feedback?(rnewman) → feedback+
Whiteboard: [qa+]
(In reply to Richard Newman [:rnewman] from comment #3)
> Comment on attachment 597140 [details] [diff] [review]
> @@ +115,5 @@
> > +      return;
> > +    }
> > +
> > +    let ct = response.headers["content-type"];
> > +    if (ct != "application/json") {
> 
> Woah, nelly. Don't tell me you're going to reject charset parameters?

*facepalm*

> @@ +120,5 @@
> > +      cb(new Error("Unsupported media type: " + ct), null);
> > +      return;
> > +    }
> > +
> > +    let result = JSON.parse(response.body);
> 
> Can JSON.parse throw?

Yes. This is why in the main RESTRequest callback I call the response handler inside a try..catch. In the new world with multiple error types, I think it would be appropriate to trap this and rethrow using one of the new error types.
Latest version. Incorporated all of feedback.
Assignee: nobody → gps
Attachment #597140 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #599711 - Attachment is patch: true
Comment on attachment 599711 [details] [diff] [review]
Implement token server client for Sync, v2

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

Don't forget the bug number in the patch summary.

Just nits.

::: services/sync/modules/auth.js
@@ +72,5 @@
> + * not force the client to manipulate URIs. This type currently enforces this
> + * practice by not implementing an API which would perform URI manipulation.
> + * If you are tempted to implement this API in the future, consider this your
> + * warning that you may be doing it wrong and that you should store full URIs
> + * instead.

+1

@@ +107,5 @@
> +   */
> +  getTokenFromBrowserIDAssertion:
> +    function getTokenFromBrowserIDAssertion(url, assertion, cb) {
> +    if (!url) {
> +      throw new TokenServerClientError("url argument is not defined.");

Strictly speaking it might be *defined* (e.g., url = null), it's just not *valid*.

@@ +154,5 @@
> +    if (!response.success) {
> +      this._log.info("Non-200 response code to token request: " +
> +                     response.status);
> +      cb(new TokenServerClientServerError("Non 2xx response code: " +
> +                                          response.status), null);

The other error callbacks include the exception. In the case of a server error, perhaps include the response?

@@ +159,5 @@
> +      return;
> +    }
> +
> +    let ct = response.headers["content-type"];
> +    if (ct != "application/json" && ct.indexOf("application/json; ") != 0) {

That space after "json;" is incorrect, I think. My reading of the spec suggests that whitespace there is permitted but not required.
Attachment #599711 - Flags: review+
* Moved to services/common \o/
* Reflected changes to JSON fields in HTTP response
* Misc docs
* Added example usage (not perfect, but better than nothing)
Attachment #599711 - Attachment is obsolete: true
Attachment #612981 - Flags: review?(rnewman)
Comment on attachment 612981 [details] [diff] [review]
Token server client, v3

Added pref to services-sync.js has been removed locally.
Comment on attachment 612981 [details] [diff] [review]
Token server client, v3

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

::: services/common/tests/unit/test_tokenserverclient.js
@@ +60,5 @@
> +  for each (let arg in args) {
> +    try {
> +      let client = new TokenServerClient();
> +      client.getTokenFromBrowserIDAssertion(arg[0], arg[1], arg[2]);
> +      do_check_true(false);

do_throw("Should not get here!");

::: services/common/tokenserverclient.js
@@ +90,5 @@
> + */
> +function TokenServerClient() {
> +  this._log = Log4Moz.repository.getLogger("Common.TokenServerClient");
> +  this._log.level =
> +    Log4Moz.Level[Prefs.get("logger.level")];

Unbreak my liiiiiiine /
Say you'll join me again /
Undo this break that you caused /
When you QQed my clause /
And wrote out to my file /
Unbreak my liiiiiiine
Attachment #612981 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/eecb4cf4c45d

On behalf of AITC, I thank you for doing this review.
Whiteboard: [qa+] → [qa+][fixed in services]
(In reply to Gregory Szorc [:gps] from comment #10)
> https://hg.mozilla.org/services/services-central/rev/eecb4cf4c45d
> 
> On behalf of AITC, I thank you for doing this review.

Sorry it took so long!
gps, can we get some qa+ steps to test this patch?  as detailed as possible, always helps!
This code isn't hooked up anywhere: it's just a standalone client.

If you load up a privileged JS console, you should be able to test things against an actual server (which I'll admit I haven't done - programming against the spec should be sufficient).

Try the following:

  Components.utils.import("resource://services-common/tokenserverclient.js");

  let client = new TokenServerClient();
  let assertion = "BIDASSERTION";
  let tokenURL = "https://stage-token.services.mozilla.com/1.0/aitc/1.0";

  client.getTokenFromBrowserIDAssertion(tokenURL, assertion, function(error, result) {
    if (error) {
      alert(error);
      return;
    }

    let {id: id, key: key, uid: uid, endpoint: endpoint} = result;

    ...
  });
(In reply to Gregory Szorc [:gps] from comment #13)
> This code isn't hooked up anywhere: it's just a standalone client.
> 
> If you load up a privileged JS console, you should be able to test things
> against an actual server (which I'll admit I haven't done - programming
> against the spec should be sufficient).
> 
> Try the following:
> 
>   Components.utils.import("resource://services-common/tokenserverclient.js");
> 
>   let client = new TokenServerClient();
>   let assertion = "BIDASSERTION";
>   let tokenURL = "https://stage-token.services.mozilla.com/1.0/aitc/1.0";
> 
>   client.getTokenFromBrowserIDAssertion(tokenURL, assertion, function(error,
> result) {
>     if (error) {
>       alert(error);
>       return;
>     }
> 
>     let {id: id, key: key, uid: uid, endpoint: endpoint} = result;
> 
>     ...
>   });

Got a syntax error when copying this into web console.
[21:55:39.223] SyntaxError: syntax error

And When i drop "https://stage-token.services.mozilla.com/1.0/aitc/1.0" into the browser, i get a 401 error:

{"status": 401, "errors": [{"description": "Unauthorized", "location": "body", "name": ""}]}

What am i missing?   I'm VPN'd into Mozilla-MPT and Mozilla-MV.

Tony
When I run it (albeit with an invalid token), I get (in the HTTP response body):

  application error: crash id e6fe4c5416ca068c862761c368b3e5aa

That's coming from the server all right. Not sure what is going on in server land.
Depends on: 731494, 743413
if I remove the "..." (which seems to be giving syntax error) I get:

[12:44:25.484] Error: Permission denied for <moz-safe-about:home> to get property XPCComponents.utils
https://hg.mozilla.org/mozilla-central/rev/eecb4cf4c45d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [qa+][fixed in services] → [qa+]
Target Milestone: --- → mozilla14
ah, I wasn't using a privileged console. With one, I get:

TokenServerClientServerError: Non 200 response code: 500
I was getting that the other day too. We'll have to ask the server people why it is returning 500. I'm sure there's something in the logs!
FWIW, I am also seeing the 500s in the AitC implementation, working with the server folks to figure out the problem.
Update: the API has changed slightly (specifically the response has different properties than the one auth.js is expecting). The latest API is documented here: http://docs.services.mozilla.com/token/apis.html

We need some tiny changes to auth.js to keep up with the server as it is deployed now, shall we create a new bug for those changes or just roll them in as part of the AitC client code (which is being tracked at bug 744257)?
(In reply to Anant Narayanan [:anant] from comment #21)

> We need some tiny changes to auth.js to keep up with the server as it is
> deployed now, shall we create a new bug for those changes or just roll them
> in as part of the AitC client code (which is being tracked at bug 744257)?

If it touches tokenserverclient, please file a new bug in Services > Firefox: Common.
Blocks: 744614
Blocks: 744627
Blocks: 907415
:gps or :rnewman
Is this still considered Resolved/Fixed?
Is there any work left for QA before marking this Verified?
Anything else that comes up can be a new bug, IMO…
Two votes makes it unanimous ;-)
Status: RESOLVED → VERIFIED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.