1038699 - Loop no longer resets hawk session token on invalid token

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

The server changed the response for /register to a json response and added more explicit errors.

We have code that detects the errors, and resets the hawk session token if necessary. This shouldn't happen in production normally, though changes in the hawk auth models mean that this may happen for the 0.5.0 to 0.9.0 upgrade.

(The code is also useful atm for developers when switching servers).

Comment 1

[Adam Roach [:abr]]

When we do this, we should surface it to the user in some way. Basically, if we change the Hawk token, then any call-back URLs issued by non-logged-in users have just been invalidated. It would be better if the browser told them this rather than having to find out by themselves.

Comment 2

[Rémy Hubscher (:natim)]

If you are logging in with your Firefox Account you keep your call-urls.

Also CallUrl changed between 0.5.0 from token base64 inside the url to data in the database with Short URLs.

At the moment 0.9.0 is not able to handle old Call Url anyway.

Comment 3

[Mark Banner (:standard8)]

Attachment: Loop no longer resets the hawk session token when it is invalid. Handle the new server responses.

As documented in comment 0, the server changed its error response body from "Unauthorized" to a json object:

{ code: 401, errno: 110, error: { error: "Unauthorized", message: "Unknown credentials", statusCode: 401 } }

As a result, the hawk client code passes us this object, rather than one it constructed, so error.errno is no longer 401.

This patch updates us to the new style of return message. It also explicitly limits us to resetting the token in the 401 and errno 110 case, as for other errnos it isn't appropraite to reset the token.

I discussed with Abr and Darrin about notifying the user, and we have a way to do that, but because of related in-progress bugs, we'll file a follow up and handle the UX there.

For now, we need this so that we can update the server and not completely break existing users.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8456950 [details] [diff] [review]
Loop no longer resets the hawk session token when it is invalid. Handle the new server responses.

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

Hmm, this is pretty domain-specific, so I'd suggest another Loop developer would be a more appropriate reviewer. But generally seems fine, so feedback+, and if you're looking for a module peer rubberstamp, consider that "r+ if you can get r+ from another Loop dev." :)

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8456950 [details] [diff] [review]
Loop no longer resets the hawk session token when it is invalid. Handle the new server responses.

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

Looks good. Only nits which may optionally be addressed.

::: browser/components/loop/MozLoopService.jsm
@@ +6,5 @@
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
> +// Invalid auth token as per
> +// https://github.com/mozilla-services/loop-server/blob/master/loop/errno.json

Nit: Please link to a given commit, eg. https://github.com/mozilla-services/loop-server/blob/45787d34108e2f0d87d74d4ddf4ff0dbab23501c/loop/errno.json#L2

@@ +250,5 @@
>          // to re-register.
>        }, (error) => {
> +        // There's other errors than invalid auth token, but we should only do the reset
> +        // as a last resort.
> +        if (error.code == 401 && error.errno == INVALID_AUTH_TOKEN) {

Nit: Could these ever be strings? If they don't, using strict check is probably safe.

::: browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
@@ +40,5 @@
> +    Assert.equal(authorizationAttempts, 2);
> +    Assert.equal(Services.prefs.getCharPref(LOOP_HAWK_PREF), fakeSessionToken2);
> +    run_next_test();
> +  }, err => {
> +    do_throw("shouldn't be a failure result");

Nit: how about adding the error string to that message?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8456950 [details] [diff] [review]
Loop no longer resets the hawk session token when it is invalid. Handle the new server responses.

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

Looks good; thanks.  r=dmose with Niko's suggestions also addressed and the spin-off bug filed.

::: browser/components/loop/MozLoopService.jsm
@@ +6,5 @@
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
> +// Invalid auth token as per
> +// https://github.com/mozilla-services/loop-server/blob/master/loop/errno.json

Or, in this case errno.json#L6

::: browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
@@ +24,5 @@
> +          statusCode: 401
> +        }
> +      }));
> +    } else {
> +      // We didn't have an authorization head, so check the pref has been cleared.

s/head/header/

Comment 7

[Mark Banner (:standard8)]

Attachment: Loop no longer resets the hawk session token when it is invalid. Handle the new server responses. v2

Updated version, waiting for tree to re-open.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a79533c6430c

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[u279076]

Does this need QA testing or is the automation coverage sufficient?

Comment 11

[u279076]

Mark, can you please answer comment 10?

Comment 12

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> Does this need QA testing or is the automation coverage sufficient?

I think the automation is sufficient here, its enough of an integration test that it can manage functionally as well.