958927 - Display messaging when a user logs out or logs in to a different Firefox Account

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

If a user logs into one Fxa account, configures Sync, performs a Sync, then logs into a different Fxa account, we probably need to take some special action.  For example, we possibly want to revert back to the state where this is the very first Fxa login - eg, reset all sync preferences to default and act as if this is the very first sync for this device.

Unclear what other considerations might apply, but this bug is to determine what they are and how we expose this in the UI.

Comment 1

[Richard Newman [:rnewman]]

Speaking to your concrete example: you very likely want to preserve the "Unlink this device" logic -- when you stop using a Firefox Account, do all the same timestamp clearing etc. behavior that Sync did on "Unlink this device". If they set up the same FxA again, great -- it'll just do a merge sync. If they set up a new FxA, you're ready to roll.

But wrt the broader point: this has been an under-designed area of original Sync, too. And I've seen pained user reports of data merging in Chrome.

The issue is that when a user starts thinking in terms of "signing in", rather than "setting up Sync", their expectations about data going away and coming back change. E.g.,

* Should you wipe this profile when you sign out? When you sign in?
* If not, should you merge the contents, or prompt differently if this is the second account you sign in to? Failure mode: my passwords end up in my friend's password manager.
* What happens if a first sync isn't finished when I sign out? Is that a problem? (With the Sync codebase it certainly is from a protocol standpoint, but it's a different kind of problem if you're about to wipe the profile!)

In Sync we avoided this by not strongly phrasing things in terms of signing in to an account -- e.g., "unlinking" rather than "signing out".

The solution in terms of Accounts is probably SITB with profile switching (Bug 749195).

Comment 2

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

We decided to display 2 warnings:

* On logout, we display a message indicating that although they have signed out, their data remains on this computer but will no longer be synced.

* On login, if we determine they previously were logged into a different account, we display a message indicating that the data from this new account will be merged into this account, and an option to cancel the login process.

We are yet to determine the exact wording and exactly how to display the message (eg, panel, modal dialog, something else?)

Comment 3

[Richard Newman [:rnewman]]

Flagging needinfo on John for wording and presentation.

Comment 4

[James Bonacci [:jbonacci]]

:rnewman
:nalexander
Do we have/need an Android version of this bug?

Comment 5

[Richard Newman [:rnewman]]

(In reply to James Bonacci [:jbonacci] from comment #4)
> :rnewman
> :nalexander
> Do we have/need an Android version of this bug?

Depends.

There's not much we can do for raw Android accounts. If we allow you to delete the account from inside its own management UI, then we'll probably include something like this.

Nick, do you have an opinion here?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #5)
> (In reply to James Bonacci [:jbonacci] from comment #4)
> > :rnewman
> > :nalexander
> > Do we have/need an Android version of this bug?
> 
> Depends.
> 
> There's not much we can do for raw Android accounts. If we allow you to
> delete the account from inside its own management UI, then we'll probably
> include something like this.

Well, we believe that the "about to be deleted" hook works for Sync Android accounts, so we could do more with that: try to clean up your remote account, try to clean up your local DBs, surface a notification saying whatever desktop says, present modal UI alerting you to what's happening, trigger Fennec to do something next time it starts, etc.
 
> Nick, do you have an opinion here?

Yes: sign in to browser is a strange paradigm :)  I'd lean towards a notification until we get some experience.  We'd want to make sure you didn't get notifications for package events, though -- removals, upgrades, etc.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #6)

> Yes: sign in to browser is a strange paradigm :)  I'd lean towards a
> notification until we get some experience.  We'd want to make sure you
> didn't get notifications for package events, though -- removals, upgrades,
> etc.

I think our deletion on SD removal will be unavoidably silent, and we're in control of re-adding, so I expect we'll be OK here.

I'll file a reminder bug in parallel to this one.

Comment 8

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

Attachment: 0010-Bug-958927-WIP-only-a-stub-display-message-on-logout.patch

This is a WIP - the main point is to show *where* the hooks for the messaging will be implemented.

Chris: I think the "logout" warning is fairly non-controversial - it's in the "Sync Options" code, and IIUC, this is the only place the signout UI will be exposed.

The "login" warning might be slightly more controversial - this patch has it down in the observer notification in browserid_identity.  This means that the first login for a session will never trigger this warning - but I can't think of how this would be a problem in practice.  Doing it as part of the initWithLoggedInUser() call is problematic, as the promise is "stalled" while this modal is up, and thus the sync options panel doesn't work as expected.

The only other alternative I can see is to do it in about:accounts itself, but that seems a little ugly as about:accounts currently has no sync-specific code.  I welcome your thoughts on this, and/or other alternatives I may have missed.

Comment 9

[Chris Karlof [:ckarlof]]

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

> The only other alternative I can see is to do it in about:accounts itself,
> but that seems a little ugly as about:accounts currently has no
> sync-specific code.  I welcome your thoughts on this, and/or other
> alternatives I may have missed.

FxAccounts.jsm could remember if someone has logged in previously (in signedInUser.json), and if someone has that's different from the current user, setSignedInUser() could display this warning before doing anything else. If the user cancels, then setSignedInUser() could do nothing.

Comment 10

[Chris Karlof [:ckarlof]]

Comment on attachment 8362789 [details] [diff] [review]
0010-Bug-958927-WIP-only-a-stub-display-message-on-logout.patch

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

::: browser/components/preferences/sync.js
@@ +251,5 @@
> +    if (confirm &&
> +        !Services.prompt.confirm(window, "Scary Logout Title",
> +                                 "Scary Logout Warning?")) {
> +      return;
> +    }

This looks fine, except it needs some final copy. :)

::: services/sync/modules/browserid_identity.js
@@ +124,5 @@
> +        if (!ok) {
> +          fxAccounts.signOut();
> +          return;
> +        }
> +      }

see my reply in the comments about an alternative approach.

Comment 11

[Chris Karlof [:ckarlof]]

Specifically, we could add previousUid: <uid> to the signedInUser.json object.

Comment 12

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

Attachment: 0009-Bug-958927-part-1-XUL-dialogs-for-sync-unlink-and-re.patch

Implement XUL warning dialogs.  Sadly the dialogs aren't quite the correct size as the description label wraps and the buttons get cropped by the dialog borders.  That might be OK in the short term though as long as the strings etc are ok.

Comment 13

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

Attachment: 0010-Bug-958927-part-2-display-warning-on-logout-and-logi.patch

Zack: this *should* work, but when about:accounts "rejects" the logout by returning early, the hosted content continues to re-send the notification every second or so - so if you cancel the dialog it keeps coming back once per second.  If you "continue" it seems to work fine.  It would be great if you could look into this for me.

ttaubert: general feedback welcome :)

Comment 14

[Chris Karlof [:ckarlof]]

Comment on attachment 8365055 [details] [diff] [review]
0010-Bug-958927-part-2-display-warning-on-logout-and-logi.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +104,3 @@
>      fxAccounts.setSignedInUser(accountData).then(
>        () => {
>          this.injectData("message", { status: "login" });

markh, here's the magic you're missing when the user cancels out. This is an acknowledgement to the hosted code that the browser received the message and processed it correctly. Otherwise it retries. This ack has dubious value (maybe only debugging value), so we might consider removing these acks and the retry logic in the hosted code.

Comment 15

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

Attachment: 0008-Bug-958927-part-2-display-warning-on-logout-and-logi.patch

Fixes the "login cycle" in the previous patch based on advice from Chris.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8365054 [details] [diff] [review]
0009-Bug-958927-part-1-XUL-dialogs-for-sync-unlink-and-re.patch

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

If we don't need any fancy dialogs we could also use nsIPromptService.confirm(). If we want to stay a little more flexible we can keep this though.

::: browser/base/content/sync/relink.xul
@@ +18,5 @@
> +<dialog id="fxaRelinkWarning"
> +        title="&relink.verify.title;"
> +        buttons="accept, cancel"
> +        buttonlabelaccept="&continue.label;"
> +        ondialogaccept="window.arguments[0].ok = true;"

Nit: I'd like |.accepted = true| a little more.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8365425 [details] [diff] [review]
0008-Bug-958927-part-2-display-warning-on-logout-and-logi.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +12,5 @@
>  // importing the constants directly into this scope causes symbol conflicts
>  let constants = {};
>  Cu.import("resource://gre/modules/FxAccountsCommon.js", constants);
>  
> +const PREF_LAST_FXA_USER = "services.sync.fxaccounts.lastaccount";

Maybe... identity.fxaccounts.lastSignedInUser or something? That shouldn't be tied to sync even though that's the reason we're doing it...

@@ +86,5 @@
> +    // only fxa consumer, so...
> +    let needWarning;
> +    try {
> +      needWarning = Services.prefs.getCharPref(PREF_LAST_FXA_USER) != accountData.email;
> +    } catch (_) {

Would be good to put this code in a function with a nice name? We could do it like so then:

let needWarning = this.getLastUsername() != accountData.email;

Comment 18

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

Attachment: 0005-Bug-958927-part-1-XUL-dialogs-for-sync-unlink-and-re.patch

Gavin requested we just use the prompt service for the first iteration

Comment 19

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

Comment on attachment 8367110 [details] [diff] [review]
0005-Bug-958927-part-1-XUL-dialogs-for-sync-unlink-and-re.patch

This should use getComplexValue/setComplexValue with nsISupportsString to support non-ASCII usernames, but otherwise looks good.

Comment 20

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

(I also want to nitpick the strings, but we can do that separately in a followup.)

Comment 21

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

https://hg.mozilla.org/integration/fx-team/rev/288310faa1ff

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/288310faa1ff

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 29.0b8
Build Id: 20140414143035
OS: Win 7 x64, Mac Os x 10.9.2, Ubuntu 13.04 x 64

Warning messages are displayed when you disconnect from the account and when you are logging to a different Firefox sync account.