756657 - Get a token from the notification server

Status: RESOLVED WONTFIX

(Cloud Services Graveyard :: Server: WebPush, defect)

Description

[Jeff Balogh (:jbalogh)]

Attachment: [PATCH] implement getToken: retrieve auth token from push server

Here's the first part of the HTTP API for the notification server. Note this is not using tokenserver or browserid; the token is just a simple auth token (that'll change later).

I have more patches like this coming for other API pieces, so I'd like to make sure the client code and testing are sound.

This depends on the patch in bug 754062, which is still waiting on the releng bot.

Comment 1

[Gregory Szorc [:gps]]

Jeff: I'm sorry I haven't reviewed this yet. I've been roped into AITC reviews, which are a high priority. Please let me know if this is urgent. Otherwise, I may not get around to it for at least another week.

Comment 2

[Jeff Balogh (:jbalogh)]

(In reply to Gregory Szorc [:gps] from comment #1)
> Jeff: I'm sorry I haven't reviewed this yet. I've been roped into AITC
> reviews, which are a high priority. Please let me know if this is urgent.
> Otherwise, I may not get around to it for at least another week.

Take your time. But note that I'm hoarding a bunch more patches that I can rain down on you once this one gets unblocked. :)=

Comment 3

[Gregory Szorc [:gps]]

(In reply to Jeff Balogh (:jbalogh) from comment #2)
> Take your time. But note that I'm hoarding a bunch more patches that I can
> rain down on you once this one gets unblocked. :)=

Make it rain.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 625271 [details] [diff] [review]
[PATCH] implement getToken: retrieve auth token from push server

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

Just a lot of minor nits. Nothing too major. I think the next review will be r+.

::: services/notifications/service.js
@@ +17,5 @@
>    this.prefs = new Preferences(PREFS_BRANCH);
> +  this.DB = {};
> +
> +  this._log = Log4Moz.repository.getLogger("Notifications.Service");
> +  this._log.level = Log4Moz.Level[this.prefs.get("logger.level")];

This won't actually log things anywhere. See bug 760906 for an example implementation.

@@ +33,5 @@
> +  get serverURL() {
> +    let url = this.prefs.get("serverURL");
> +    let last = url.length - 1;
> +    return url[last] == "/" ? url.substring(0, last) : url;
> +  },

Does notifications use a hard-coded server URL or is the URL obtained when obtaining a new token from the token service? I'm just curious.

@@ +43,5 @@
> +  /**
> +   * Load database state from a file.
> +   */
> +  loadState: function loadState(cb) {
> +    CommonUtils.jsonLoad(this.filePath, this, function(json) {

Nit: Always give functions a name (it makes stack traces easier to understand).

@@ +45,5 @@
> +   */
> +  loadState: function loadState(cb) {
> +    CommonUtils.jsonLoad(this.filePath, this, function(json) {
> +      if (json) {
> +        this._log.info("Loading state from " + this.filePath);

s/Loading/Loaded/

You may also want to log before you call jsonLoad.

@@ +48,5 @@
> +      if (json) {
> +        this._log.info("Loading state from " + this.filePath);
> +        this.DB = json;
> +      }
> +      if (cb) cb(null, json);

Nit: Always use braces.

Also, if cb() raises, that exception may get swallowed. It is a best practice to catch exceptions in user-supplied callbacks and log any exceptions thrown. See CommonUtils for some helper functions to make this easy.

@@ +64,5 @@
> +  /**
> +   * Get an authentication token from the local DB or the push server.
> +   *
> +   * @param cb
> +   *        Callback function that runs on success or error.

Nit: Document the arguments and conditions for the callback.

@@ +69,5 @@
> +   */
> +  getToken: function getToken(cb) {
> +    if (this.DB.token) {
> +      this._log.debug("getToken from database");
> +      return cb(null, this.DB.token);

What if cb is not a function? In this case, it is probably fine. But, down below, it could result in breakage.

@@ +79,5 @@
> +      if (error) {
> +        return cb(error.message);
> +      }
> +      if (!this.response.success) {
> +        return cb("Service error: " + this.response.status);

Nit: When callbacks receive an error, always use Error instances, not raw strings.

@@ +83,5 @@
> +        return cb("Service error: " + this.response.status);
> +      }
> +      try {
> +        self.DB.token = JSON.parse(this.response.body).token;
> +        self.saveState();

Should you wait for the callback from saveState()?

::: services/notifications/tests/unit/head_helpers.js
@@ +37,5 @@
> +    if (body) {
> +      response.bodyOutputStream.write(body, body.length);
> +    }
> +  };
> +}

I don't like code that is copied and pasted wholesale. You can have your xpcshell tests do a relative import of services/common's head_helpers.js to obtain these functions.

@@ +52,5 @@
> +  if (!count) {
> +    count = inputStream.available();
> +  }
> +  return new BinaryInputStream(inputStream).readBytes(count);
> +}

This function now exists on CommonUtils. (At least it does in my tree - not sure if it has landed in s-c yet.)

::: services/notifications/tests/unit/test_get_token.js
@@ +12,5 @@
> +  _("Test getting the token from the server.");
> +  let request;
> +
> +  let server = httpd_setup({
> +    "/token/": function(req, response) {

Nit: name function

@@ +22,5 @@
> +
> +      let body = JSON.stringify({token: TOKEN});
> +      response.bodyOutputStream.write(body, body.length);
> +    }
> +  });

Instead of creating a dummy HTTP handler for every test, you may want to invest the time in a standalone JS HTTP server and just instantiate that. We have that for the storage service and AITC. It is quite nice because we have confidence the JS server behaves properly (because we can point the Python functional tests at it to confirm so). This means that an individual test case may not accidentally incorrectly implement an HTTP handler. It also makes the test code smaller.

@@ +26,5 @@
> +  });
> +
> +  Service.prefs.set("serverURL", TEST_SERVER_URL);
> +
> +  Service.getToken(function(error, token) {

Nit: name function

@@ +27,5 @@
> +
> +  Service.prefs.set("serverURL", TEST_SERVER_URL);
> +
> +  Service.getToken(function(error, token) {
> +    do_check_eq(error, null);

do_check_null (this function was created in the last 2 weeks). There are other uses in this file.

Comment 5

[Jeff Balogh (:jbalogh)]

Thanks for the review! Follow-up coming soon.

(In reply to Gregory Szorc [:gps] from comment #4)
> @@ +33,5 @@
> > +  get serverURL() {
> > +    let url = this.prefs.get("serverURL");
> > +    let last = url.length - 1;
> > +    return url[last] == "/" ? url.substring(0, last) : url;
> > +  },
> 
> Does notifications use a hard-coded server URL or is the URL obtained when
> obtaining a new token from the token service? I'm just curious.

The API root is hard-coded to something like "https://notifications.mozilla.org". When I want to connect to a websocket there's an API call to get a list of socket server addresses, so the root is the only thing hard-coded.

> @@ +43,5 @@
> > +  /**
> > +   * Load database state from a file.
> > +   */
> > +  loadState: function loadState(cb) {
> > +    CommonUtils.jsonLoad(this.filePath, this, function(json) {
> 
> Nit: Always give functions a name (it makes stack traces easier to
> understand).

Ah yeah, I forget that sometimes/often.

> @@ +69,5 @@
> > +   */
> > +  getToken: function getToken(cb) {
> > +    if (this.DB.token) {
> > +      this._log.debug("getToken from database");
> > +      return cb(null, this.DB.token);
> 
> What if cb is not a function? In this case, it is probably fine. But, down
> below, it could result in breakage.

If cb is not a function then you won't be able to get the auth token and your code will be broken. Do you think this is something I should be guarding against?

> @@ +83,5 @@
> > +        return cb("Service error: " + this.response.status);
> > +      }
> > +      try {
> > +        self.DB.token = JSON.parse(this.response.body).token;
> > +        self.saveState();
> 
> Should you wait for the callback from saveState()?

I just put the callback in there to make testing easier. Do you think I should wait for the callback to make sure all state is saved to disk before returning data to a webpage?
 
> ::: services/notifications/tests/unit/head_helpers.js
> @@ +37,5 @@
> > +    if (body) {
> > +      response.bodyOutputStream.write(body, body.length);
> > +    }
> > +  };
> > +}
> 
> I don't like code that is copied and pasted wholesale. You can have your
> xpcshell tests do a relative import of services/common's head_helpers.js to
> obtain these functions.

That makes me happier too. I didn't know about relative imports.
 
> @@ +26,5 @@
> > +  });
> > +
> > +  Service.prefs.set("serverURL", TEST_SERVER_URL);
> > +
> > +  Service.getToken(function(error, token) {
> 
> Nit: name function

If I give all my callbacks in tests the same name, have we really made progress? :)

Comment 6

[Gregory Szorc [:gps]]

(In reply to Jeff Balogh (:jbalogh) from comment #5)
> > @@ +69,5 @@
> > > +   */
> > > +  getToken: function getToken(cb) {
> > > +    if (this.DB.token) {
> > > +      this._log.debug("getToken from database");
> > > +      return cb(null, this.DB.token);
> > 
> > What if cb is not a function? In this case, it is probably fine. But, down
> > below, it could result in breakage.
> 
> If cb is not a function then you won't be able to get the auth token and
> your code will be broken. Do you think this is something I should be
> guarding against?

It all boils down to visibility. Sometimes if you are inside a function being executed from a callback and an exception is thrown (possibly due to trying to call a variable that isn't a function), that error can be swallowed and never heard from. This makes debugging stalled programs really hard.

We combat the problem through validating arguments where it makes sense (especially in functions that are part of a public API) and/or guarding each callback invocation with a try..catch.

If you aren't in a callback, I'm fine with the "garbage in garbage out" rule. If someone is dumb enough to pass a non-function to a required callback argument and that callback is executed in the primary call site of the function (not from a callback), then let things blow up! Of course, if you are calling a callback from within the main function invocation (not a callback), it often makes sense to return
data instead of invoking a (synchronous) callback.

> > @@ +83,5 @@
> > > +        return cb("Service error: " + this.response.status);
> > > +      }
> > > +      try {
> > > +        self.DB.token = JSON.parse(this.response.body).token;
> > > +        self.saveState();
> > 
> > Should you wait for the callback from saveState()?
> 
> I just put the callback in there to make testing easier. Do you think I
> should wait for the callback to make sure all state is saved to disk before
> returning data to a webpage?

I don't know! What are the consequences if the save fails? If it takes too long?

> > ::: services/notifications/tests/unit/head_helpers.js
> > @@ +37,5 @@
> > > +    if (body) {
> > > +      response.bodyOutputStream.write(body, body.length);
> > > +    }
> > > +  };
> > > +}
> > 
> > I don't like code that is copied and pasted wholesale. You can have your
> > xpcshell tests do a relative import of services/common's head_helpers.js to
> > obtain these functions.
> 
> That makes me happier too. I didn't know about relative imports.

Yeah, it is hacky but it works. Of course, [head] files can probably be replaced by testing-only JS modules (https://developer.mozilla.org/en/Developing_Tests#JavaScript_test-only_modules) once they actually work.

Comment 7

[Jeff Balogh (:jbalogh)]

Attachment: [PATCH] implement getToken: retrieve auth token from push server v2

I think the only suggestion I did not heed was about building a separate notification server in JS. Looking at some future patches, I'm usually writing handlers that fail in different ways, and it feels more obvious to have those contained within the test.

I put the callback into the save handler so clients don't get into a state where they give out notification URLs but don't have that stored on disk.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 635005 [details] [diff] [review]
[PATCH] implement getToken: retrieve auth token from push server v2

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

Looks good.

The timeout and back-off aspects will need to be solved before this service is enabled in Firefox. Could you at least insert a TODO referencing a follow-up bug. You may also want to come up with a Bugzilla whiteboard or tracking bug for notifications release blocking.

::: services/notifications/service.js
@@ +40,5 @@
> +    return url[last] == "/" ? url.substring(0, last) : url;
> +  },
> +
> +  init: function init() {
> +    this.loadState();

Do you want to use a callback, just in case?

@@ +104,5 @@
> +   */
> +  getToken: function getToken(cb) {
> +    let callback = this.callbackWrapper(cb);
> +    if (this.DB.token) {
> +      this._log.debug("getToken from database");

How about "Returning token from local database."

@@ +111,4 @@
>  
> +    let self = this;
> +    let url = this.serverURL + "/token/";
> +    new RESTRequest(url).post("", function onResponse(error) {

Do you care about the timeout here? The default timeout for RESTRequest is "no timeout." I think necko interprets this to be many minutes. It's probably a good idea to set something.

On that vein, what mechanism does the token server have to prevent the "thundering herd" problem? In other words, if the server is down or slow, what kind of back-off mitigation do you have?

@@ +130,5 @@
> +        });
> +      } catch (e) {
> +        return callback(e);
> +      }
> +    });

This is fine for now. But, if you start having more code that interacts with a remote service (especially a service that is well-defined), I'm going to have you split out all the HTTP/transport code into a separate module so it is all consolidated. If you don't do it this way, you inevitably duplicate code and make it hard to reconcile spec changes with your code because the code is scattered about.

@@ +135,4 @@
>    }
>  };
>  
>  let Service = new NotificationSvc();

Is it too late to name the exported symbol something with "notification" in it? Maybe "NotificationsService?" There is already a magic "Services" global and "Service" is both very similar and generic, IMO.

Sorry I didn't notice this in the original commit.

::: services/notifications/tests/unit/test_get_token.js
@@ +1,1 @@
> +function run_test() {

initTestLogging()

(assuming that is available - I think it is.

@@ +40,5 @@
> +
> +
> +add_test(function test_get_token_from_memory() {
> +  _("The token should be in Service.DB after the previous test.");
> +  do_check_eq(Service.DB.token, TOKEN);

In the ideal world, every test added through add_test() would be self-standing and would rely on no state from a previous test. May I suggest combining this with the previous test?

Comment 9

[Jeff Balogh (:jbalogh)]

(In reply to Gregory Szorc [:gps] from comment #8)
> ::: services/notifications/service.js
> @@ +40,5 @@
> > +    return url[last] == "/" ? url.substring(0, last) : url;
> > +  },
> > +
> > +  init: function init() {
> > +    this.loadState();
> 
> Do you want to use a callback, just in case?

I was wondering about whether I should include an isReady boolean that blocks API calls until the DB is loaded. Is there a common practice for that sort of thing?

> @@ +111,4 @@
> >  
> > +    let self = this;
> > +    let url = this.serverURL + "/token/";
> > +    new RESTRequest(url).post("", function onResponse(error) {
> 
> Do you care about the timeout here? The default timeout for RESTRequest is
> "no timeout." I think necko interprets this to be many minutes. It's
> probably a good idea to set something.

Oh yes, I'll attach another patch that has a proper timeout.

> On that vein, what mechanism does the token server have to prevent the
> "thundering herd" problem? In other words, if the server is down or slow,
> what kind of back-off mitigation do you have?

It doesn't. For this patch, the API call would occur as a result of user/website interaction: the website asks for permission to send notifications, the user says yes, so we need to set up an auth token and create a push URL.

For opening websocket connections, I'm going to have randomized reconnect delays + backoff to avoid the herd.

> @@ +130,5 @@
> > +        });
> > +      } catch (e) {
> > +        return callback(e);
> > +      }
> > +    });
> 
> This is fine for now. But, if you start having more code that interacts with
> a remote service (especially a service that is well-defined), I'm going to
> have you split out all the HTTP/transport code into a separate module so it
> is all consolidated. If you don't do it this way, you inevitably duplicate
> code and make it hard to reconcile spec changes with your code because the
> code is scattered about.

I have more API calls in my patch queue, so I'll look at splitting it out.

> @@ +135,4 @@
> >    }
> >  };
> >  
> >  let Service = new NotificationSvc();
> 
> Is it too late to name the exported symbol something with "notification" in
> it? Maybe "NotificationsService?" There is already a magic "Services" global
> and "Service" is both very similar and generic, IMO.

Yeah, I'll fix it in an updated patch.

> @@ +40,5 @@
> > +
> > +
> > +add_test(function test_get_token_from_memory() {
> > +  _("The token should be in Service.DB after the previous test.");
> > +  do_check_eq(Service.DB.token, TOKEN);
> 
> In the ideal world, every test added through add_test() would be
> self-standing and would rely on no state from a previous test. May I suggest
> combining this with the previous test?

The Firefox test environment seemed like a particularly non-ideal world since it doesn't have per-test setup and teardown. Do you manually reset global state in other test suites?

Comment 10

[Gregory Szorc [:gps]]

(In reply to Jeff Balogh (:jbalogh) from comment #9)
> > Do you want to use a callback, just in case?
> 
> I was wondering about whether I should include an isReady boolean that
> blocks API calls until the DB is loaded. Is there a common practice for that
> sort of thing?

I'd say the common practice is to use a callback and not do anything until that callback is called.

Adding a guard is find. And, it is probably encouraged in cases where there could be a memory leak or you could do significant harm, etc. For internal code like service/singletons, I don't think it is worth the hassle: just create the instance properly.

Another crafty solution would be to install an internal callback that finished setting up the state of the instance. e.g. you could replace the instance's .prototype in the callback for the real one. The instantiated instance would not have a real prototype. After initialization, the new prototype would be fully-featured. That way, if the caller attempted to call a function before initialization was complete, they would be calling null and would get an exception. I haven't seen anyone do this. But I think it is a nifty solution and I would r+ it if the code is clean.

> > @@ +40,5 @@
> > In the ideal world, every test added through add_test() would be
> > self-standing and would rely on no state from a previous test. May I suggest
> > combining this with the previous test?
> 
> The Firefox test environment seemed like a particularly non-ideal world
> since it doesn't have per-test setup and teardown. Do you manually reset
> global state in other test suites?

You are correct that the xpcshell test API is teh suck. I've wanted to utilize a proper test harness for a while now. If I had infinite time...

In other tests, I try to not rely on state between tests. There may be global variables in a test file. But, I try to make things such that the add_test()-added tests could be executed in any order and things will still work. I will concede that most people don't do this and rely on consistent test ordering. I argue this is bad because it can cover up legitimate bugs (never mind that we don't support random execution today).

Comment 11

[JR Conlin [:jrconlin,:jconlin]]

I believe that this can be closed as the context has changed and both parties are no longer involved with this.