965896 - allow legacy sync reset to expose FxA accounts

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

Followup for patch 2 (attachment 8366432 [details] [diff] [review]) in bug 959222.

See bug 959222 comment 35 and bug 959222 comment 38.

Comment 1

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

Attachment: 0001-Bug-965896-allow-a-legacy-sync-reset-to-re-enable-fx.patch

rnewman: this is an update to the "part 2" patch from bug 959222 which you previously gave feedback on.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8369003 [details] [diff] [review]
0001-Bug-965896-allow-a-legacy-sync-reset-to-re-enable-fx.patch

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

I thought this looked familiar :)

::: services/sync/modules/service.js
@@ +888,5 @@
>      Svc.Prefs.set("lastversion", WEAVE_VERSION);
>  
>      this.identity.deleteSyncCredentials();
>  
> +    // Reset the identity manager, then re-initialize it so the Fxa manager is

Nit: s/Fxa/FxA

@@ +893,5 @@
> +    // used.
> +    this.identity.username = "";
> +    Services.prefs.clearUserPref("services.sync.fxaccounts.enabled");
> +    this.status.__authManager = null;
> +    this.identity = Status._authManager;

Can this getter fail?

Comment 3

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

(In reply to Richard Newman [:rnewman] from comment #2)
Thanks!

> Nit: s/Fxa/FxA
Fixed.

> Can this getter fail?

In theory, no.  There are no error paths in browserid_identity, and the sync init code where the identity manager is first initialized also makes no attempt to catch errors, so I left this alone.

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

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/cff85c90a8a3 for https://tbpl.mozilla.org/php/getParsedLog.php?id=34109165&tree=Fx-Team

Comment 5

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

(In reply to Phil Ringnalda (:philor) from comment #4)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/cff85c90a8a3
> for https://tbpl.mozilla.org/php/getParsedLog.php?id=34109165&tree=Fx-Team

Yeah, for (retrospectively) obvious reasons :(

rnewman: there are 44 explicit calls to Service.startOver() in the sync unit tests, and they all expect the old identity manager to exist after it completes.  Some do this just to cleanup (but following tests still expect the old identity), while others explicitly test certain things after it is called.  It could *almost* be argued this latter class of tests are no longer valid (but even if I could make a case for that, it still leaves the former class).

I've no good ideas here.  Among the not-good ideas are:

* A special pref just for testing that tells startOver to not reset the identity.

* Change every relevant add_test call to use a new helper like "add_identity_test" - something like "add_legacy_identity_test".  This could have an observer that notes the startover-finish observer and resets the identity back to a legacy one.  This might not even work in practice, as part of startOver is now async

* Add a "global" helper function - something like "startOverWithLegacyIdentity()" - that does the same observer trick (but this can probably spin until that async notification fires)

Do you have any better ideas, or failing that, which of the above do you consider the least-worst?

Comment 6

[Richard Newman [:rnewman]]

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

In short: probably a combination of all of these things :(


> * A special pref just for testing that tells startOver to not reset the
> identity.

I'd generalize this a little bit: make the behavior of startOver configurable (indeed, pref-driven), such that in one state a reset will switch over to the FxA identity system, and in the other it'll follow the old code path (modulo potential asyncness). Configure appropriately before each suite of tests runs. That gets you the old tests passing, perhaps with some mechanical async adjustments.

That doesn't solve the whole problem, of course -- you'll still have tests poking at the wrong things in FxA-mode. But I think it's one of the least-hacky starting points, given that we're warping a test suite to incorporate flexibility it was never designed to have.

For the tests that are explicitly testing or relying on the behavior of startOver -- well, those have to change, because you're changing the behavior of the thing under test. For those tests, yeah, start to partition them between old-only and new-only, so you end up with three sets of tests:

* old only
* run for old, then run for new
* new only

Then you'll need to make sure there's coverage for the new behavior.

Comment 7

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

Attachment: 0002-Bug-965896-allow-a-sync-startOver-to-re-enable-Firef.patch

This patch has a number of differences from the last one:

* browserid_identity.js used the module global |fxAccounts| instead of |this._fxaService| which causes a problem for testing - this._fxaService is replaced with a mocked version by the tests, where the global remains the real, unmocked version.  I added comments where this._fxaService is initially setup to try and avoid this happening in the future.

* startOver() now looks for a pref "services.sync-testing.startOverKeepIdentity" to see if it should clobber the identity or not.  This isn't in the "services.sync" branch as resetPrefs() is regularly called for that branch, which defeats the test suite's attempt at keeping it set to true.  This pref is set in head_helpers.js - we can work out how to better generalize some of this when we have more time to spend on tests.

* A new test specifically for startOver - it checks various things are reset and the FxA identity manager is created.

Enough changes here that a re-review is warranted :(

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8371973 [details] [diff] [review]
0002-Bug-965896-allow-a-sync-startOver-to-re-enable-Firef.patch

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

Looks sane to me.

::: services/sync/modules/service.js
@@ +892,5 @@
> +    // If necessary, reset the identity manager, then re-initialize it so the
> +    // FxA manager is used.  This is configurable via a pref - mainly for tests.
> +    let keepIdentity = false;
> +    try {
> +      keepIdentity = Services.prefs.getBoolPref("services.sync-testing.startOverKeepIdentity");

let keepIdentity = Svc.Prefs.get("services.sync-testing.startOverKeepIdentity", false);

::: services/sync/tests/unit/test_fxa_startOver.js
@@ +27,5 @@
> +
> +  // remember some stuff so we can reset it after.
> +  let oldIdentidy = Service.identity;
> +  let oldClusterManager = Service._clusterManager;
> +  let defered = Promise.defer();

deferred

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8371973 [details] [diff] [review]
0002-Bug-965896-allow-a-sync-startOver-to-re-enable-Firef.patch

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

::: services/sync/tests/unit/test_fxa_startOver.js
@@ +10,5 @@
> +  initTestLogging("Trace");
> +  run_next_test();
> +}
> +
> +add_task(function test_startover() {

Nit: I've been told to use function* when declaring generator functions.

Comment 10

[Richard Newman [:rnewman]]

I just spotted something missing here.

Weave.fxAccountsEnabled needs to be flipped when this reset happens, too, else modules that dynamically check that -- such as FHR -- will be incorrect. (Encountered when reviewing Bug 958561 -- please coordinate with gps.)

Comment 11

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

(In reply to Richard Newman [:rnewman] from comment #10)
> I just spotted something missing here.
> 
> Weave.fxAccountsEnabled needs to be flipped when this reset happens, too,
> else modules that dynamically check that -- such as FHR -- will be
> incorrect.

Huh.  This version of the patch doesn't have the change to Weave.js specifically to handle this.  I wonder where it went :)  But thanks for noticing this, and I'll also add a check t the new test for this.

Comment 12

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

Thanks - fixed all review comments and reinstated the code (and added a test) to ensure .fxAccountsEnabled toggles correctly.

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

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c8670d2e6efd for https://tbpl.mozilla.org/php/getParsedLog.php?id=34389306&tree=Fx-Team and https://tbpl.mozilla.org/php/getParsedLog.php?id=34389300&tree=Fx-Team

Comment 14

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

(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 8371973 [details] [diff] [review]
> ::: services/sync/modules/service.js
> @@ +892,5 @@
> > +    // If necessary, reset the identity manager, then re-initialize it so the
> > +    // FxA manager is used.  This is configurable via a pref - mainly for tests.
> > +    let keepIdentity = false;
> > +    try {
> > +      keepIdentity = Services.prefs.getBoolPref("services.sync-testing.startOverKeepIdentity");
> 
> let keepIdentity =
> Svc.Prefs.get("services.sync-testing.startOverKeepIdentity", false);

Doh - I thought this wouldn't work due to Svc.Prefs using a different root branch, but assumed there might be magic I didn't know about so tried it anyway - and thought I'd re-run the tests.

Turns out I was correct first time, and wrong the second time :/

Re-landed with the original block left intact (ie, using Services.prefs directly)

https://hg.mozilla.org/integration/fx-team/rev/3e8524fcafcd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e8524fcafcd

Comment 16

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

Attachment: As landed

Patch as landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Sync.Next
User impact if declined: Existing users of Sync will be unable to reset and use Firefox Accounts
Testing completed (on m-c, etc.): Landed on m-c, has test coverage.
Risk to taking this patch (and alternatives if risky): Risk limited to sync
String or IDL/UUID changes made by this patch: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e1a2fd4a2a0