1017902 - Handed-out links should stay valid across link-generator browser restarts

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

MozLoopService.jsm currently doesn't look to see if a loop.hawk-session-token exists and append it to the XHR query on registration.  This means that every startup of the browser registers a brand session, effectively orphaning any previous session (ie old links handed out will no longer work).

It should instead read the pref, and try attaching it request.  Not sure if/how expired session tokens would be dealt with most effectively in this context.

Comment 1

[Mark Banner (:standard8)]

Moving to MLP, I don't think this can wait until mvp.

Comment 2

[Mark Banner (:standard8)]

Attachment: WIP Initial Hawk remembering details

This is a draft patch based on using the HAWKAuthenticatedRESTRequest from services' common code. It seems to work so far, I'll probably consider using HawkClient instead of HAWKAuthenticatedRESTRequest for the final version, once I look at tidying it up a bit further.

Comment 3

[Mark Banner (:standard8)]

Attachment: Use saved hawk session tokens, if possible, when registering with the server to preserve the session across restarts.

This changes the MozLoopService to handle sending the hawk session tokens to the server if they are supplied.

I was considering using hawkclient.js (which is a wrapper around HAWKAuthenticatedRESTRequest), but that currently doesn't give access to the headers in the results, and it raises the question of if we should be handling clock skew on the gecko and content side of our server connection in a uniform way. As hawk seems to be generally fine with timestamps at the moment, I'm going to suggest that we push this out to a later bug and get those who know hawk better than I do to investigate and tell us if there's going to be a longer term issue or not (if there is, its likely going to be covering changing both content and gecko sides of loop, which would be a different bug anyway).

I think we still might switch to HawkClient anyway, especially as it uses promises, but again, I think that should be a separate bug, as we'd need to get it modified and check we didn't breaking the FxA code at the same time.

A lot of the test changes are to avoid boilerplate where possible. test_loopservice_token_save.js was split out from test_loopservice_registration.js as it was preventing the former correctly running all its tests (as registration had completed). test_loopservice_token_send.js is the new test to go with this change.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8433312 [details] [diff] [review]
Use saved hawk session tokens, if possible, when registering with the server to preserve the session across restarts.

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

I'm not sure that we shouldn't bite the bullet with changes to hawkclient to support this use-case, and possibly even refactor things so deriveHawkCredentials from FxAccountsClient can be used here.  Also, I'm concerned about not handling clock-skew as this caused all kinds of hard-to-diagnose grief for FxA.  Jed, you have a good understanding of these issues, what do you think?

::: browser/components/loop/MozLoopService.jsm
@@ +365,5 @@
> +   * @param {String} pushUrl The push url given by the push server.
> +   * @param {Boolean} noRetry Optional, don't retry if authentication fails.
> +   */
> +  registerWithLoopServer: function(pushUrl, noRetry) {
> +    var sessionToken;

should use 'let' in this function

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8433312 [details] [diff] [review]
Use saved hawk session tokens, if possible, when registering with the server to preserve the session across restarts.

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

This looks good to me, Mark.

I think you will want to do something about clock skew (see notes below).

Apart from that, I have just a few questions.

I think the patch does what is intended, and I don't want you to be blocked, so r=me.  I would nevertheless be glad to take another look and dig further into the clock skew business with you, if you like, on subsequent iterations.

::: browser/components/loop/MozLoopService.jsm
@@ +212,5 @@
>    /**
> +   * Returns true if the expiry time is in the future.
> +   */
> +  urlExpiryTimeIsInFuture: function() {
> +    return this.expiryTimeSeconds * 1000 > Date.now();

I agree with :markh that clock skew makes things a little tricky here.

The hawkclient provides a localtimeOffsetMsec() getter that may be useful.  With every request that comes back, the client compares the server's date with the reported current local time, and adjusts the offset accordingly.  (This means the first request could fail if the clocks are way off, but there's nothing to be done about that.)

So the expiration of a token would be computed as:

    now() + hawkClient.localtimeOffsetMsec() + duration

I don't know if this will solve all your possible problems, but hopefully it will help.

@@ +375,2 @@
>  
> +    var credentials;

Again, use 'let'?

@@ +380,2 @@
>  
> +    this.loopXhr = new HAWKAuthenticatedRESTRequest(this.loopServerUri + "/registration",

Rather than string concatenation, consider using

  Services.io.newURI(this.loopServerUri, null, null).resolve("/registration")

to make nice urls in the event that someone sets the loop.server pref to something ending in a '/' etc.

@@ +390,5 @@
> +        }
> +
> +        // Authorization failed, invalid token, we need to try again with a new token.
> +        Services.prefs.clearUserPref("loop.hawk-session-token");
> +        this.registerWithLoopServer(pushUrl, true);

It seems that, in some perverse situation, this could get you into an infinite loop.

::: browser/components/loop/test/xpcshell/test_loopservice_token_save.js
@@ +26,5 @@
> +    try {
> +      hawkSessionPref = Services.prefs.getCharPref("loop.hawk-session-token");
> +    } catch (ex) {
> +      // avoid slowing/hanging the tests if the pref isn't there
> +      dump("unexpected exception: " + ex + "\n");

or maybe fail explicitly here?

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Mark Hammond [:markh] from comment #4)

> I'm not sure that we shouldn't bite the bullet with changes to hawkclient to
> support this use-case, and possibly even refactor things so
> deriveHawkCredentials from FxAccountsClient can be used here.

That consolidation sounds like a good idea to me.  I'd be glad to help out if you decide to go down that path.

Comment 7

[Mark Banner (:standard8)]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #5)
> ::: browser/components/loop/MozLoopService.jsm
> @@ +212,5 @@
> >    /**
> > +   * Returns true if the expiry time is in the future.
> > +   */
> > +  urlExpiryTimeIsInFuture: function() {
> > +    return this.expiryTimeSeconds * 1000 > Date.now();
... 
> The hawkclient provides a localtimeOffsetMsec() getter that may be useful. 
> With every request that comes back, the client compares the server's date
> with the reported current local time, and adjusts the offset accordingly. 
> (This means the first request could fail if the clocks are way off, but
> there's nothing to be done about that.)

So the bit of code here, isn't actually to do with the hawk authentication. It is to handle urls that we supply to the user, and when they expire.

> @@ +380,2 @@
> >  
> > +    this.loopXhr = new HAWKAuthenticatedRESTRequest(this.loopServerUri + "/registration",
> 
> Rather than string concatenation, consider using
> 
>   Services.io.newURI(this.loopServerUri, null, null).resolve("/registration")
> 
> to make nice urls in the event that someone sets the loop.server pref to
> something ending in a '/' etc.

Ooh, that sounds nice.

> @@ +390,5 @@
> > +        }
> > +
> > +        // Authorization failed, invalid token, we need to try again with a new token.
> > +        Services.prefs.clearUserPref("loop.hawk-session-token");
> > +        this.registerWithLoopServer(pushUrl, true);
> 
> It seems that, in some perverse situation, this could get you into an
> infinite loop.

That's why I added the additional parameter, I'm not sure I could see a way to hit an infinite loop with that.

> ::: browser/components/loop/test/xpcshell/test_loopservice_token_save.js
> @@ +26,5 @@
> > +    try {
> > +      hawkSessionPref = Services.prefs.getCharPref("loop.hawk-session-token");
> > +    } catch (ex) {
> > +      // avoid slowing/hanging the tests if the pref isn't there
> > +      dump("unexpected exception: " + ex + "\n");
> 
> or maybe fail explicitly here?

Shouldn't be there at all, I've fixed this already in another patch, will update this one in a bit.

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Mark Banner (:standard8) from comment #7)

> That's why I added the additional parameter, I'm not sure I could see a way
> to hit an infinite loop with that.

Oh whoops!  I don't know how I missed that.  Perfect.

Comment 9

[Mark Banner (:standard8)]

Attachment: Use saved hawk session tokens, if possible, when registering with the server to preserve the session across restarts. v2

Updated for bitrot on Elm & for comments. It looks like we're going to be wanting to move all of our calls to go via the MozLoopService for various reasons, so I think it makes sense to follow-up with something that uses HawkClient and the goodness there.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8434496 [details] [diff] [review]
Use saved hawk session tokens, if possible, when registering with the server to preserve the session across restarts. v2

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

OK, I'm sold for MLP, but please open a followup which blocks MVP to use hawkclient (and whatever hawk related changes are necessary).  Please copy me, jedp and ckarlof on that new bug.

Comment 11

[Mark Banner (:standard8)]

Filed bug 1020859 and bug 1020876 on follow-ups for using hawkclient.

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/1439cebf74e1

Closing for tracking purposes.

Comment 13

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/1439cebf74e1

Comment 14

[u279076]

Does this need manual testing or is in-testsuite coverage sufficient?

Comment 15

[u279076]

Dan, can you please answer comment 14?

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Yes, this sounds like a good thing to manually test.  The steps to test:

* start the browser
* drop down the loop panel
* copy the URL
* quit the browser
* restart the browser

In another browser (either a different Profile, or just use Chrome):

* paste the link into the URL bar
* click the Start button
* verify that the call can be completed

Comment 17

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #16)
> Yes, this sounds like a good thing to manually test.  The steps to test:
> 
> * start the browser
> * drop down the loop panel
> * copy the URL
> * quit the browser
> * restart the browser

Just to note, there's a 5 second registration delay on starting the browser where you won't be able to receive anything until that happens.

Comment 18

[u279076]

Sorry this took so long, it fell off my radar. I've now tested this and it works as expected.