727210 - Implement token server client for Sync

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Implement token server client for Sync

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.

Comment 1

[Tarek Ziadé (:tarek)]

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.

Comment 2

[Tarek Ziadé (:tarek)]

(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 3

[Richard Newman [:rnewman]]

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?

Comment 4

[Gregory Szorc [:gps]]

(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.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Implement token server client for Sync, v2

Latest version. Incorporated all of feedback.

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Token server client, v3

* Moved to services/common \o/
* Reflected changes to JSON fields in HTTP response
* Misc docs
* Added example usage (not perfect, but better than nothing)

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 612981 [details] [diff] [review]
Token server client, v3

Added pref to services-sync.js has been removed locally.

Comment 9

[Richard Newman [:rnewman]]

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

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/eecb4cf4c45d

On behalf of AITC, I thank you for doing this review.

Comment 11

[Richard Newman [:rnewman]]

(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!

Comment 12

[Tony Chung [:tchung]]

gps, can we get some qa+ steps to test this patch?  as detailed as possible, always helps!

Comment 13

[Gregory Szorc [:gps]]

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;

    ...
  });

Comment 14

[Tony Chung [:tchung]]

(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

Comment 15

[Gregory Szorc [:gps]]

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.

Comment 16

[Tracy Walker [:tracy]]

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

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/eecb4cf4c45d

Comment 18

[Tracy Walker [:tracy]]

ah, I wasn't using a privileged console. With one, I get:

TokenServerClientServerError: Non 200 response code: 500

Comment 19

[Gregory Szorc [:gps]]

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!

Comment 20

[Anant Narayanan [:anant]]

FWIW, I am also seeing the 500s in the AitC implementation, working with the server folks to figure out the problem.

Comment 21

[Anant Narayanan [:anant]]

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)?

Comment 22

[Richard Newman [:rnewman]]

(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.

Comment 23

[James Bonacci [:jbonacci]]

:gps or :rnewman
Is this still considered Resolved/Fixed?
Is there any work left for QA before marking this Verified?

Comment 24

[Richard Newman [:rnewman]]

Anything else that comes up can be a new bug, IMO…

Comment 25

[James Bonacci [:jbonacci]]

Two votes makes it unanimous ;-)