959548 - Determine which identity manager to use as Sync initializes

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

Attachment: Determine-what-identity-manager-to-use-as-sync-is-in.patch

In Fx 29, we want sync to initialize using the old identity manager if sync is already configured, or initialize with the Fxa identity manager if unconfigured.

This patch sticks with the existing preference identity.fxaccounts.enabled.  If this preference already exists, we trust it and use whatever identity manager it implies.

If this pref is not set, we look and see if sync is already configured, and if so, we set this preference to false.  Otherwise, we set this pref to true, meaning that a future configure of sync will take the Fxa path.  It depends on that patch in bug 959222 (even though the patch in that bug may change slightly)

Comment 1

[Chris Karlof [:ckarlof]]

Comment on attachment 8359745 [details] [diff] [review]
Determine-what-identity-manager-to-use-as-sync-is-in.patch

>--- a/services/sync/Weave.js
>+++ b/services/sync/Weave.js
>@@ -65,20 +65,34 @@ WeaveService.prototype = {
>+      try {
>+        fxAccountsEnabled = !Services.prefs.getCharPref("services.sync.username");
>+      } catch (_) {

Weave.js does something similar to determine if it's enabled. The pattern it uses is:

let prefs = Services.prefs.getBranch(SYNC_PREFS_BRANCH);
fxAccountsEnabled = !prefs.prefHasUserValue("username");

FxA based Sync would also likely use this pref, but I assume the idea is that FxA based Sync would always have identity.fxaccounts.enabled set to true.

I note that this patch would effectively "turn on" FxA based Sync in Nightly. In our Sync standup today we decided we would make the decision to do that on 1/24/14. It's good to have this ready to go though.

Comment 2

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

(In reply to Chris Karlof [:ckarlof] from comment #1)
> Comment on attachment 8359745 [details] [diff] [review]
> Determine-what-identity-manager-to-use-as-sync-is-in.patch
> 
> >--- a/services/sync/Weave.js
> >+++ b/services/sync/Weave.js
> >@@ -65,20 +65,34 @@ WeaveService.prototype = {
> >+      try {
> >+        fxAccountsEnabled = !Services.prefs.getCharPref("services.sync.username");
> >+      } catch (_) {
> 
> Weave.js does something similar to determine if it's enabled. The pattern it
> uses is:
> 
> let prefs = Services.prefs.getBranch(SYNC_PREFS_BRANCH);
> fxAccountsEnabled = !prefs.prefHasUserValue("username");

Yes, thanks, I'll change it to do this.

> FxA based Sync would also likely use this pref, but I assume the idea is
> that FxA based Sync would always have identity.fxaccounts.enabled set to
> true.

Yep.

> I note that this patch would effectively "turn on" FxA based Sync in
> Nightly. In our Sync standup today we decided we would make the decision to
> do that on 1/24/14. It's good to have this ready to go though.

Yeah, I'll likely land many of these patches in one hit after coordinating with everyone else.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8359745 [details] [diff] [review]
Determine-what-identity-manager-to-use-as-sync-is-in.patch

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

::: services/sync/Weave.js
@@ +80,5 @@
> +      // that pref doesn't exist - so let's assume this is a first-run
> +      // If sync already appears configured, we assume it's for the legacy
> +      // provider.
> +      try {
> +        fxAccountsEnabled = !Services.prefs.getCharPref("services.sync.username");

prefHasUserValue?

@@ +83,5 @@
> +      try {
> +        fxAccountsEnabled = !Services.prefs.getCharPref("services.sync.username");
> +      } catch (_) {
> +        // no username pref, which means not configured, which means fxa.
> +        fxAccountsEnabled = true;

const SHOULD_ENABLE_FXA = false;

...

    fxAccountsEnabled = SHOULD_ENABLE_FXA;

This way we at least test this code in the wild for a few days, and the turn-on patch is a single line.

Comment 4

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

Attachment: 0007-Bug-959548-Determine-what-identity-manager-to-use-as.patch

(In reply to Richard Newman [:rnewman] from comment #3)
> const SHOULD_ENABLE_FXA = false;
> 
> ...
> 
>     fxAccountsEnabled = SHOULD_ENABLE_FXA;

This patch takes a slightly different route - it uses the pref "identity.fxaccounts.enabled" to indicate if the feature is enabled in any way - the idea is that this patch could land now, but users still will never see Fxa-based sync unless they have manually set that pref.  The "turn on" patch then just removes the few lines that checks for this pref.

Assuming this pref has been manually set, the pref "services.sync.fxaccounts.enabled" indicates the current state of Fxa - ie, whether we previously determined that sync was already configured for the legacy provider and thus should keep using it.

I re-jigged this as I feel it will be difficult for us to make a decision about the readiness of Fxa without the patches landed which allow it to be tested.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

I feel like this should be a method in FxAccounts.jsm. I would need this for bug 957460 as well.

I wonder whether this method should really set a pref like "services.sync.fxaccounts.enabled" on first run to detect whether to use FxA or not. Wouldn't it be sufficient to check whether we have a user signed into FxA?

Should they not be signed into FxA and have sync setup we just report FxA as disabled and won't bother them. I feel like about:accounts should probably just redirect or not exist for people in that group. If still somehow someone manages to sign into FxA with OldSync I guess we should just not care?

Checking "identity.fxaccounts.enabled" for now seems like a good idea to me to not force this onto everyone on Nightly.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-959548-Determine-which-identity-manager-to-use-a.patch

So... how about this?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-959548-Determine-which-identity-manager-to-use-a.patch, v2

Missed some .fxAccountsEnabled usages.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-959548-Determine-which-identity-manager-to-use-a.patch, v3

Trying to use that new function, I think it would actually be beneficial if it would resolve to [enabled, userData] so that for example about:home and other entry points don't have to call .getSignedInUser() themselves again but could just use the data we requested.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-959548-Determine-which-identity-manager-to-use-a.patch, v4

Sorry, fixing some small mistakes.

Comment 10

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> I feel like this should be a method in FxAccounts.jsm. I would need this for
> bug 957460 as well.

I don't quite understand this - see below, but once fxAccountsEnabled returns true, you should blindly trust that Fxa is being used for sync, regardless of the current Fxa logged-in state.

> I wonder whether this method should really set a pref like
> "services.sync.fxaccounts.enabled" on first run to detect whether to use FxA
> or not. Wouldn't it be sufficient to check whether we have a user signed
> into FxA?
> 
> Should they not be signed into FxA and have sync setup we just report FxA as
> disabled and won't bother them.

I'm actually trying to prevent this sync usage from dictating if Fxa is globally disabled.  I'm thinking of a future where Fxa will be used for other services or features, but the user still has "legacy" sync configured.  IOW, I'm trying to avoid globally disabling Fxa just because the user is using an old sync.

So "services.sync.fxaccounts.enabled" just reflects sync and isn't a global state for Fxa in general.

The cases we need to for sync are:

1) Not signed into Fxa, sync not configured (the case for a new user).  In this case Fxa must be enabled and "old sync" is not available, preiod.
2) Previously configured Sync with Fxa, but are now signed out.  This can happen if the server rejects auth, such as when the user changes their password on another device.
3) Signed into sync with an old identity - in this case we want sync to *never* use Fxa, even though other things might.
4) Signed into sync with an old identity, but they "unlink" the device - this resets them back to state (1)

So really, it is (2) that is the problem - the fact sync is configured but they are not logged into Fxa is a real possibility, and we must not mistake that state for "configured with old style sync".  "services.sync.fxaccounts.enabled" captures this state - effectively it records "have we previously determined we should be using Fxa for sync" as it seems difficult to always deduce this from the *current* state.  Note that once we've determined they should be using Fxa for sync, old-style sync is totally hidden.  To put this another way, once fxAccountsEnabled returns true, there is nothing that can happen in the future that will make it return false

> Checking "identity.fxaccounts.enabled" for now seems like a good idea to me
> to not force this onto everyone on Nightly.

Keep in mind though that the intention is to remove this pref entirely once we decide we are going ahead with this.  IOW, this pref is the "go/no-go" flag.

Regarding bug 962729 (Sync toolbar button and menu item need to open the prefs dialog when logged in) - this should just be a matter of checking if the fxAccountsEnabled flag is true and fxAccounts.getSignedInUser() provides data?

Given the above, I'm not particularly comfortable with this patch.  I think checking the non-promise-based fxAccountsEnabled flag on the top-level Weave service will do the right thing, is suitable for all the other use-cases we have and doesn't prevent other things using Fxa in the future.

Comment 11

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

Comment on attachment 8363877 [details] [diff] [review]
0002-Bug-959548-Determine-which-identity-manager-to-use-a.patch, v4

Marking as obsolete as Tim mentioned in IRC he's happy with the original approach.

Comment 12

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

Attachment: 0004-Bug-959548-Determine-what-identity-manager-to-use-as.patch

I've rebased this patch so it shouldn't depend on any other bugs.  Carrying r=ckarlof forward, but rnewman, if you could review this soon I think it is safe to land.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8364090 [details] [diff] [review]
0004-Bug-959548-Determine-what-identity-manager-to-use-as.patch

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

::: services/sync/Weave.js
@@ +92,5 @@
> +      // If sync already appears configured, we assume it's for the legacy
> +      // provider.
> +      let prefs = Services.prefs.getBranch(SYNC_PREFS_BRANCH);
> +      fxAccountsEnabled = !prefs.prefHasUserValue("username");
> +      Services.prefs.setBoolPref("services.sync.fxaccounts.enabled", fxAccountsEnabled);

Here's a flow.

* Set up old Sync.
* Upgrade to 29.
* Cause fxAccountsEnabled to be called. This pref is set to false.
* Turn off old Sync.
* Now try to set up FxA. You can't: the pref is still set. (What even happens in this case?)

As I understand it, this is how we'd tell people to manually migrate. If so, this seems to add some additional pain: they have to get cozy with about:config.

Two proposals:

* Alter this check: only use the pref in the positive case. Once you set up FxA, you're locked into FxA until you delete services.sync.fxaccounts.enabled.

* Delete that pref when deleting the old Sync account.

Comment 14

[Mark Hammond [:markh] [:mhammond] [PTO until Sept 7]]

(In reply to Richard Newman [:rnewman] from comment #13)
> Here's a flow.
> 
> * Set up old Sync.
> * Upgrade to 29.
> * Cause fxAccountsEnabled to be called. This pref is set to false.
> * Turn off old Sync.

"Turn off old Sync" is "unlink device", right?  If so, patch 2 in bug 959222 also flips this flag back to undefined, then re-initializes the identity manager, which will reset this flag to true.

IIUC, that solves this problem, right?

/me runs off to check bug 959222 does indeed have the latest version of the patch...

Comment 15

[Richard Newman [:rnewman]]

E_TOO_MANY_PATCHES

Great, so you already take my second suggested approach. Sounds good.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/690fc57732bb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/690fc57732bb