Loop no longer resets hawk session token on invalid token

RESOLVED FIXED in mozilla33

Status

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1])

Attachments

(1 attachment, 1 obsolete attachment)

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).
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.
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.
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.
Attachment #8456950 - Flags: review?(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." :)
Attachment #8456950 - Flags: review?(dolske) → feedback+
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?
Attachment #8456950 - Flags: feedback+
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/
Attachment #8456950 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a79533c6430c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Does this need QA testing or is the automation coverage sufficient?
Whiteboard: [p=1] → [p=1][qa?]
Flags: qe-verify?
Whiteboard: [p=1][qa?] → [p=1]
Mark, can you please answer comment 10?
Flags: needinfo?(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.
Flags: needinfo?(standard8)
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.