Closed Bug 1383663 Opened 7 years ago Closed 7 years ago

Detect when account email has changed, and update local state on Desktop to match

Categories

(Firefox :: Firefox Accounts, enhancement, P1)

Product:
Firefox
Component:
Firefox Accounts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: rfkelly, Assigned: eoger)

References

Details

(Whiteboard: [fxa-waffle])

Attachments

(3 files)

Bug 1383663 part 1 - Revert bug 1385191 changes.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1383663 part 3 - Update FxA local state on profile email change.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1383663 part 2 - Remove fx-desktop-v1 client capabilities from aboutaccounts.js.
59 bytes, text/x-review-board-request
markh
: review+
rfkelly
: review+
Details 
FxA will soon be shipping the ability for the user to change their primary email address.  Desktop Firefox may need to update its local login state when this happens, e.g. to ensure it displays the correct profile information to the.  This bug is for us to:

* Check for what information Firefox will need to update, if any
* Agree on how it will detect this change event
* Implement the necessary listeners etc to respond to the change.

Filing as a tracking bug for Q3 planning purposes.
Whiteboard: [fxa-waffle]
Summary: Detect when account email has changed, and update local state to match → Detect when account email has changed, and update local state on Desktop to match
Priority: -- → P1
Assignee: nobody → eoger
Ryan, how do you see this working?

* I imagine we can probably expect a push, but best I can tell, there's no existing suitable push message. We could possibly piggy-back the "empty" notification (currently that just hits /recovery_email/status, but we could also check some additional end-point), but we might want to deprecate the empty notification. There is the "fxaccounts:profile_updated" message, which doesn't seem quite right, but we certainly could argue the email address *is* owned by the profile?

* Similarly, we probably want to assume push didn't arrive, so we'd want to hit some end-point just to check. I can't see a suitable endpoint we already hit, except, again, checking the profile.

So my specific questions: what push message will be sent when this happens, and do you think it makes sense to use the existing profile endpoints and mechanics for this? (The downside of using the profile mechanism is that it kinda locks us out in the future should other account data change that isn't already in the profile, but that seems unlikely?)
Flags: needinfo?(rfkelly)
+Vijay, who I forgot to CC onto this for some reason.

> I imagine we can probably expect a push [...]
> There is the "fxaccounts:profile_updated" message, which doesn't seem quite right,
> but we certainly could argue the email address *is* owned by the profile?

This feels like more-or-less the right thing to me, treating your email is part of your profile information.  But I'm open to suggestions that it's not a good fit.  Vijay, thoughts?

> we'd want to hit some end-point just to check. I can't see a suitable endpoint we already hit,
> except, again, checking the profile

Do you already do this to detect changes in e.g. display name?  If so, it seems like the right place for it.

This raises a related question - we need to confirm that the profile endpoint correctly reflects the updated email address after an email change.

Does desktop do anything special with the email address that's different to how it handles the display-name, e.g. use it as a primary key in any of its data stores?  I recall some sort of "lastSignedInUserHash" feature but I don't recall if that used the email address or display name.
Flags: needinfo?(vbudhram)
Flags: needinfo?(rfkelly)
Flags: needinfo?(markh)
(In reply to Ryan Kelly [:rfkelly] from comment #2)
> Do you already do this to detect changes in e.g. display name?  If so, it
> seems like the right place for it.

Yep - although we'll probably still need to do a little more work here, such as changing the email address in the "signedInUser" blob (which is stored separately from the profile)

> Does desktop do anything special with the email address that's different to
> how it handles the display-name, e.g. use it as a primary key in any of its
> data stores?  I recall some sort of "lastSignedInUserHash" feature but I
> don't recall if that used the email address or display name.

Ah, yes, good memory :) This does use the email address, so we'd need to fix that too (both in aboutaccounts and FxAWebChannel). We also store the username in sync's prefs, which is an email address, and while I don't think it's used other than as a "flag", we should probably ensure it's accurate too. I can't think of any other places this is relevant.
Flags: needinfo?(markh)
> I imagine we can probably expect a push [...]
> There is the "fxaccounts:profile_updated" message, which doesn't seem quite right,
> but we certainly could argue the email address *is* owned by the profile?

:eoger mentioned something similar and it does make sense to me. However, I am not sure if we need to introduce a new message or if we can piggyback off "fxaccounts:profile_updated".

> This raises a related question - we need to confirm that the profile 
> endpoint correctly reflects the updated email address after an email change.

I can confirm that the profile server does return the correct primary email address after it has been changed. When a client receives a "fxaccounts:profile_updated" message, does it call the `/profile` to get the updated data? Would we be able to update the email address from this?
Flags: needinfo?(vbudhram)
> When a client receives a "fxaccounts:profile_updated" message, does it call the `/profile` to get the updated data

Not immediately: http://searchfox.org/mozilla-central/source/services/fxaccounts/FxAccountsProfile.jsm#57

I think we should implement a different message type, because we're not taking the same code path anyway and the semantics would be better, but I don't really care that much :)
(In reply to Edouard Oger [:eoger] from comment #5)
> > When a client receives a "fxaccounts:profile_updated" message, does it call the `/profile` to get the updated data
> 
> Not immediately:
> http://searchfox.org/mozilla-central/source/services/fxaccounts/
> FxAccountsProfile.jsm#57

IIUC though, that should cause /profile to be hit the next time we want to show this data (because otherwise the display name would remain wrong, which would be just as bad from the user's POV as the address being wrong)

> I think we should implement a different message type, because we're not
> taking the same code path anyway and the semantics would be better, but I
> don't really care that much :)

I don't really care that much either, but I'm reluctant to have everywhere that already listens for the profile change (ie, the prefs page and UIState.jsm) to have another observer listener for what is conceptually the same. So if we did have another message, I'd be inclined to implement it as (a) change the address in the signedInUser json blob, then (b) send the same "profile change" notification (ie, a new push message is fine, but a new observer the browser needs to listen for to update its UI seems wrong)
Strategically/conceptually, I think we should aim to treat the email address as just another piece of profile data we have about the user.  It's not that way currently because we've been using it as a special account identifier, but the more we can move away from that the better.  IMHO that argues for trying to re-use the "profile changed" notification and update mechanism unless the code makes that difficult.

But like everyone else in the thread, I'm not exactly willing to die on that particular hill :-)

> I think we should implement a different message type, because [...] the semantics would be better

:eoger, could you please say more about what you have in mind here?

> > I recall some sort of "lastSignedInUserHash" feature but I don't recall if that used the email address 
> Ah, yes, good memory :) This does use the email address

Mark, would it make sense for us to migrate this to using the uid, since "email address as account identifier" is a pattern we're trying to move away from more generally?
Flags: needinfo?(markh)
Flags: needinfo?(eoger)
If we're moving away from "email as a key identifier" to "just another piece of the profile data", I'm fine with re-using the profile message/handling.
Flags: needinfo?(eoger)
+:grisha for context, since this might affect what we end up doing on Android as well.
(In reply to Ryan Kelly [:rfkelly] from comment #7)
> Mark, would it make sense for us to migrate this to using the uid, since
> "email address as account identifier" is a pattern we're trying to move away
> from more generally?

Absolutely - we could probably also avoid hashing the value like to we now to the email address, making this whole thing much simpler.
Flags: needinfo?(markh)
If we enable the feature as opt-in for users before this bug lands, we will almost certainly end up in the situation that the "email" stored in the account blob we have will be different from the profile blob we have. Given this will happen anyway, I wonder if we could leverage it in the shorter term? Basically, that we'd prefer the email address from the profile.

Everywhere the address is shown now, the basic pattern we use is:

fxa.getSignedInUser().then(user => {
  // user.email is what we use.
  fxa.getSignedInUserProfile(profile => {
    // use display name if we have it (but we ignore profile.email)
  }
}

What might be possible is that we update the email we use from getSignedInUserProfile. The end result is likely to be that for a very brief period, the *original* email will be shown, but this would then be updated with the email from the profile (which is roughly what happens today for a user with a display name - we very briefly show just the email, then update it with the name when the profile arrives). I expect this would be a fairly small patch in only 3 places - sync.js from the 2 preferences impls, and UIState.jsm - in all cases, probably a single line change.

Does that sound too hacky to do in the meantime? Ed, what do you think?
As a temporary fix this is okay with me. We'll also have to make sure we don't break anything using services.sync.username.
See Also: → 1385191
Assignee: eoger → nobody
Tested this with the latest nightly and everything worked as expected. The new primary email address was reflected almost instantly on Desktop.
Priority: P1 → P2
With the short-term fix from Bug 1385191 landed, what behaviour changes remain to be tackled in this bug?
Flags: needinfo?(markh)
(In reply to Ryan Kelly [:rfkelly] from comment #14)
> With the short-term fix from Bug 1385191 landed, what behaviour changes
> remain to be tackled in this bug?

Off the top of my head:

* Change the code which uses the identity.fxaccounts.lastSignedInUserHash preference to use a UID instead of the email address.

* Arrange for the account info itself to have the new email address instead of relying on the profile having the correct address - currently desktop will briefly show the "old" email address before updating to the one from the profile.

* For completeness, update sync's "account name" preference to the new address (although this probably isn't strictly necessary as that pref is currently used more as a flag that someone is logged in - the actual value doesn't really matter)
Flags: needinfo?(markh)
Assignee: nobody → eoger
Priority: P2 → P1
Comment on attachment 8900361 [details]
Bug 1383663 part 1 - Revert bug 1385191 changes.

https://reviewboard.mozilla.org/r/171726/#review177218
Attachment #8900361 - Flags: review?(markh) → review+
Comment on attachment 8900362 [details]
Bug 1383663 part 3 - Update FxA local state on profile email change.

https://reviewboard.mozilla.org/r/171728/#review177222

Looks great except for the comment below re email vs uid.

::: browser/base/content/aboutaccounts/aboutaccounts.js:35
(Diff revision 1)
>    // dump("FXA: " + msg + "\n");
>  }
>  
>  function getPreviousAccountNameHash() {
>    try {
> -    return Services.prefs.getStringPref(PREF_LAST_FXA_USER);
> +    return Services.prefs.getStringPref(fxAccountsCommon.PREF_LAST_FXA_USER);

I see that you are updating this has when we notice a change, but I'm not sure that's enough - eg, consider a user who disconnects, then some time later changes their email, then some time later reconnects. It's a bit of a pain, but I think it might be worth deprecating the old pref name and using a new pref with the UID (and I can't see why it would be necessary to hash it). This makes the check more complex (as we'll need to check both prefs) but I think it's probably worthwhile given users may be quite concerned if they see this message with a false-positive given we don't show them what the previous email was (as we don't know)

OTOH, doing that would mean a user deleting their account then re-creating it with the same email means they would be warned whereas they aren't currently - although this feels like more of an edge-case than the above. If you feel strongly about it and can convince a few others that sticking with email is the best thing, I'm happy to change my mind.
Attachment #8900362 - Flags: review?(markh)
> might be worth deprecating the old pref name and using a new pref with the UID

To make this work, I think we'd also need to change fxa-content-server to include the uid in the "can_link_account" webchannel message, right?

> (and I can't see why it would be necessary to hash it)

Agreed.

> doing that would mean a user deleting their account then re-creating it with
> the same email means they would be warned whereas they aren't currently

That seems OK as an edge-case to me.  Alternatively, we could check both email and uid and proceed without a warning if either matches.
(In reply to Ryan Kelly [:rfkelly] from comment #20)
> To make this work, I think we'd also need to change fxa-content-server to
> include the uid in the "can_link_account" webchannel message, right?

Ouch, yes, it would.
> Ouch, yes, it would.

I don't see a problem with doing that if it's the Right Thing, just wanted to call it out explicitly :-)
Opened a PR server-side: https://github.com/mozilla/fxa-content-server/pull/5386.
Will update the Desktop patch next.
Comment on attachment 8900362 [details]
Bug 1383663 part 3 - Update FxA local state on profile email change.

https://reviewboard.mozilla.org/r/171728/#review180252

::: services/fxaccounts/FxAccountsWebChannel.jsm:494
(Diff revision 2)
>     *
>     * @private
>     */
> -  _needRelinkWarning(acctName) {
> -    let prevAcctHash = this.getPreviousAccountNameHashPref();
> -    return prevAcctHash && prevAcctHash != this.sha256(acctName);
> +  _needRelinkWarning(acctName, acctUid) {
> +    let prevAcctHashes = this.getPreviousAccountHashes();
> +    return prevAcctHashes && prevAcctHashes.name != CryptoUtils.sha256Base64(acctName)

I think we should add a comment here to say why we are checking both (ie, a brief note about the fact that deleting and recreating the account will have a different UID but same email).

Also, is this going to do the right thing for users without the UID pref already set (ie, existing users who haven't changed their email address)?

And TBH, I think I'd probably prefer to just keep the UID and treat the email pref as deprecated, and live with that edge-case of a user deleting then recreating their account just to keep the complexity down, but I can live with this if you feel strongly about it.

::: services/sync/modules/browserid_identity.js:308
(Diff revision 2)
>        this.resetCredentials();
>        this._ensureValidToken().catch(err =>
>          this._log.error("Error while fetching a new token", err));
>        break;
> +
> +    case fxAccountsCommon.EMAIL_CHANGED:

ISTM there's a significant risk that this isn't going to see all such changes. eg, IIUC, we are likely to try and refresh the profile within a few seconds of browser startup and before this module is loaded.

Further, initializeWithCurrentIdentity seems to unconditionally set this anyway, so I think we can probably avoid this completely, and the worst that will happen is that username pref might be wrong until the browser restarts, which should be fine given we don't actually care what the value is?
Attachment #8900362 - Flags: review?(markh)
Comment on attachment 8902860 [details]
Bug 1383663 part 2 - Remove fx-desktop-v1 client capabilities from aboutaccounts.js.

https://reviewboard.mozilla.org/r/174556/#review180254

This is more a rubber-stamp :) I see you've requested review from Ryan, so if he and/or Shane are happy with this change then it SGTM.
Attachment #8902860 - Flags: review?(markh) → review+
Comment on attachment 8902860 [details]
Bug 1383663 part 2 - Remove fx-desktop-v1 client capabilities from aboutaccounts.js.

I am a big +1 to removing this dead code, but I want to double-check with Shane that we're not expecting to use it in any corner-cases.
Attachment #8902860 - Flags: review?(stomlinson)
Attachment #8902860 - Flags: review?(rfkelly)
Attachment #8902860 - Flags: review+
Patch updated, thank you both.
Comment on attachment 8902860 [details]
Bug 1383663 part 2 - Remove fx-desktop-v1 client capabilities from aboutaccounts.js.

https://reviewboard.mozilla.org/r/174556/#review181622

LGTM!
Attachment #8902860 - Flags: review?(rfkelly) → review+
Comment on attachment 8900362 [details]
Bug 1383663 part 3 - Update FxA local state on profile email change.

https://reviewboard.mozilla.org/r/171728/#review182198

This looks great, but I don't think we can completely ignore the old pref we used - most users will not change their email address, so IIUC those existing users would therefore lose the relink warning completely?

::: services/fxaccounts/FxAccountsWebChannel.jsm:462
(Diff revision 3)
> -
> -  /**
> -   * Given a string, returns the SHA265 hash in base64
>     */
> -  sha256(str) {
> -    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> +  setPreviousAccountHash(uid) {
> +    Services.prefs.setStringPref(PREF_LAST_FXA_USER_UID, CryptoUtils.sha256Base64(uid));

I think we should reset the (now old, deprecated) pref here.

::: services/fxaccounts/FxAccountsWebChannel.jsm:489
(Diff revision 3)
>     * if they want to continue.
>     *
>     * @private
>     */
> -  _needRelinkWarning(acctName) {
> -    let prevAcctHash = this.getPreviousAccountNameHashPref();
> +  _needRelinkWarning(acctUid) {
> +    let prevAcctHash = this.getPreviousAccountHash();

I think we still want to check the (now old, deprecated) preference if it exists.
Attachment #8900362 - Flags: review?(markh)
Updated thanks, we should file a follow-up bug to remove that old pref check in 3 or 4 releases from now.
Comment on attachment 8900362 [details]
Bug 1383663 part 3 - Update FxA local state on profile email change.

https://reviewboard.mozilla.org/r/171728/#review184110

Thanks!
Attachment #8900362 - Flags: review?(markh) → review+
Comment on attachment 8900362 [details]
Bug 1383663 part 3 - Update FxA local state on profile email change.

Mark realized that the content-server doesn't send the account's uid. It is because the webchannel request happens before we create/login to the account.
Let's push this patch so it lands before the 57 merge. We'll have to live with the edge case of a user disconnecting, changing the primary email then reconnecting.
Attachment #8900362 - Flags: review+ → review?(markh)
Comment on attachment 8900362 [details]
Bug 1383663 part 3 - Update FxA local state on profile email change.

LGTM, thanks.
Attachment #8900362 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7d46e2e8ddc
part 1 - Revert bug 1385191 changes. r=markh
https://hg.mozilla.org/integration/autoland/rev/9d26a627e2f8
part 2 - Remove fx-desktop-v1 client capabilities from aboutaccounts.js. r=markh,rfkelly
https://hg.mozilla.org/integration/autoland/rev/f384a524cac6
part 3 - Update FxA local state on profile email change. r=markh
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7b8c72dc868
part 1 - Revert bug 1385191 changes. r=markh
https://hg.mozilla.org/integration/autoland/rev/81836819468a
part 2 - Remove fx-desktop-v1 client capabilities from aboutaccounts.js. r=markh,rfkelly
https://hg.mozilla.org/integration/autoland/rev/1ea2cdca150e
part 3 - Update FxA local state on profile email change. r=markh
Product: Core → Firefox
Target Milestone: mozilla57 → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: