1226449 - A 401 during email status poll should invalidate the session token

Status: RESOLVED WORKSFORME

(Firefox :: Sync, defect, P1)

Description

[Ryan Kelly [:rfkelly]]

Discovered in https://github.com/mozilla/fxa-content-server/issues/2002#issuecomment-158250210

STR:

* In a fresh profile, go to set up sync and create a new account
* Enter an email that will hard-bounce, like "[e-mail]"
* Proceed to create the account
* Wait about 10 seconds for the verification email to bounce, and the server to delete the account in response

Actual result:

* The browser says that "[e-mail]" is signed in but not verified

Expected result:

* The browser should detect that the account has gone away and boot you out of the "connected" state.


In browser logs, I can see the email status polling detecting that the account has gone away by getting a 401:


    INFO: 1447982022726	Sync.BrowserIDManager	INFO	Waiting for user to be verified.
    INFO: 1447982023002	Hawk	DEBUG	(Response) /recovery_email/status: code: 200 - Status text: OK
    INFO: 1447982023003	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -1003
    INFO: 1447982023003	FirefoxAccounts	DEBUG	checkEmailStatus -> {"email":"[e-mail]","verified":false}
    INFO: 1447982023004	FirefoxAccounts	DEBUG	polling with timeout = 5000
    INFO: 1447982028005	FirefoxAccounts	DEBUG	entering pollEmailStatus: timer
    INFO: 1447982028005	FirefoxAccounts	DEBUG	entering pollEmailStatus: timer
    INFO: 1447982028005	FirefoxAccounts	DEBUG	entering pollEmailStatus: timer
    INFO: 1447982028243	Hawk	DEBUG	(Response) /recovery_email/status: code: 401 - Status text: Unauthorized
    INFO: 1447982028243	Hawk	DEBUG	(Response) /recovery_email/status: code: 401 - Status text: Unauthorized
    INFO: 1447982028243	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -243
    INFO: 1447982028243	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -243
    INFO: 1447982028243	Hawk	DEBUG	(Response) /recovery_email/status: code: 401 - Status text: Unauthorized
    INFO: 1447982028243	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -243
    INFO: 1447982028244	Hawk	DEBUG	Received 401 for /recovery_email/status: retrying
    INFO: 1447982028244	Hawk	DEBUG	Received 401 for /recovery_email/status: retrying
    INFO: 1447982028244	Hawk	DEBUG	Received 401 for /recovery_email/status: retrying
    INFO: 1447982028490	Hawk	DEBUG	(Response) /recovery_email/status: code: 401 - Status text: Unauthorized
    INFO: 1447982028490	Hawk	DEBUG	(Response) /recovery_email/status: code: 401 - Status text: Unauthorized
    INFO: 1447982028490	Hawk	DEBUG	(Response) /recovery_email/status: code: 401 - Status text: Unauthorized
    INFO: 1447982028490	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -490
    INFO: 1447982028490	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -490
    INFO: 1447982028490	Hawk	DEBUG	Clock offset vs https://api.accounts.firefox.com/v1: -490
    INFO: 1447982028491	FirefoxAccounts	ERROR	error GETing /recovery_email/status: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
    INFO: 1447982028491	FirefoxAccounts	ERROR	error GETing /recovery_email/status: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
    INFO: 1447982028491	FirefoxAccounts	ERROR	error GETing /recovery_email/status: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
```

But this doesn't seem to trigger the browser to discard its now-invalid session token and ask for re-authentication.

(It's also not clear to me why the error is reported three times, I hope this doesn't mean we had three status poll requests in-flight simultaneously...)

Comment 1

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

There's a UX consideration here too - it would seem a little hostile to simply make Sync revert back to a "no user" state - presumably the user doesn't know they entered an invalid address, so would probably just assume they hit a client bug. I'm not sure what the friendly thing to do is though...

Comment 2

[Ryan Kelly [:rfkelly]]

Indeed.  There's a bit of related discussion in the linked github issue.

Comment 3

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

(In reply to Ryan Kelly [:rfkelly] from comment #2)
> Indeed.  There's a bit of related discussion in the linked github issue.

It looks from that issue that the auth-server doesn't quite handle this ideally, but a combination of:

* browser enters "needs reauth" state
and
* auth server shows something sane as this user attempts to re-login with the (now deleted) account name

is probably OK.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Ryan Kelly [:rfkelly] from comment #0)

> * Enter an email that will hard-bounce, like
> "[e-mail]"

That's my new alias, FYSA.

Comment 5

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

So a complication here is that we already have code in place to check if the account exists after verification - the idea is to automagically log out if the account is deleted from a different device (bug 999190). So I think we are going to make that state also enter needs reauth.

I made a patch to do that - the auth server then simply says "Unknown Account" which isn't ideal. But worse, there's no UI on that page to disconnect their account or create a new one - so they remain stuck - they'll need to guess they need to close the auth-server tab, go back to Sync prefs, then click "Forget this Email" and start from scratch - which is a significantly worse outcome for users who deleted their account elsewhere - the current behaviour of silently disconnecting is better.  So I'm starting to think we should either silently disconnect for this case, or add another state to the browser to reflect "account has gone away".

Comment 6

[Chris Karlof [:ckarlof]]

If the data in https://bugzilla.mozilla.org/show_bug.cgi?id=1232522 is accurate, this suggests that 10% of new signups will be affected by this bug.

Comment 7

[Chris Karlof [:ckarlof]]

If the user mistyped her email during signup, just silently removing her account from Firefox might be surprising. Ryan, should we introduce another state which messages the user on the sync preferences pane that there was an error trying to send her an email (possibly using the Desktop notifications as well) and ask her to sign up again (or dismiss)?

Comment 8

[Ryan Feeley [:rfeeley]]

Attachment: mistyped.png

I agree that removing the account + a desktop notification that points to about:accounts is likely the best direction

Title: Mistyped email?
Body: Your Sync verification email was returned. Please re-register with a valid email.
(in longer languages "Please re-register" is fine on its own).
Link: about:accounts?action=signup&entrypoint=bouncenotification?

Comment 9

[Chris Karlof [:ckarlof]]

Ryan, if the user misses the notification, do we need a persistent message on about:preferences#sync describing the problem? Otherwise, about:preferences#sync would just show the logged out state after we detect the deleted account.

Comment 10

[Ryan Feeley [:rfeeley]]

We do have a in-content messages that we can use (they were used during Sync migration).

We could persist a message like: "The account for [e-mail] has been removed" with a way to manually remove it.

We could also automatically remove it when the user successfully adds another account.

I would also like this feature to work for:

1. users who have deleted their account
2. registrants who have entered the wrong email and clicked the "Wrong email? Start over" link (that we should offer)

Comment 11

[Edwin Wong [:edwong]]

from sync eng mtg: rfeeley to finalize UI for specific workflows

Comment 12

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

(In reply to Edwin Wong [:edwong] from comment #11)
> from sync eng mtg: rfeeley to finalize UI for specific workflows

Note also that we decided this bug should solve only comment 0 (ie, when a password has been changed on a different device but the account remains intact) and thus should have no UI/UX changes at all. We then want another bug to offer a better UX for when we find an account is actually deleted as part of this process.

Comment 13

[Ryan Feeley [:rfeeley]]

Proposed UX approach:
1. Show the push notification as it appears in #c8 and have it link to about:accounts?action=signup&entrypoint=bouncenotification (entry point?)
2. Apply the warning (yellow, caution icon) state to the hamburger menu with the text "Verification email returned" which links to about:preferences#sync
3. Using the in-content notification bar styles established for sync migration, show a dismissable message with "Your Sync verification email was returned. Please re-register with a valid email." with the of sync preferences in original state.

Comment 14

[Chris Karlof [:ckarlof]]

(In reply to Mark Hammond [:markh] from comment #12)
> (In reply to Edwin Wong [:edwong] from comment #11)
> > from sync eng mtg: rfeeley to finalize UI for specific workflows
> 
> Note also that we decided this bug should solve only comment 0 (ie, when a
> password has been changed on a different device but the account remains
> intact) and thus should have no UI/UX changes at all. We then want another
> bug to offer a better UX for when we find an account is actually deleted as
> part of this process.

Mark, are you confusing this with bug 1044530? In comment 0, the user has created an account with a bogus email, not changed her password.

Comment 15

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

(In reply to Chris Karlof [:ckarlof] from comment #14)
> Mark, are you confusing this with bug 1044530? In comment 0, the user has
> created an account with a bogus email, not changed her password.

Oops, I am, sorry.

Comment 16

[Victor Ng [:vng] [:crankycoder]]

Attachment: Turn on the yellow warning badge on hamburger menu on 401 error

Initial patch to put up the yellow warning badge on the hamburger menu on a 401 error caused by hard email bounce.

I'm still wrapping my head around mochitest and this code neeeds a testcase under services/fxaccounts/tests/mochitest/test_invalidEmailCase.html

Comment 17

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

Comment on attachment 8726368 [details] [diff] [review]
Turn on the yellow warning badge on hamburger menu on 401 error

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

::: services/fxaccounts/FxAccounts.jsm
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +/* global Components: true, XPCOMUtils: true */

what's this line for?

@@ +1061,5 @@
>            this.pollEmailStatusAgain(currentState, sessionToken, timeoutMs);
>          }
> +
> +        // The server will return 401 on a hard bounce of an email
> +        // address.  We need to inform the user that the 

nit - trailing whitespace here.

@@ +1065,5 @@
> +        // address.  We need to inform the user that the 
> +        // signin has failed with a UI prompt that the authentication
> +        // needs a refresh.
> +        if (error && error.code && error.code == 401) {
> +            this.notifyObservers(UI_REQUEST_REFRESH_AUTH);

We should create a new observer notification for this. This constant is used only by FxAccountsManager.jsm, which is only used by b2g (it's a bit of a mess really)

It also looks like there should be a change that listens for this notification?

I think FxAccountsCommon should probably grow a ONAUTHCHANGED_NOTIFICATION (or similar) constant, and browser-fxaccounts.js should listen for it. updateUI in that function will probably need to check what the state is and update accordingly. I think you said you had that working, but that part seems to be missing from the patch.

Also, updating the UI isn't itself going to fix this bug as FxAccounts is still going to report the user is signed in. Also, the notification will work for, eg, the hamburger menu, but when the user next visits about:prefs#sync, it will not be able to determine the user has been deleted.

Kit is handling a similar situation in bug 1044530, where he detects if the user has been deleted and just does an explicit signout. I wonder if we should consider adding a .deleted flag to the signedInUser state here - then both this case and Kit's case would allow for the UI to notice the .deleted flag and update the UI accordingly - there would still technically be a signed in user (and the user would need to explicitly unlink/disconnect it) but at least the UI could adapt sanely - as I mentioned in comment 1, I don't think an auto-disconnect is appropriate in this case.

Comment 18

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

Kit, see comment 17 - I wonder what you think about adding an explicit .deleted flag to the signedInUser state instead of silently and automatically disconnecting the user?

Comment 19

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #18)
> Kit, see comment 17 - I wonder what you think about adding an explicit
> .deleted flag to the signedInUser state instead of silently and
> automatically disconnecting the user?

I think that makes a lot of sense. This would also address Chris' concern from comment 9 about the user missing the "account deleted" notification, and later wondering, "why isn't Sync working?"

Victor, would you like to handle that in your patch, or should I add it in bug 1044530?

Comment 20

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

Kit's recent work covered this - if we see a 401 checking the account status we will drop the session token, forcing us into a needs-reauth state.