999190 - Deleting an account does not clear the Sync Prefs account information.

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[James Bonacci [:jbonacci]]

Going through the FxA/Sync account deletion flow does not update the FxA/Sync preferences page.

I can delete an account, but the email listed on the Prefs page is still the old account.
Even if I create a new account or sign in with a different current account, the preferences page will still show the old email.
I even tried to restart the browser to clear this field, but it did not clear.

Now if I attempt to create a new account, I still see the old account email in the prefs page. The only way to "clear" this appears to be by clicking on the "Forget this email" link.
Once I do that, I get the nasty yellow bar and a 401 before I am switched over to the new account info so that I can complete a sync.

STRs:
1. Delete the current account
2. Restart the browser
3. Sign in with a current account or a new account
4. Go to Prefs > Sync and note that the old account is still listed.
5. Try syncing - this is successful even though the logs show that sync appears to be happening with the old/deleted account
6. To clear the old account info, click on Prefs > Sync > "Forget this email"
7. Get a yellow bar and a 401
8. The new account will now work for syncing.

Comment 1

[James Bonacci [:jbonacci]]

That sequence of steps appears to give me this error in the console (terminal) when I close Firefox:
1398119064656	Sync.BrowserIDManager	ERROR	Non-authentication error in _fetchTokenForUser: assertion argument is not valid.
1398119064660	Sync.BrowserIDManager	ERROR	Failed to fetch a token for authentication: TokenServerClientError({"message":"assertion argument is not valid."})
1398119064796	Sync.BrowserIDManager	ERROR	Non-authentication error in _fetchTokenForUser: assertion argument is not valid.
1398119064798	Sync.BrowserIDManager	ERROR	Failed to fetch a token for authentication: TokenServerClientError({"message":"assertion argument is not valid."})
1398119064849	FirefoxAccounts	ERROR	error POSTing /session/destroy: {"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"}
1398119064849	FirefoxAccounts	ERROR	Error during remote sign out of Firefox Accounts: [object Object]

Comment 2

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

This is a bit of a paper-cut for people who want to delete their account, so let's target 30.  I doubt it will be too hard, so taking it.

Comment 3

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

Actually, I think we can fix this simply by having the accounts.firefox.com code arrange to send us an event with the 'sign_out' command, causing Fx to perform a logout when this happens.  If this turns out to be true, an added bonus is that it will also fix the issue for 29 - ie, it will be fixed without any changes to Fx itself.

Chris, can you help find someone who can help with that?

Comment 4

[Chris Karlof [:ckarlof]]

> Actually, I think we can fix this simply by having the accounts.firefox.com code arrange to send us an event with the 'sign_out' command, causing Fx to perform a logout when this happens

This would be a good thing to have. 

Since delete account happens from the web, this approach would require having the browser listen for events from accounts.firefox.com (similar to Bug 998051).

However, this doesn't address making the UX better for "signed in browsers" when the user deletes her accounts somewhere else (e.g., on Safari). 

If we want to address Bug 998051 in whole or in part, I can provide resources for the hosted code side.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Obviously we'll want this across all branches, so tracking all the way to 32, while still targeting 30.  This is something that should get landed in the first couple of weeks of Beta (before May 16th) so that we can get testing and check for regressions in time to pull if need be.

Comment 7

[Tracy Walker [:tracy]]

Any progress here?  We are approaching half way through the 30 beta cycle.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to James Bonacci [:jbonacci] from comment #0)
> STRs:
> 1. Delete the current account
> 2. Restart the browser
> 3. Sign in with a current account or a new account

How do you sign in at this step of the process? I would have thought that the Firefox UI would still be in the "signed in state", which doesn't offer sign-in UI.

I think we probably don't want to track this for 30 at this point.

Comment 9

[James Bonacci [:jbonacci]]

Ah, so in the original issue, I was able to click on the Sync icon to bring up the FxA Sign Up/Sign In page. So, I signed in with a current account, then went to the manage page (Prefs) and saw that the old account was still listed...

Comment 10

[aleth [:aleth]]

A faster way to STR this is simply to be signed into sync and then (via "Manage..." in Preferences) to delete the FX account. Then, on restarting Firefox, you get the yellow error bar (as a sync failed) and when you open Preferences, the old sync email is still listed.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to James Bonacci [:jbonacci] from comment #9)
> Ah, so in the original issue, I was able to click on the Sync icon

Which Sync icon? When you signed in did the URL bar have "about:accounts", or "https://accounts.firefox.com"? I would expect this behavior if it was the latter, but I think it shouldn't be possible to sign in again on about:accounts if the prefs are showing your old account (we should hit the "if (user)" case at http://hg.mozilla.org/mozilla-central/annotate/e4843f4f08a7/browser/base/content/aboutaccounts/aboutaccounts.js#l289).

That oddity aside, I don't think we need to track the core issue here (delete account not signing you out) for Firefox 30, since it's a niche case. I'll put this in the Firefox backlog for prioritization along with the rest of the Sync bugs.

Comment 12

[Chris Karlof [:ckarlof]]

FWIW, a user's account could also be deleted if she is in the account creation process, and during the email polling, we detect her email address hard bounces when we try to send her an email. In this case, we delete the user's account on the server, and it would be appropriate for us to remove her account from the browser (and/or possibly message her).

Comment 13

[Chris Karlof [:ckarlof]]

In the same browser case, we can send a "disconnect" message over a WebChannel to remove the user from Sync.

If the user deletes her account in a different browser, can use the /account/status API call (https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#get-v1accountstatus) to detect if the user's account is deleted after we get an authentication error.

Comment 14

[Edouard Oger [:eoger]]

Attachment: bug-999190.patch

Here's what I have right now, it's working great in practice but making all the tests work is a PITA, because all of them are now calling accountStatus() which is supposed to hit the internet (which is not allowed in xpcshell-tests).
I tried putting mocks for accountStatus() everywhere but without any success.
Am I going in the right direction Mark?

Comment 15

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

Comment on attachment 8650746 [details] [diff] [review]
bug-999190.patch

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

::: services/sync/modules/browserid_identity.js
@@ +239,5 @@
>            Svc.Prefs.set("firstSync", "resetClient");
>            Services.obs.notifyObservers(null, "weave:service:setup-complete", null);
>            Weave.Utils.nextTick(Weave.Service.sync, Weave.Service);
>          }
>        }).then(null, err => {

should probably change this to .catch while we are here.

@@ +249,5 @@
> +        this._fxaService.accountStatus().then(exists => {
> +          if (!exists) {
> +            return fxAccounts.signOut(true);
> +          }
> +        }, onFinish)

I think this should probably read something like:

this._fxaService.accountStatus().then(exists => {
 ... signout ...
}.catch(err => {
  log.error("oh noes\n");
}).then(() => {
  this.whenReadyToAuthenticate.reject(err);
});

::: services/sync/tests/unit/test_browserid_identity.js
@@ +90,5 @@
> +
> +//     let fetchTokenForUserCalled = false;
> +//     let accountStatusCalled = false;
> +
> +//     let MockFxAccountsClient = function() {

This looks roughly like the right direction. Note that instead of mocking the FxAccountsClient object it should be possible to add a mock accountState() function directly on the "internal" object - so maybe configureFxAccountIdentity() could take an extra param with is additional mocks to add - or even just directly adding it to the .internal object after calling that function should work.

It's hard to say much more without knowing exactly what is failing for you though.

Comment 16

[Chris Karlof [:ckarlof]]

Comment on attachment 8650746 [details] [diff] [review]
bug-999190.patch

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

::: services/sync/modules/browserid_identity.js
@@ +249,5 @@
> +        this._fxaService.accountStatus().then(exists => {
> +          if (!exists) {
> +            return fxAccounts.signOut(true);
> +          }
> +        }, onFinish)

This is pointed in the right direction.

We probably need this check and reaction consolidated somewhere, because I expect we'll need it in various spots. Anytime we get a auth error from the token server, we probably need to start from scratch, i.e., check to see if the account exists and get a new keypair signed.

Comment 17

[Edouard Oger [:eoger]]

Attachment: bug-999190.patch

Here's what I have so far. I'll wait for the try server results to continue working on that.

Comment 18

[Edouard Oger [:eoger]]

Oh and the xpcshell-tests are finally working

Comment 19

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b55cccc7bee

Comment 20

[Edouard Oger [:eoger]]

Attachment: Part 2: Listen to logout commands from fxa remote server

I think this one is ready for review. It's tricky to test though, because it needs https://github.com/mozilla/fxa-content-server/pull/2994.

Comment 21

[Edouard Oger [:eoger]]

Attachment: Part 1: Detect fxa deletion on sync auth error

Comment 22

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

Comment on attachment 8652063 [details] [diff] [review]
Part 2: Listen to logout commands from fxa remote server

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

It's a real shame about:accounts tests don't support what we need here, but I don't think it is fair we ask for that to be added as part of this patch - so that's fine - but...

The webchannel stuff looks like it will work, but desktop isn't using channels for these commands - the events desktop does use look like they simply don't work and that the functionality hasn't been tested on desktop?

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +278,2 @@
>     */
> +  onLogout: function () {

this looks wrong - the function is called with a param that's not passed.

@@ +281,3 @@
>  
> +    return fxAccounts.getSignedInUser().then(userData => {
> +      if (userData.uid === uid) {

|uid| doesn't exist - I assume this should reference the uid attribute of the param that's missing?

This implies that this code hasn't been tested?

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ -758,5 @@
>      });
>    });
>  });
>  
> -add_test(function test_sign_out() {

I don't think these changes should be made - we are testing a function called |signOut| (but yeah, the fact the function is called signOut while the observer is "logout" sucks) - so let's not touch this file.

Comment 23

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

Comment on attachment 8652064 [details] [diff] [review]
Part 1: Detect fxa deletion on sync auth error

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

See comment 15 where I previously gave some feedback.

Comment 24

[Edouard Oger [:eoger]]

Attachment: Part 2: Listen to logout commands from fxa remote server

Removed the about:accounts commands, since we only pass the delete/logout messages via WebChannel.

Comment 25

[Edouard Oger [:eoger]]

Attachment: Part 1: Detect fxa deletion on sync auth error

Comments addressed. I'm not sure where to add the other "status" calls, /info/collections is hit without any issue with the delete account on manual sync.

Comment 26

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

Comment on attachment 8652617 [details] [diff] [review]
Part 1: Detect fxa deletion on sync auth error

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

This looks good to me, but I'm worried how Sync will react when we do this as part of a node reassignment (ie, when we get a 401 mid-sync rather than at login time). What's going to happen here is that we find Sync is reset *during* a Sync. Eg, one issue may be that by the time https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/stages/enginesync.js#58 is hit, Sync has been reset - but it then changes this.service.status.sync, so it no longer *looks* reset. This is complicated by the fact that the account check is done in the background (so the timing may vary).

On the other hand though, we already have this issue if a user explicitly signs out during a sync, so I guess this is no different - ie, if there is an issue with this code the same issue will exist in that explicit signout case, so mitigation would need to take both scenarios into account.

So I'm a little nervous, but let's go for it :)

::: services/sync/modules/browserid_identity.js
@@ +244,1 @@
>          this._shouldHaveSyncKeyBundle = true; // but we probably don't have one...

We should move this down to where whenReadyToAuthenticate is rejected (they are basically a pair)

@@ +251,5 @@
> +          }
> +        }).catch(err => {
> +          this._log.error("Error while trying to determine FXA existence", err);
> +        }).then(() => {
> +          this.whenReadyToAuthenticate.reject(err)

It's important that the |err| we are rejecting with here is the original error from line 243 - while it is correct, I'm worried it isn't obvious and later someone may accidentally change or be confused. So I think we should add a new variable at line 244, eg, "let authRejectionReason = err;" with a comment like "note that we must reject the authentication promise with this original error, not a subsequent error we may see checking the account status", and do the final reject with authRejectionReason instead of err.

@@ +257,3 @@
>        });
>        // and we are done - the fetch continues on in the background...
>      }).then(null, err => {

let's change this to .catch while we are here.

Comment 27

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

Comment on attachment 8652517 [details] [diff] [review]
Part 2: Listen to logout commands from fxa remote server

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

Don't we still want the about:accounts change here?

Comment 28

[Edouard Oger [:eoger]]

Attachment: Part 1: Detect fxa deletion on sync auth error

Comment addressed on part 1.
About the part 2, I'm not sure we need the about:accounts parts, because we on the fxa-content-server side we are using webchannels and not dom custom events.

Comment 29

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

(In reply to Edouard Oger [:eoger] from comment #28)
> About the part 2, I'm not sure we need the about:accounts parts, because we
> on the fxa-content-server side we are using webchannels and not dom custom
> events.

Shane, can you please clarify here. I've been under the impression that about:accounts for desktop doesn't use web channels at all - is that still true, and true for even the new commands being added here?

Comment 30

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

oops - please see comment 29

Comment 31

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #29)
> (In reply to Edouard Oger [:eoger] from comment #28)
> > About the part 2, I'm not sure we need the about:accounts parts, because we
> > on the fxa-content-server side we are using webchannels and not dom custom
> > events.
> 
> Shane, can you please clarify here. I've been under the impression that
> about:accounts for desktop doesn't use web channels at all - is that still
> true, and true for even the new commands being added here?


:markh is correct, WebChannels are not used for about:accounts, about:accounts still uses custom DOM events. It's pretty easy to migrate to WebChannels, but the work has not been done yet.

Comment 32

[Edouard Oger [:eoger]]

Sorry I wasn't very clear in my comment Mark.
We use at least 2 ways to communicate with the browser from fxa-content-server : the (old) custom dom events and WebChannel.
If I remember correctly the WebChannel currently only handles profile-changes (that's what we use for avatars).
I implemented the "delete" command with WebChannel on the fxa-content-server side.

Comment 33

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

(In reply to Edouard Oger [:eoger] from comment #32)
> Sorry I wasn't very clear in my comment Mark.

No problem - I'm still a little confused though :) Why do we need part 2 then?

I thought that along with your "on 401 check the account status" part, there was also going to be a part that *immediately* detected the account deletion so the device doing the deletion didn't need to wait for the tokens to expire before it knows the account is deleted, and that is what the channel/event changes were for.

Comment 34

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

Comment on attachment 8652517 [details] [diff] [review]
Part 2: Listen to logout commands from fxa remote server

Explained to me in a meeting!

Comment 35

[Edouard Oger [:eoger]]

Let's try this before landing.

Comment 36

[Edouard Oger [:eoger]]

Attachment: Part 2: Listen to logout commands from fxa remote server

Comment 37

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6daf0c4debc9

Comment 38

[Edouard Oger [:eoger]]

Attachment: Part 1: Detect fxa deletion on sync auth error

We found a way to make the migration test pass. Thanks Mark!
Let's hope this try push works :)

Comment 39

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77a40dab1a8

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9f7a5f4f8d1f
https://hg.mozilla.org/integration/fx-team/rev/01a4752969ce

Comment 41

[Paul Silaghi, QA [:pauly]]

43.0a1 (2015-09-06) Win 7
Deleted account information still persists in about:preferences#sync and menu panel, before the browser is restarted. Thoughts?

Comment 42

[Edouard Oger [:eoger]]

We have to find a way to detect it on sync.

Comment 43

[Paul Silaghi, QA [:pauly]]

(In reply to Edouard Oger [:eoger] from comment #42)
> We have to find a way to detect it on sync.
So, I guess the behavior from comment 41 is expected for now?

Comment 44

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

I think we'll just need to handle the ONLOGOUT notification in the Sync prefs pane, browser-syncui and browser-fxaccounts. I'll open a new bug.

Comment 45

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

Comment 41 is expected for now and does verify the main part of this bug is fixed. The verification (or possibly fix) of the second part can be tracked in bug 1202510.

Comment 46

[Paul Silaghi, QA [:pauly]]

Thanks Mark.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f7a5f4f8d1f
https://hg.mozilla.org/mozilla-central/rev/01a4752969ce