1368147 - Detect when account email has changed, and update local state on Android to match

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Firefox Accounts, enhancement, P1)

Description

[Vijay Budhram]

FxA is in the process of designing and prototyping the ability to change a user's primary email account. This bug is to help track any of the integration issues from this feature with respect to Firefox Android.

The current implementation goal is to move away (as much as possible) from using the email on the account table and start using email on the emails table. When a user `changes` their primary email, that email is set as the primary email in the emails table and will be used over the email on the account table. The email address on the account table needs to still exist because it is used to compute the password hash to login.

It was mentioned that Android FxA implementation uses an email that is assumed constant. It could be possible to update their session information with the new email, or even invalidate the session and require a login with new email?

What are some possible issues that could arise from this feature?

Comment 1

[Ryan Kelly [:rfkelly]]

ni? Nick for comment.

There's also Bug 1173566 which may be a dupe but doesn't have a lot more info.

Comment 2

[Nick Alexander :nalexander [he/him]]

The Android Account Manager identifies accounts by a tuple `(name, type)`, where `name` is an arbitrary string and `type` is a per-Android App fixed value.  The Android system user interface displays the `name` string in several places throughout the UI.  It's standard to identify accounts by email address (so `name` is the email address), and we do so for Firefox Accounts on Android.  (We identified Legacy Sync accounts by email address as well, even though there was no strong connection between a Legacy Sync account and the "recovery email" provided.)

Android version 21 has added a `renameAccount` method [1] for precisely this purpose, but we support version 15+.  We could probably ape the functionality on pre-21 Android devices, especially since we use only one or two small parts of the Account API -- basically just `{get,set}UserData`.  So, if we could reliably determine that the primary email address has changed remotely, we could probably make the Android system react correctly (in most cases: the edges will be sharp -- I would expect at least some deleted-and-never-re-created accounts in the wild).  This gets more difficult in the face of disconnected clients that don't witness the primary email address changing, potentially such that their registered email address is no longer associated with the account.

What would need to happen is we'd need to audit all of the Firefox Account interactions and ensure that they're using the allocated UID to identify the account (to the service, internally, etc) and not the transient identifier (primary email address).  I doubt that it's _too_ much work, but it is some work and it is non-trivial risk.  We'd also need to figure out what happens when the user signs in using a secondary email address: presumably the system normalizes to the primary email address, just like the system normalizes capitalization.

[1] https://developer.android.com/reference/android/accounts/AccountManager.html#renameAccount(android.accounts.Account,%20java.lang.String,%20android.accounts.AccountManagerCallback%3Candroid.accounts.Account%3E,%20android.os.Handler)

Comment 3

[Ryan Kelly [:rfkelly]]

> We'd also need to figure out what happens when the user signs in using a secondary email address:
> presumably the system normalizes to the primary email address, just like the system normalizes capitalization.

Yes, (if and) when we allow logging in with a secondary email address, we will normalize it to the primary before messaging it out to the browser.

I'm hearing a hierarchy of downside risks here:

1) The client is using an API that identifies the account by email address.  In this case, the user risks being booted out of sync, or perhaps left with sync in a broken state and needing them to sign out and sign back in.

2) The client can't or doesn't update the Android account model with the new email address.  It sounds like the impact is a confusing UX that continues displaying their old email address.  Would the user's sync setup continue to work in this case?

I think we can live with (2) on older devices if we have to, but we'll have to think carefully about avoiding (1).  So it sounds like this:

> What would need to happen is we'd need to audit all of the Firefox Account interactions and ensure that
> they're using the allocated UID to identify the account (to the service, internally, etc) and not the transient identifier

is the clear next step, with an eye to assessing the risk of (1).

Thanks Nick!

Comment 5

[Alex Davis [:adavis]]

If it can be of use, here is the break down of Fx users by versions of Android:
https://sql.telemetry.mozilla.org/queries/2834#5302

Unfortunately, I don't think we track the version of Android for FxA users alone. Ultimately, I'm trying to figure out what % of FxA users won't have support for `renameAccount` method.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Alex Davis [:adavis] [PM FxA+Sync] from comment #5)
> If it can be of use, here is the break down of Fx users by versions of
> Android:
> https://sql.telemetry.mozilla.org/queries/2834#5302
> 
> Unfortunately, I don't think we track the version of Android for FxA users
> alone. Ultimately, I'm trying to figure out what % of FxA users won't have
> support for `renameAccount` method.

Don't spend too much time: as I said in https://bugzilla.mozilla.org/show_bug.cgi?id=1368147#c2, I expect we can make this work on pre-21 devices.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan Kelly [:rfkelly] from comment #3)
> > We'd also need to figure out what happens when the user signs in using a secondary email address:
> > presumably the system normalizes to the primary email address, just like the system normalizes capitalization.
> 
> Yes, (if and) when we allow logging in with a secondary email address, we
> will normalize it to the primary before messaging it out to the browser.
> 
> I'm hearing a hierarchy of downside risks here:
> 
> 1) The client is using an API that identifies the account by email address. 
> In this case, the user risks being booted out of sync, or perhaps left with
> sync in a broken state and needing them to sign out and sign back in.
> 
> 2) The client can't or doesn't update the Android account model with the new
> email address.  It sounds like the impact is a confusing UX that continues
> displaying their old email address.  Would the user's sync setup continue to
> work in this case?

If we address (1), then yes, everything should function just fine: to the best of my knowledge, the "name" identifer is opaque to the Android system.

> I think we can live with (2) on older devices if we have to, but we'll have
> to think carefully about avoiding (1).  So it sounds like this:
> 
> > What would need to happen is we'd need to audit all of the Firefox Account interactions and ensure that
> > they're using the allocated UID to identify the account (to the service, internally, etc) and not the transient identifier
> 
> is the clear next step, with an eye to assessing the risk of (1).

Correct.  There aren't a huge number of consumers of the FxA client information, so this shouldn't be too hard.

Comment 8

[Vijay Budhram]

Had a chance to test this out with an Android API 23 client.

https://drive.google.com/a/mozilla.com/file/d/0B7MZ5bFBfvpFUWpFTlhTZzlROWM/view?usp=sharing

From the video, after changing your email, the clients don't seem adversely affected. When the user signs out and back in, the updated email is shown and Sync says it is enabled. I should hopefully get this deployed to a dev box so you would be able to test it out.

Comment 9

[Ryan Kelly [:rfkelly]]

The current proposal is that Firefox can detect that the email address has changed in one of three ways:

1) By receiving a "profile:change" webchannel message, if the change was performed in a tab in the current browser
2) By receiving a "fxaccounts:profile_updated" push message, if registered for push
3) By periodically refreshing its cache of the user's profile data and noticing that the email has changed

This is the same set of ways you might detect change to other profile information like displayName or avatar, and trying to treat the email more like other profile information seems like the right thing to me.

If we have both (1) and (2) above both trigger the refresh process mentioned in (3), then ISTM that we could have a single code-path where we detect and fixup this issue.  But I've no idea whether that works well with the way Android sync is architected with background processes etc.

:grisha, thoughts?

Comment 10

[:Grisha Kruglov]

Before diving into the code, that seems like a reasonable approach. I'll put together a WIP patch to see what's involved in practice.

Comment 11

[:Grisha Kruglov]

Finally starting to write code for this.

So far I've only seen one place where we use email for anything interesting. Namely, "sync prefs" are stored on the filesystem keyed by "username", which happens to be the now-ephemeral "email" value. These prefs contain quite a bit of info - it's a state that's persisted between syncs. Collection state (timestamps, syncIDs, some custom data), backoff information, etc.

Some careful "pre" work is required. Ideally we should be keying these prefs by a canonical per-account value - I assume uid is our best bet. This changeover requires that we delete the old prefs file and copy contents over to a new one with a different name. Changing prefs data from underneath a running sync is foolish - prefs are used extensively by almost every moving part throughout a sync life-cycle.

And so a move of the prefs data needs to happen in a sync-aware fashion (sync might run at any point in the background). A naive approach would be to perform a switchover whenever someone actually requests syncPrefs - thankfully, access to them is centralized. Downside is that we would need to perform a check _every_ time this happens. An upside is that if coupled with some locking it's likely the safest thing to do.

Other than that, we have some internal keyed-by-email caches, but they're not very problematic.

Another obvious hurdle is supporting pre-21 devices. Essentially this means removing an account, and adding another one with the new name while also carrying over account data. I'm yet to see what the side-effects of that are going to be. If docs are to be trusted, the post-21 "renameAccount" API essentially does the same thing, and it appears to "just work", so I'm somewhat optimistic.

Comment 12

[Ryan Kelly [:rfkelly]]

> Ideally we should be keying these prefs by a canonical per-account value - I assume uid is our best bet.

Sounds right to me FWIW.

Comment 13

[:Grisha Kruglov]

Another smallish hurdle: we need to de-bounce "profile update" events.

When a profile change happens (originating from local device, or from any other device), we receive an FxA push message, which should trigger a profile re-fetch (it doesn't currently, we fail to parse incoming JSON since we assume it will always have a 'data' key).

If the profile update happens _on_ the current device, we also receive a webchannel "profile update" tickle at around the same time (likely just prior to the FxA push, since it's all local in this case), which also re-fetches the profile. These two messages might coincide, and should be collapsed into one.

Another approach is to ensure we don't send an FxA push message to the device which originated the profile update. That requires server changes, but likely to be less effort overall since I assume all clients will otherwise face the same problem.

Comment 14

[:Grisha Kruglov]

Ryan, what do you think of a server-side change mentioned in Comment 13?

Comment 15

[Ryan Kelly [:rfkelly]]

> Another approach is to ensure we don't send an FxA push message to the device which originated the profile update.

This seems reasonable, I believe we do that for some other push types already e.g. password change.

Comment 16

[Ryan Kelly [:rfkelly]]

I filed https://github.com/mozilla/fxa-auth-server/issues/2084 to track on the auth-server side.  Are you blocked on us getting this done, or is it simply a performance enhancement to avoid redundant work?

Comment 17

[:Grisha Kruglov]

Attachment: Bug 1368147 - Pre: tighten up access levels in AndroidFxAccount

Review commit: https://reviewboard.mozilla.org/r/174768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174768/

Comment 18

[:Grisha Kruglov]

Attachment: Bug 1368147 - Pre: remove unused code

Review commit: https://reviewboard.mozilla.org/r/174770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174770/

Comment 19

[:Grisha Kruglov]

Attachment: Bug 1368147 - Pre: Don't forget to call super.finalize in finalize override

Review commit: https://reviewboard.mozilla.org/r/174772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174772/

Comment 20

[:Grisha Kruglov]

Attachment: Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

TODO:
- test! and perhaps tests
- syncPrefs will be lost after a rename; switch them to be keyed by a UID in the filesystem, and write a migration
- debounce profile update events; we might get both webchannel and fxa push message around the same time, and
-- should only perform one update operation.

Review commit: https://reviewboard.mozilla.org/r/174774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174774/

Comment 21

[:Grisha Kruglov]

It's largely to avoid additional performance enhancement work; however, even with those changes done on the server side, we still have a situation in which a profile update might actually happen on two devices at the same time (perhaps in a sync fest??), in which case a debouncing profile fetcher will still prove handy - although in a rare edge situation.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8902999 [details]
Bug 1368147 - Pre: tighten up access levels in AndroidFxAccount

https://reviewboard.mozilla.org/r/174768/#review180950

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8903000 [details]
Bug 1368147 - Pre: remove unused code

https://reviewboard.mozilla.org/r/174770/#review180952

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8903001 [details]
Bug 1368147 - Pre: Don't forget to call super.finalize in finalize override

https://reviewboard.mozilla.org/r/174772/#review180954

Comment 25

[:Grisha Kruglov]

Ryan, was it ever the case that the 'fxaccounts:login' webchannel message didn't have UID in its payload?

I'm trying to determine if it's possible for UIDs to be missing in persisted account data on any devices in the wild. As far as I can tell, client will just fail silently if that were the case in some rarely-touched points in the codebase, and won't actually crash at any point... and so, we're flying blind here.

Comment 26

[Ryan Kelly [:rfkelly]]

> Ryan, was it ever the case that the 'fxaccounts:login' webchannel message didn't have UID in its payload?

I would be extremely surprised if we had ever send that without the uid in the payload.  If anyone can remember a time when we didn't send the uid in this message, it'll be Shane, so switching ni? to him

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan Kelly [:rfkelly] from comment #26)
> > Ryan, was it ever the case that the 'fxaccounts:login' webchannel message didn't have UID in its payload?
> 
> I would be extremely surprised if we had ever send that without the uid in
> the payload.  If anyone can remember a time when we didn't send the uid in
> this message, it'll be Shane, so switching ni? to him

Android was also late to the WebChannel/custom postMessage party; we used a native interface for a about 10 releases, IIRC, before switching to the about:accounts style.  So the likelihood of an Android device seeing a :login message with no UID is very low, and my vague memory is that it was guaranteed by the fxa-content-server by the time I implemented the about:accounts style flow.

Comment 28

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

More archeology - webchannels landed in desktop in bug 1146904 (https://hg.mozilla.org/mozilla-central/rev/eac6ac60b5e6), and there are a number of references to a uid there - I think that's fairly conclusive (but I'll leave the ni? for Shane as I might be missing something subtle)

Comment 29

[:Grisha Kruglov]

Attachment: Bug 1368147 - Support renaming an account in response to profileUpdate events

TODO:
- test! and perhaps tests
- debounce profile update events; we might get both webchannel and fxa push message around the same time, and
-- should only perform one update operation.

Review commit: https://reviewboard.mozilla.org/r/177144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177144/

Comment 30

[:Grisha Kruglov]

Comment on attachment 8902999 [details]
Bug 1368147 - Pre: tighten up access levels in AndroidFxAccount

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174768/diff/1-2/

Comment 31

[:Grisha Kruglov]

Comment on attachment 8903000 [details]
Bug 1368147 - Pre: remove unused code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174770/diff/1-2/

Comment 32

[:Grisha Kruglov]

Comment on attachment 8903001 [details]
Bug 1368147 - Pre: Don't forget to call super.finalize in finalize override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174772/diff/1-2/

Comment 33

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/1-2/

Comment 34

[:Grisha Kruglov]

All right, thanks folks. I'm leaving in hard assertions about presence of UIDs then.

Patches above "seem to work well" for 21+ devices, and _almost_ work for pre-21 devices. As is evident by the volume of self-questioning TODOs in the code, I'm not yet happy with them, so not marking them for a review just yet. But very soon!

Comment 35

[Shane Tomlinson [:stomlinson]]

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

> I would be extremely surprised if we had ever send that without the uid in
> the payload.  If anyone can remember a time when we didn't send the uid in
> this message, it'll be Shane, so switching ni? to him

We added support to communicate with Fx over WebChannels in [1]. When the `fxaccounts:login` was
sent [2], it already included `uid` [3].

The simple answer - no, we have never knowingly sent an fxaccounts:login message without a uid.

[1] - https://github.com/mozilla/fxa-content-server/commit/92b118c9711e3d69078e9fe68b10d83b0f243715
[2] - https://github.com/mozilla/fxa-content-server/commit/92b118c9711e3d69078e9fe68b10d83b0f243715#diff-cdedc953ca0f16413bc416cdac0a5e0dR152
[3] - https://github.com/mozilla/fxa-content-server/commit/92b118c9711e3d69078e9fe68b10d83b0f243715#diff-cdedc953ca0f16413bc416cdac0a5e0dR158

Comment 36

[:Grisha Kruglov]

Comment on attachment 8902999 [details]
Bug 1368147 - Pre: tighten up access levels in AndroidFxAccount

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174768/diff/2-3/

Comment 37

[:Grisha Kruglov]

Comment on attachment 8903000 [details]
Bug 1368147 - Pre: remove unused code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174770/diff/2-3/

Comment 38

[:Grisha Kruglov]

Comment on attachment 8903001 [details]
Bug 1368147 - Pre: Don't forget to call super.finalize in finalize override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174772/diff/2-3/

Comment 39

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/2-3/

Comment 40

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/1-2/

Comment 41

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/3-4/

Comment 42

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/2-3/

Comment 43

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/3-4/

Comment 44

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/4-5/

Comment 45

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/4-5/

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

https://reviewboard.mozilla.org/r/174774/#review183468

OK, one substantive error that I need clarification on.

This should be easily tested with a JUnit 4/Robolectric migration test, and I think it's worth the effort, since the username rewriting can be subtle.

A few comments, all from memory:

* the pickler was introduced eons ago to work around Android's terrible approach to persistent accounts when Firefox for Android was installed on removable media: they were just deleted when the media was removed!  We no longer allow installing Firefox on removable media.  The pickler is complex and duplicates the Android `AccountManager`, probably badly.  Consider if it's time to get rid of the pickler entirely.

* the user data cache was introduced to work around Android cache invalidation and locking issues.  It might be time to get rid of it entirely.

* the layered user data store was introduced to allow us to be flexible, versioning different pieces of the account, while working around bugs with Android's `get/setUserData` implementations.  I think those bugs are gone.  We might dramatically simplify this whole thing by just using `get/setUserData` and not layeirng the data store.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AccountPickler.java:337
(Diff revision 5)
> +
>        this.state = getState(bundle);
>      }
>  
>      private void unpickleV3(final ExtendedJSONObject json)
> -        throws NonObjectJSONException, NoSuchAlgorithmException, InvalidKeySpecException {
> +        throws NonObjectJSONException, NoSuchAlgorithmException, InvalidKeySpecException, JSONException{

nit: insert space before final {.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AccountPickler.java:377
(Diff revision 5)
>        }
>      }
> +
> +    @Nullable
> +    /* package-private */ String getUID() {
> +      return this.state.uid;

It's not at all obvious, but the original implementation has a 2-layer representation.  The outer layer includes stuff for Android (email, type, a little bit more) and then an inner bundle of stuff for FxA.  The idea was that the inner bundle could get corrupted and the Android account wouldn't be damaged; this was necessary because the `get/setUserData` API was mysteriously buggy (on some devices?  on some versions?), and we didn't trust that we could store `N` pieces of data robustly.  (And we definitely couldn't do so in a single method invocation.)

Reaching into `this.state` like this might fail if the inner bundle of state is corrupt, which definitely runs counter to the original spirit.

However, I'm not sure that the original spirit is represented in the current implementation at this point; or if it should be.  You're very careful to anticipate (unexpected!) null `UID` parameters; can you check that `state` isn't `null` here?  Or is there a guarantee that `state` is non-null at this point?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:184
(Diff revision 5)
>      }
>    }
>  
> +  private String getAccountUID() {
> +    // UID isn't going to change for the lifetime of our account.
> +    // Hopefully this call doesn't go to disk every time...

Generally, no: the Account data is backed by an SQLite database, which is cached in memory.  Old versions of Android (on some devices?) didn't invalidate that cache correctly, perhaps across account deletions in particular (my memory is not precise here).

So I don't think you'll have a huge performance impact here.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:224
(Diff revision 5)
>    /**
>     * Saves the given data as the internal bundle associated with this account.
>     * @param bundle to write to account.
>     */
>    private synchronized void persistBundle(ExtendedJSONObject bundle) {
> -    perAccountBundleCache.put(account.name, bundle);
> +    perAccountBundleCache.put(getAccountUID(), bundle);

I expect an updated `perAccountBundleCache.get(getAccountUID())`, and I'm not seeing it.

Am I missing something?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/Utils.java:251
(Diff revision 5)
>  
>    /**
>     * Get shared preferences path for a Sync account.
>     *
>     * @param product the Firefox Sync product package name (like "org.mozilla.firefox").
> -   * @param username the Sync account name, optionally encoded with <code>Utils.usernameFromAccount</code>.
> +   * @param accountKey local Sync account identifier; 'email' prior to Bug 1368147, 'uid' after.

But wait, there's more!  `usernameFromAccount` was actually working around a Sync 1.0 change as accounts were identified originally by an email address, then by a one-way function of an email address, then by an email address, and now by a UID.

You might not want to put the history lesson in this comment, since consumers should never care.

Comment 47

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

https://reviewboard.mozilla.org/r/174774/#review183490

::: commit-message-a5a09:7
(Diff revision 5)
> +
> +Due to how we access our prefs files (read: all over the place), the idea here is to perform the migration whenever
> +some component actually attempts to get the prefs. This guarantees that every consumer of prefs will receive the
> +correct version, and we won't accidentally duplicate our shared prefs either.
> +
> +I would have preferred to just perform this migration at a set point, but due to presence of background services

That shared point is intended to be https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountUpgradeReceiver.java#29, which should be robust in the face of background services -- but this is done, now.

Comment 48

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

https://reviewboard.mozilla.org/r/177144/#review183492

This all looks sensible.  r- just so that I look at it again, but you're as good (probably better!) at ensuring the details of the replacement are correct as I am.  If I recall correctly there's no way to enumerate `Account` user data, so we just have to remember to update the recreation logic if we ever add anything.  Fun times.  Consider having a static map of keys that are recreated (rather than `currentUserData.putString(ACCOUNT_KEY_ACCOUNT_VERSION, ...)` in the code), although most people will see the existing usage pretty easily as they cult their new `ACCOUNT_KEY_...`.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/AccountLoader.java:153
(Diff revision 5)
>    // Observer which receives notifications when the data changes.
>    protected BroadcastReceiver makeNewObserver() {
>      return new BroadcastReceiver() {
>        @Override
>        public void onReceive(Context context, Intent intent) {
> +        // TODO check if this is just an account rename? Or it doesn't matter?

Why would it be different?  Anything that displays the details of a Firefox Account in Firefox for Android should use an `AccountLoader`, and an account rename should be reflected in the overlying UI.  So either update this comment, or remove it; or explain what I'm missing.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountAbstractActivity.java:22
(Diff revision 5)
>  
>    protected final boolean cannotResumeWhenAccountsExist;
>    protected final boolean cannotResumeWhenNoAccountsExist;
>  
>    public static final int CAN_ALWAYS_RESUME = 0;
> -  public static final int CANNOT_RESUME_WHEN_ACCOUNTS_EXIST = 1 << 0;
> +  public static final int CANNOT_RESUME_WHEN_ACCOUNTS_EXIST = 1;

Eh?  This way of writing the flags (1 << 0, 1 << 1, 1 << 2, ...) is very common and clear.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:139
(Diff revision 5)
>  
>    protected final Context context;
>    private final AccountManager accountManager;
> -  protected final Account account;
>  
> +  // This is really, really meant to be final. Only changed when account name changes.

Man, the Android `Account` API is such a mess.  What happens to deleted `Account` objects?  Answer (as of API 15 or so): nothing, really.  They're just an `(email, type)` tuple that keys in the underlying SQLite DB, and you get some `SecurityException` or something vague if you use a bad `Account` object.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:1023
(Diff revision 5)
>            break;
>        }
>      }
>    }
>  
> +  private void renameAccountIfNecessary(final String profileData, final Runnable callback) {

Explain to me why you don't signal to `callback` whether the attempted rename failed?  That is, it looks like you'll `updateBundleValues` even in the face of garbage, which I don't think was the case before.  That is, now you catch exceptions and and invoke `callback`; before, the exceptions would stop the code before you updated.

Am I wrong?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:1128
(Diff revision 5)
> +      @Override
> +      public void run(AccountManagerFuture<Boolean> future) {
> +        if (future.isCancelled()) {
> +          Logger.error(LOG_TAG, "Account remove task cancelled. Moving on.");
> +          callback.run();
> +          accountManager.setUserData(account, ACCOUNT_KEY_RENAME_IN_PROGRESS, null);

Can we restructure this a little, to have just one point (or just an error point and a success point) where we clear the `RENAME_IN_PROGRESS` flag?

That might mean a boolean `success` flag set from all the future/result considerations.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:1166
(Diff revision 5)
> +          // Oh, this isn't good. This could mean that account already exists, or that something else
> +          // happened. Account should not already exist, since supposedly we just removed it.
> +          // AccountManager docs are not specific on what "something else" might actually mean.
> +          // At this point it's as if we never had an account to begin with, and so we skip calling
> +          // our callback.
> +          // TODO send a telemetry event to monitor if this actually happens in the wild?

Link to ticket.

Comment 49

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #47)
> That shared point is intended to be
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/java/org/mozilla/gecko/fxa/receivers/FxAccountUpgradeReceiver.java#29,
> which should be robust in the face of background services -- but this is
> done, now.

It wasn't obvious to me that this would actually work correctly at all times. Are we guaranteed to have strict ordering of events here? We access sync sharedprefs both from fennec-proper, and from our background code (sync). This class kicks off some work in response to android.intent.action.PACKAGE_REPLACED - which I assume is supposed to fire right after our package is upgraded. Do we know for certain that there won't be any overlap between those upgrade methods and other parts of the codebase that might be reading the prefs? I _think_ that's the case - but it also seemed implausible to synchronize these operations, and tricky to verify we'd end up doing the right thing.

Comment 50

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #49)
> (In reply to Nick Alexander :nalexander [parental leave until September
> 15th] from comment #47)
> > That shared point is intended to be
> > https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> > main/java/org/mozilla/gecko/fxa/receivers/FxAccountUpgradeReceiver.java#29,
> > which should be robust in the face of background services -- but this is
> > done, now.
> 
> It wasn't obvious to me that this would actually work correctly at all
> times. Are we guaranteed to have strict ordering of events here? We access
> sync sharedprefs both from fennec-proper, and from our background code
> (sync). This class kicks off some work in response to
> android.intent.action.PACKAGE_REPLACED - which I assume is supposed to fire
> right after our package is upgraded. Do we know for certain that there won't
> be any overlap between those upgrade methods and other parts of the codebase
> that might be reading the prefs?

I really can't say.  We do many asynchronous things, so it's dangerous to assume that the Android intent has fired -- and completed -- at any particular time :(

 I _think_ that's the case - but it also
> seemed implausible to synchronize these operations, and tricky to verify
> we'd end up doing the right thing.

I concur.  And the approach you sketch here is mostly in place!

Comment 51

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

https://reviewboard.mozilla.org/r/174774/#review183468

> It's not at all obvious, but the original implementation has a 2-layer representation.  The outer layer includes stuff for Android (email, type, a little bit more) and then an inner bundle of stuff for FxA.  The idea was that the inner bundle could get corrupted and the Android account wouldn't be damaged; this was necessary because the `get/setUserData` API was mysteriously buggy (on some devices?  on some versions?), and we didn't trust that we could store `N` pieces of data robustly.  (And we definitely couldn't do so in a single method invocation.)
> 
> Reaching into `this.state` like this might fail if the inner bundle of state is corrupt, which definitely runs counter to the original spirit.
> 
> However, I'm not sure that the original spirit is represented in the current implementation at this point; or if it should be.  You're very careful to anticipate (unexpected!) null `UID` parameters; can you check that `state` isn't `null` here?  Or is there a guarantee that `state` is non-null at this point?

It's helpful to hear the backstory here, thanks!

There's a guarantee that state won't be NULL - our state processing code ensures that it produces something, or fails loudly. This could be clarified in a comment. However, there are no guarantees that state's internal "state" would be sane. We certainly treat it as such elsewhere in the codebase, but I wouldn't bet on it in presense of buggy get/setUserData. You seem optimistic that these bugs have been ironed out, so let's be on the lookout for these failures.

As you note, I don't think this original intent around this separation has been maintained. This separation is rather blurry now, but perhaps I should re-read the code with it in mind.

In either case, I'm inclined to leave this method as-is, unless you still think that would be unwise. Note that we also peak into `state` while constructing AndroidFxAccount just above; changing that behaviour will require further refactoring which isn't obvious to me at the moment.

I'm very much in favor of simplifying/retiring components which outlived their usefuleness, but I don't want to block this feature on that cleanup. Soon!

> I expect an updated `perAccountBundleCache.get(getAccountUID())`, and I'm not seeing it.
> 
> Am I missing something?

Oops.

Comment 52

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

https://reviewboard.mozilla.org/r/177144/#review183492

> Explain to me why you don't signal to `callback` whether the attempted rename failed?  That is, it looks like you'll `updateBundleValues` even in the face of garbage, which I don't think was the case before.  That is, now you catch exceptions and and invoke `callback`; before, the exceptions would stop the code before you updated.
> 
> Am I wrong?

It's not clear to me what the behaviour should be in case of encountering a bad payload here, or any other hard errors. If we don't crash, user's only real recourse at this point, depending on what went wrong, is _probably_ to remove their account entirely and login again. However, we don't signal this state explicitly in the UI...

I think there are three ways this can go wrong.
1) Bug upstream in the client code, which mangled an otherwise good payload.
2) Server bug resulting in a bad payload. In a sense, that's the case as of writing this patch: caching bug on the server results in a structurally correct, but wrong payload (email doesn't change). That's too subtle of an issue and not something client itself can reliably detect.
3) Our Android API calls fail.

For (1), we want to crash.
For (2), ideally we want to be liberal in what we accept from the server, while maintaining a baseline experience in case of server bugs. Another side of this coin is that we often rely on faulty client behaviour to _detect_ server bugs...
For (3), we want to crash if it was our fault, and otherwise we want to be tolerable of bugs in Android. After the initial shake-down on a variety of devices and API levels, it seems that misbehaviour here won't be our fault.

These were my general considerations. It's a hard set of concerns to balance. Things would be easier if we could rely on out-of-band error reporting mechanism, a la Sentry. Relying on crashing the client to detect abnormalities is... non-ideal.

Prior behaviour, as I understand it: we'll call into updateBundleValues regardless of what's in the payload. If it's garbage, we'll write garbage. (note that before, iirc, we did not parse the profile string before writing it, so who knows what's in it).

Updated behaviour: parse the payload assuming some structure ("email" is present), perform renaming if necessary; if payload doesn't parse as JSON, or doesn't contain an "email", proceed as before. If we fail otherwise, proceed as before (updating the bundle). And so, the behavior is "same as before, but now with an extra cautious, intermediary step".

The only exception here is failing to add an account after deleting it on pre-21 devices. It's not clear to me what that state actually entails. It seems that proceedig to persist a bundle is unnecessary at this point, since we probably don't even have a valid Android account anymore.

In case of non-recoverable errors (but, what are they?), we could consider cleaning up entirely - as if the user logged out... I'm not sure if that's a good step to take though.

Comment 53

[:Grisha Kruglov]

Comment on attachment 8902999 [details]
Bug 1368147 - Pre: tighten up access levels in AndroidFxAccount

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174768/diff/3-4/

Comment 54

[:Grisha Kruglov]

Comment on attachment 8903000 [details]
Bug 1368147 - Pre: remove unused code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174770/diff/3-4/

Comment 55

[:Grisha Kruglov]

Comment on attachment 8903001 [details]
Bug 1368147 - Pre: Don't forget to call super.finalize in finalize override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174772/diff/3-4/

Comment 56

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/5-6/

Comment 57

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/5-6/

Comment 58

[:Grisha Kruglov]

Updated patches as per feedback. Thanks, Nick. Unit tests for sync pref migration upcoming.

Comment 59

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

https://reviewboard.mozilla.org/r/174774/#review183840

Comment 60

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

https://reviewboard.mozilla.org/r/177144/#review183842

Comment 61

[Sebastian Kaspari (:sebastian; :pocmo)]

Mostly rubber-stamped this. I'm not all too familiar with this part of the code and I think nalexander has you covered :)

Comment 62

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

https://reviewboard.mozilla.org/r/174774/#review183978

I think this looks good.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:236
(Diff revisions 5 - 6)
>    /**
>     * Retrieve the internal bundle associated with this account.
>     * @return bundle associated with account.
>     */
>    private synchronized ExtendedJSONObject unbundle(boolean allowCachedBundle) {
> +    final String accountUID = getAccountUID();

We should bail if `accountUID` is null, right?

::: commit-message-a5a09:7
(Diff revision 6)
> +
> +Due to how we access our prefs files (read: all over the place), the idea here is to perform the migration whenever
> +some component actually attempts to get the prefs. This guarantees that every consumer of prefs will receive the
> +correct version, and we won't accidentally duplicate our shared prefs either.
> +
> +I would have preferred to just perform this migration at a set point, but due to presence of background services

Let's include a note about why we don't trust the package update system intent.

Comment 63

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

https://reviewboard.mozilla.org/r/177144/#review183980

Nifty!

Just for posterity, I thought of another way to address this that might have been good.  One could persist the changed account name somewhere but not act on it immediately.  This might avoid races and complexity when push notifications come in changing the account name, but while the account is being viewed in the app.  Then we could push the check for a changed account into the unpickling code (or the unpickling call site), which, by definition, has existing code already ready to handle new "unpickled" accounts.  That way we might narrow the set of interactions between the add-remove code and users of the account.

Just a note for future archaeologists.

This does remind me: test this with a push notification arriving while the status activity is open, and arriving while the send tab list is open.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:135
(Diff revisions 5 - 6)
>      m.put(BrowserContract.AUTHORITY, true);
>      DEFAULT_AUTHORITIES_TO_SYNC_AUTOMATICALLY_MAP = Collections.unmodifiableMap(m);
>    }
>  
> +  // List of accounts keys which are explicitly carried over when account is renamed on pre-21 devices.
> +  private static final List<String> ACCOUNT_KEYS_TO_CARRY_OVER_ON_RENAME;

It would be nice if `ACCOUNT_KEY_` matched this set in DXR; maybe `ACCOUNT_KEY_TO_CARRY_OVER_SET`?

Comment 64

[:Grisha Kruglov]

Comment on attachment 8902999 [details]
Bug 1368147 - Pre: tighten up access levels in AndroidFxAccount

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174768/diff/4-5/

Comment 65

[:Grisha Kruglov]

Comment on attachment 8903000 [details]
Bug 1368147 - Pre: remove unused code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174770/diff/4-5/

Comment 66

[:Grisha Kruglov]

Comment on attachment 8903001 [details]
Bug 1368147 - Pre: Don't forget to call super.finalize in finalize override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174772/diff/4-5/

Comment 67

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/6-7/

Comment 68

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/6-7/

Comment 69

[:Grisha Kruglov]

Comment on attachment 8903002 [details]
Bug 1368147 - Migrate sync and reading list preferences to be keyed by account UID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174774/diff/7-8/

Comment 70

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/7-8/

Comment 71

[:Grisha Kruglov]

Addressed final feedback, added some unit tests. Let's see how it does on try; in the meantime, I'll run some more manual tests, and will land the series afterwards.

Comment 72

[:Grisha Kruglov]

One small hurdle: on pre-21 devices, after account is renamed its automatic syncing is disabled on the system level. It's a simple UI toggle, but hidden deep in the system prefs. We have different syncing intervals depending on our current sync constellation, verification state, etc, and so I think the simplest thing would be to simple kick-off an immediate sync after account is renamed - which should take care of setting up correct sync intervals.

Comment 73

[:Grisha Kruglov]

(In reply to :Grisha Kruglov from comment #72)
> One small hurdle: on pre-21 devices, after account is renamed its automatic
> syncing is disabled on the system level. It's a simple UI toggle, but hidden
> deep in the system prefs. We have different syncing intervals depending on
> our current sync constellation, verification state, etc, and so I think the
> simplest thing would be to simple kick-off an immediate sync after account
> is renamed - which should take care of setting up correct sync intervals.

Actually, scratch that. Calling into setAuthoritiesToSyncAutomaticallyMap which sets of correct system flags for the new account is likely to be enough.

Comment 74

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #72)
> One small hurdle: on pre-21 devices, after account is renamed its automatic
> syncing is disabled on the system level. It's a simple UI toggle, but hidden
> deep in the system prefs.

This needs to be addressed; it has to do with the ContentResolver and sync authorities.  The existing code is complex, since it wanted to handle opting-out of Reading List and new authorities.  But it should be possible to add the account with the syncing preserved with the existing state.

Good catch!

 We have different syncing intervals depending on
> our current sync constellation, verification state, etc, and so I think the
> simplest thing would be to simple kick-off an immediate sync after account
> is renamed - which should take care of setting up correct sync intervals.

The sync intervals are mostly controlled by the SyncAdapter backoff code, so yes, forcing a sync should do this.  But it won't do anything if syncing is disabled, IIRC.  (Maybe IMMEDIATE ignores the disable flag?  We talked about this recently.)

Comment 75

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/8-9/

Comment 76

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/9-10/

Comment 77

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/9-10/

Comment 78

[:Grisha Kruglov]

Above patches do the trick for pre-21 APIs - system settings and intervals are carried over, it seems. Unfortunately, the post-21 APIs aren't much more helpful in reality - they carry over userData associated with the account, but drop everything else (configured periodic syncs).

So, we need to do the same migration of settings and intervals regardless of what device we're running on.

Comment 79

[:Grisha Kruglov]

Comment on attachment 8905350 [details]
Bug 1368147 - Support renaming an account in response to profileUpdate events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177144/diff/10-11/

Comment 80

[:Grisha Kruglov]

All right, I think we can now let this bake on Nightly and start testing in earnest.

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f05ca5366731
Pre: tighten up access levels in AndroidFxAccount r=sebastian
https://hg.mozilla.org/integration/autoland/rev/487a253f5548
Pre: remove unused code r=sebastian
https://hg.mozilla.org/integration/autoland/rev/0ca257bd9dcb
Pre: Don't forget to call super.finalize in finalize override r=sebastian
https://hg.mozilla.org/integration/autoland/rev/d881fed7d5ca
Migrate sync and reading list preferences to be keyed by account UID r=nalexander,sebastian
https://hg.mozilla.org/integration/autoland/rev/3044b28ca2ba
Support renaming an account in response to profileUpdate events r=nalexander,sebastian

Comment 82

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f05ca5366731
https://hg.mozilla.org/mozilla-central/rev/487a253f5548
https://hg.mozilla.org/mozilla-central/rev/0ca257bd9dcb
https://hg.mozilla.org/mozilla-central/rev/d881fed7d5ca
https://hg.mozilla.org/mozilla-central/rev/3044b28ca2ba