959222 - Make browserid_identity a first-class identity module

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

Attachment: Make-browserid_identity-a-first-class-identity-modul.patch

Currently sync initializes with the legacy identity provider.  When the final-ui-startup observer fires, we wait 10 seconds, then replace the identity module with browserid_identity and do the Fxa dance - ie, it's kinda a "second class" identity module as it gets monkey-patches in.  Among the problems with this, one is that sync may initialize before the timer fires (eg, when sync options is shown) causing a number of issues.

This patch tries to make browserid_identity a "first class" provider.  It:

* Changes status.js so that _authManager is a getter which determines what provider to use based on the same preference Weave.js currently uses.

* Changes service.js so that identity is a getter, which simply delegates to status._authManager.

* Adds a new promise-based method initializeIdentityManager to both providers - this is a noop for the legacy provider, and all of the maybeInitWithFxAccountsAndEnsureLoaded() logic is moved from Weave.js into this new method in bid_identity.

* Weave.js's ensureLoaded now calls initializeIdentityManager for the current provider.

The end result should be that this makes the browserid_identity module a first-class identity module, and moves much of the Fxa-specific code out of Weave.js and back into bid_identity where it belongs.

Comment 1

[Richard Newman [:rnewman]]

See also Bug 584771.

Comment 2

[Richard Newman [:rnewman]]

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

> * Changes service.js so that identity is a getter, which simply delegates to
> status._authManager.

See Bug 949260, particularly my justification for not doing that…

Comment 3

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

Attachment: 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

This patch removes the controversial changes in the last patch - identity is no longer a getter, so what's left is the new initializeIdentityManager() method, the move of the fxa-specific code from Weave.js to bid_identity.js and the initialization if ._authManager based in the preference value

Note that I will probably still upload a "part 2" with the controversial changes - I failed to sell the idea of disallowing existing sync users to "unlink" so they too can use Fxa, but let's cross that bridge when I have a patch I'm happy with.

Comment 4

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

Richard: We had a discussion in IRC where you suggested re-calling onStartup - however, it seems that's going to be quite painful as there aren't relevant "uninit" methods available - eg, there seem to be *lots* of observers I'd need to remove (particularly in the SyncScheduler) and a number of similar issues.

Given this, and given our use-case is really just for a once-off reset from a legacy account to an un-logged-in Fxa account, do you think we can get away with not re-calling onStartup and just have .startOver pull a few hacks to re-init enough things to keep things working?

IOW, what's the least amount of work you think I can get away with for the forthcoming "part 2"?

Comment 5

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

*sob* - another complication is when a user logs out of their fxa account and logs into another.  There's another bug on that which calls for warnings, but there isn't anything in place to reset and start sync over again using the new identity.

Comment 6

[Richard Newman [:rnewman]]

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

What about splitting onStartup into onStartup and onAccountReady?

Observers etc. in the former, account-related internal configuration in the latter.

We don't really have to solve the observer/init problem, only being able to re-configure Sync itself when an authenticator changes.

Comment 7

[Richard Newman [:rnewman]]

Target Milestone tracks code landings, not (as you might imagine) target milestones.

I guess we can use status-* for this.

Comment 8

[Chris Karlof [:ckarlof]]

(In reply to Mark Hammond [:markh] from comment #5)
> *sob* - another complication is when a user logs out of their fxa account
> and logs into another.  There's another bug on that which calls for
> warnings, but there isn't anything in place to reset and start sync over
> again using the new identity.

In our first work week, we tried to handle this by observing 'fxaccounts:onlogout'. And then calling Weave.Service.startOver(). It's currently commented out in Weave.js, because I recall startOver() makes some calls to the storage server to clean up, but these need to be authenticated and the user has already logged out! We need to address this.

Comment 9

[Chris Karlof [:ckarlof]]

Comment on attachment 8360002 [details] [diff] [review]
0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

::: services/sync/modules/browserid_identity.js
@@ +57,5 @@
>    // we don't consider the lack of the bundle to mean that the authState is
>    // such that we need to authorize.
>    _shouldHaveSyncKeyBundle: false,
>  
> +  initializeIdentityManager: function() {

can this be named "initialize" instead?

@@ +65,5 @@
> +
> +    // This isn't quite sufficient here to handle all the cases. Cases
> +    // we need to handle:
> +    //  - User is signed in to FxAccounts, btu hasn't set up sync.
> +    return fxAccounts.getSignedInUser().then(

With this code reorganization, it now does a double fetch of the user info from FxAccounts.jsm in browserid_identity, once here and once in initWithLoggedInUSer(). I think it would be nice to refactor the bulk of what initWithLoggedInUser does into here.

@@ +79,5 @@
> +            // and records that status in Weave.Status
> +            if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
> +              // This makes sure that Weave.Service is loaded
> +              Svc.Obs.notify("weave:service:setup-complete");
> +              // TODO: this shouldn't be here. It should be at the end

I can't remember the status of this, but if we can remove this cruft, we should.

@@ +88,5 @@
> +            }
> +          }.bind(this));
> +        } else if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
> +          // This makes sure that Weave.Service is loaded
> +          this.ensureLoaded();

This was a function in Weave.js, not in the browserid_identity.js.

Comment 10

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

A new patch will address these comments, except for:

(In reply to Chris Karlof [:ckarlof] from comment #9)
> Comment on attachment 8360002 [details] [diff] [review]
> @@ +65,5 @@
> > +
> > +    // This isn't quite sufficient here to handle all the cases. Cases
> > +    // we need to handle:
> > +    //  - User is signed in to FxAccounts, btu hasn't set up sync.
> > +    return fxAccounts.getSignedInUser().then(
> 
> With this code reorganization, it now does a double fetch of the user info
> from FxAccounts.jsm in browserid_identity, once here and once in
> initWithLoggedInUSer(). I think it would be nice to refactor the bulk of
> what initWithLoggedInUser does into here.

I'm not sure what you mean here.  This block was simply moved from Weave.js into browserid_identity.js.  There is some double-fetching of the logged in data, but I don't believe this patch has changed that in any way.  Can you please clarify?

Comment 11

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

Attachment: 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

I think this is getting close to ready, but not requesting review until Chris clarifies the other comment on the previous patch, but I think I've addressed all other comments.

This version removes lots of cruft.  It allows a user to sign in and out of Fxa and sync "does the right thing".  It doesn't attempt to handle the case of a different Fxa user signing in - that's bug 958927.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8361141 [details] [diff] [review]
0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

::: services/sync/modules/browserid_identity.js
@@ +71,5 @@
> +    Components.utils.import("resource://services-sync/main.js");
> +
> +    // This isn't quite sufficient here to handle all the cases. Cases
> +    // we need to handle:
> +    //  - User is signed in to FxAccounts, btu hasn't set up sync.

s/btu/but

File a follow-up and leave the Bug # here?

@@ +105,5 @@
> +      // Bug 958927 exists to work out what to do if that's not true.  It might
> +      // be that the :onlogout observer does a .startOver (or maybe not - TBD)
> +      // But for now, do nothing, and sync will just start re-synching in its
> +      // own sweet time...
> +      this.initializeWithCurrentIdentity();

You might also need to call one of the login methods in Service, or otherwise let Sync know what's going on -- as soon as it tries to sync after logging out, it'll enter Status.login = LOGIN_FAILED_NO_USERNAME.

::: services/sync/modules/status.js
@@ +28,5 @@
> +                    .getService(Components.interfaces.nsISupports)
> +                    .wrappedJSObject;
> +    let idClass = service.fxAccountsEnabled ? BrowserIDManager : IdentityManager;
> +    this.__authManager = new idClass();
> +    this.__authManager.initialize();

You need to spin on this promise, or the caller of this is going to have a Bad Time.

Comment 13

[Chris Karlof [:ckarlof]]

Comment on attachment 8361141 [details] [diff] [review]
0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

::: services/sync/modules/browserid_identity.js
@@ +111,5 @@
> +
> +    case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> +      this.resetCredentials();
> +      this.username = null;
> +      this._account = null;

I'm not sure if this is everything we need to do to "reset". I think we should do Weave.Service.startOver(), but I that does some calls to the server, which is problematic, but we are unable to authenticate those calls after the user has logged out.

::: services/sync/modules/status.js
@@ +28,5 @@
> +                    .getService(Components.interfaces.nsISupports)
> +                    .wrappedJSObject;
> +    let idClass = service.fxAccountsEnabled ? BrowserIDManager : IdentityManager;
> +    this.__authManager = new idClass();
> +    this.__authManager.initialize();

Richard is correct.

Comment 14

[Chris Karlof [:ckarlof]]

I think we're going to need to re-visit the "unlink"/"logout" path, but otherwise looks fine.

Comment 15

[Chris Karlof [:ckarlof]]

(In reply to Mark Hammond [:markh] from comment #10)
> A new patch will address these comments, except for:
> 
> (In reply to Chris Karlof [:ckarlof] from comment #9)
> > Comment on attachment 8360002 [details] [diff] [review]
> > @@ +65,5 @@
> > > +
> > > +    // This isn't quite sufficient here to handle all the cases. Cases
> > > +    // we need to handle:
> > > +    //  - User is signed in to FxAccounts, btu hasn't set up sync.
> > > +    return fxAccounts.getSignedInUser().then(
> > 
> > With this code reorganization, it now does a double fetch of the user info
> > from FxAccounts.jsm in browserid_identity, once here and once in
> > initWithLoggedInUSer(). I think it would be nice to refactor the bulk of
> > what initWithLoggedInUser does into here.
> 
> I'm not sure what you mean here.  This block was simply moved from Weave.js
> into browserid_identity.js.  There is some double-fetching of the logged in
> data, but I don't believe this patch has changed that in any way.  Can you
> please clarify?

I agree the double fetch previously existed (I was partly responsible for it), but the fetches were happening in separate files, which is slightly less egregious. Now they are happening essentially one after another in a method call. It shouldn't block this landing, but this should be cleaned up at some point. :)

Comment 16

[Chris Karlof [:ckarlof]]

I'm currently having trouble applying this patch, but I could be doing something wrong.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Chris Karlof [:ckarlof] from comment #13)

> I'm not sure if this is everything we need to do to "reset". I think we
> should do Weave.Service.startOver(), but I that does some calls to the
> server, which is problematic, but we are unable to authenticate those calls
> after the user has logged out.

For that we'd need an ONBEFORELOGOUT_NOTIFICATION which grabs a token and queues up the work. Or the ONLOGOUT_NOTIFICATION needs to come with enough handy attributes to do the job.

Comment 18

[Chris Karlof [:ckarlof]]

> For that we'd need an ONBEFORELOGOUT_NOTIFICATION which grabs a token and queues up the work. Or the ONLOGOUT_NOTIFICATION needs to come with enough handy attributes to do the job.

Warner and I have gone through similar options a while back. Nothing is awesome.

A related thing is when your password gets reset on another device, the session token on your current device is no longer valid. Now your current device can't clean up it's state without logging back in, but after you do log back in, does it appear as a new client to the Sync backend (oops) or a continuation of the old client (I hope)?

I think these are lower priority issues than getting the UX nailed down and the initial support landed.

Comment 19

[Richard Newman [:rnewman]]

(In reply to Chris Karlof [:ckarlof] from comment #18)
> does it appear as a new client to the Sync backend (oops) or a
> continuation of the old client (I hope)?

So long as the client ID doesn't change (which it shouldn't unless you nuke the world by calling startOver() on a password change), it'll be a continuation. This works right now when you fix the Sync Key or password if they're invalid.

Comment 20

[Chris Karlof [:ckarlof]]

(In reply to Richard Newman [:rnewman] from comment #19)
> (In reply to Chris Karlof [:ckarlof] from comment #18)
> > does it appear as a new client to the Sync backend (oops) or a
> > continuation of the old client (I hope)?
> 
> So long as the client ID doesn't change (which it shouldn't unless you nuke
> the world by calling startOver() on a password change), it'll be a
> continuation. This works right now when you fix the Sync Key or password if
> they're invalid.

Awesome. These are details we'll need to get right. :)

Comment 21

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

Attachment: 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 8361141 [details] [diff] [review]
> 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch
> > +    // This isn't quite sufficient here to handle all the cases. Cases
> > +    // we need to handle:
> > +    //  - User is signed in to FxAccounts, btu hasn't set up sync.
> 
> s/btu/but
> 
> File a follow-up and leave the Bug # here?

Actually, I just removed the comment as (I believe) it is now stale.  Before this refactor, this method tried to force a "sync now", and I believe the comment applied to that case - now it's just setting up the identity and leaving the "when to sync" alone.  Hopefully Chris can correct me if I'm wrong.

> @@ +105,5 @@
> > +      // Bug 958927 exists to work out what to do if that's not true.  It might
> > +      // be that the :onlogout observer does a .startOver (or maybe not - TBD)
> > +      // But for now, do nothing, and sync will just start re-synching in its
> > +      // own sweet time...
> > +      this.initializeWithCurrentIdentity();
> 
> You might also need to call one of the login methods in Service, or
> otherwise let Sync know what's going on -- as soon as it tries to sync after
> logging out, it'll enter Status.login = LOGIN_FAILED_NO_USERNAME.

That doesn't seem necessary, probably because initWithCurrentIdentity() ends up doing:

            if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
              // This makes sure that Weave.Service is loaded
              Svc.Obs.notify("weave:service:setup-complete");

I can log out of my Fxa account, then log back in, and "sync now" causes the logs to report success.

> You need to spin on this promise, or the caller of this is going to have a
> Bad Time.

Done.

Chris:  I also changed initWithLoggedInUser() to take the account data as an arg (and renamed it to initWithUser()), which (IIUC) drops that extra fetch of the account data you mentioned.

(In reply to Chris Karlof [:ckarlof] from comment #13)
> Comment on attachment 8361141 [details] [diff] [review]
> 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch
> 
> Review of attachment 8361141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/modules/browserid_identity.js
> @@ +111,5 @@
> > +
> > +    case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> > +      this.resetCredentials();
> > +      this.username = null;
> > +      this._account = null;
> 
> I'm not sure if this is everything we need to do to "reset". I think we
> should do Weave.Service.startOver(), but I that does some calls to the
> server, which is problematic, but we are unable to authenticate those calls
> after the user has logged out.

With the understanding that this patch doesn't attempt to handle a different user logging in, it seems a Weave.Service.logout() does what we need here - the log shows:

1389974598166   Sync.BrowserIDManager   INFO    Username changed. Removing stored credentials.
1389974598167   Sync.Service    INFO    Logging out
1389974598167   Sync.Status     DEBUG   Status.login: success.login => error.login.reason.no_username
1389974598167   Sync.Status     DEBUG   Status.service: success.status_ok => service.client_not_configured

which looks correct - do you concur?

(In reply to Chris Karlof [:ckarlof] from comment #16)
> I'm currently having trouble applying this patch, but I could be doing
> something wrong.

It's probably the other dependencies, which aren't yet captured in bugzilla, but https://github.com/mhammond/gecko-dev/tree/fxa-sync might help.

Thanks for the feedback - hopefully we are getting close!

Comment 22

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

Attachment: 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

Richard wrote:
> You might also need to call one of the login methods in Service, or
> otherwise let Sync know what's going on -- as soon as it tries to sync after
> logging out, it'll enter Status.login = LOGIN_FAILED_NO_USERNAME.

Unsurprisingly, you are correct :)  I'm not sure how sync *sometimes* thought things were OK here, but this new patch adds a .login() call right at the end up setting things up for the user.

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 8362790 [details] [diff] [review]
0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

::: services/sync/modules/browserid_identity.js
@@ +77,5 @@
> +        if (accountData) {
> +          // Init the identity module with any account data from
> +          // firefox accounts. The Identity module will fetch the signed in
> +          // user from fxAccounts directly.
> +          this.initWithUser(accountData).then(function () {

This seems like it warrants an error handler. After all, there are a number of steps that could fail before we successfully get account data.

If you hit those, you should enter a "there's a problem" state (even a transient one), rather than dumping and throwing, no?

I want to make sure that if we fail to get a token here, we don't show the "Set up Sync" interface in prefs!

@@ +85,5 @@
> +            // checkSetup() will check the auth state of the identity module
> +            // and records that status in Weave.Status
> +            if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
> +              // This makes sure that Weave.Service is loaded
> +              Svc.Obs.notify("weave:service:setup-complete");

login() will notify setup-complete for you in some circumstances, and throw in others:

      if (this._checkSetup() == CLIENT_NOT_CONFIGURED) {
        throw "Aborting login, client not configured.";
      }

      // Calling login() with parameters when the client was
      // previously not configured means setup was completed.
      if (initialStatus == CLIENT_NOT_CONFIGURED
          && (username || password || passphrase)) {
        Svc.Obs.notify("weave:service:setup-complete");
      }

We don't hit the latter case, because we're not specifying arguments to login(), but you should be aware of side-effects and exceptions from the login() call.

@@ +299,5 @@
> +      this._token = token;
> +      // Set the username to be the uid returned by the token server.
> +      // TODO: check here to see if the uid is different that the current
> +      // this.username. If so, we may need to reinit sync, detect if the new
> +      // user has sync set up, etc

Refer to Bug 958927.

@@ +318,5 @@
> +      // Set the clusterURI for this user based on the endpoint in the
> +      // token. This is a bit of a hack, and we should figure out a better
> +      // way of distributing it to components that need it.
> +      let clusterURI = Services.io.newURI(this._token.endpoint, null, null);
> +      clusterURI.path = "/";

See Bug 960816.

Comment 24

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

Attachment: 0007-Bug-959222-part-2-allow-a-sync-reset-to-re-enable-fx.patch

/me frantically uploads the patch :)

This is still a bit of a WIP, but should allow an "unlink device" to do the right thing when the user is using old sync.

Comment 25

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

Attachment: 0007-Bug-959222-part-2-allow-a-sync-reset-to-re-enable-fx.patch

Sorry for bugspam - this patch clears the pref *before* resetting __authManager.

Comment 26

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

Attachment: 0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

After many iterations, I'm happy with this.  It subsumes bug 959088 and bug 961017 as it makes the login process moar-async:

* The initialization of the browserid identity manager is still promise-based, but quick.  It just gets the fxAccounts logged-in user, starts a "background" fetch of the sync keys and returns.

* There is a new login failure state of LOGIN_FAILED_NOT_READY.  The bid id manager now reports this state while we are waiting for the sync keys to arrive.

So this gets around the problem we had with the promise resolving late, or not-at-all if the address was unverified - in this case we initialize OK, but the login state stays stuck at LOGIN_FAILED_NOT_READY.

If not for the fact a few tests still fail, I'd be asking for review.  This is a fairly big patch, so it would be great if you could give it a look over ASAP in the hope I'll get the test fixed soon!

Comment 27

[Chris Karlof [:ckarlof]]

Comment on attachment 8364302 [details] [diff] [review]
0005-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

::: services/sync/modules/browserid_identity.js
@@ +69,5 @@
> +    return this._shouldHaveSyncKeyBundle;
> +  },
> +
> +  initialize: function() {
> +    Services.obs.addObserver(this, fxAccountsCommon.ONVERIFIED_NOTIFICATION, false);

We need to listen to fxAccountsCommon.ONLOGIN_NOTIFICATION to handle the case that a previously verified user logs in. I believe you only receive ONVERIFIED_NOTIFICATION when a user creates her account and the verifies her email.

@@ +82,5 @@
> +    // Reset the world before we do anything async.
> +    this._shouldHaveSyncKeyBundle = false;
> +    this.username = null; // this calls resetCredentials which drops the key bundle.
> +
> +    return fxAccounts.getSignedInUser().then(accountData => {

As I mentioned above, we should wire this into ONLOGIN_NOTIFICATION, too, and fail fast if accountData.verified is false.

@@ +127,5 @@
> +    case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> +      Components.utils.import("resource://services-sync/main.js");
> +      this.username = null; // this calls resetCredentials which drops the key bundle.
> +      this._account = null;
> +      Weave.Service.logout();

This is a can of worms here, and I bet this currently doesn't do all the necessary cleanup, but it at least stops the user syncing. :)

::: services/sync/modules/service.js
@@ +651,5 @@
>    verifyLogin: function verifyLogin() {
> +    // If the identity isn't ready it  might not know the username...
> +    if (!this.identity.readyToAuthenticate) {
> +      this._log.info("Not ready to authenticate in verifyLogin.");
> +      this.status.login = LOGIN_FAILED_NOT_READY;

Where does anyone react to the LOGIN_FAILED_NOT_READY state? Is it just meant to be "none of the other states"?

::: services/sync/modules/status.js
@@ +31,5 @@
> +    let idClass = service.fxAccountsEnabled ? BrowserIDManager : IdentityManager;
> +    this.__authManager = new idClass();
> +    // .initialize returns a promise, so we need to spin until it resolves.
> +    let cb = Async.makeSpinningCallback();
> +    this.__authManager.initialize().then(cb, cb);

It looks like we need a TODO for handling errors in initialize().

Comment 28

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

Attachment: 0006-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

This has fixed most of the tests simply by disabling the ones using Fxa :(  I fear fixing this will chew up too much time and I'm relatively confident the issues are to do with the test fixtures rather than the code.  If that's OK, I'll open another bug to reenable the tests and reference the bug number where they are disabled.

Apart from that, I think this is in relatively good shape.

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8364861 [details] [diff] [review]
0006-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

f+ 'cos I think there are a couple of open issues.

::: services/sync/modules-testing/utils.js
@@ +151,5 @@
>      // do the FxAccounts thang...
>      configureFxAccountIdentity(ns.Service.identity, config);
> +    return ns.Service.identity.initializeWithCurrentIdentity().then(() => {
> +      // need to sleep until readyToAuthenticate becomes true.
> +      // XXX - any maybe until Service.status.login reflects the login??

Resolve, remove, or tidy up XXX comments -- e.g., s/any/and.

@@ +161,5 @@
> +            deferred.resolve();
> +            return;
> +          }
> +          makeTimer();
> +        }, 100, timerNamespace, "_sleepTimer");

If that's really meant to be 100msec, please leave a brief comment explaining why.

timerNamespace (and thus the timer) here is only referenced by the promise closure. That's unusual.

Also, I suspect this should be made a little less byzantine. What I think you're doing here is:

* We're done initializing with the current identity.
* I would like to continue with tests when the returned promise resolves.
* However, we can't do so until readyToAuthenticate is true.

So I think there are two more idiomatic ways to do this.

The first is: make this part of the identity. identity.whenReadyToAuthenticate.then(() => …). It can even fire immediately if it's already ready. This might be neater because you're in control of when readyToAuthenticate is assigned.

The second is: break this out into a more understandable piece of logic: a function that returns a promise which resolves when a field on an object turns into `true`. You might call that `poll`, which would look something like this (untested):

  /**
   * Return a promise which resolves when `fn` evaluates to true.
   */
  function poll(fn, interval) {
    let deferred = Promise.defer();
    function makeTimer() {
      let timer = CommonUtils.namedTimer(function onTimer() {
        if (fn()) {
          deferred.resolve();
        } else {
          makeTimer();
        }
      }, interval, deferred, "_pollTimer");
    }
    makeTimer();
    return deferred.promise;
  }

@@ +246,4 @@
>    });
>    // another task for the FxAccounts identity manager.
> +/* XXX - these are disabled for now.
> +   TODO: if I can get away with this disabling, then we need a bug# :)

This didn't start off with much in the way of testing, so walking in the wrong direction worries me.

The FxA auth process is complicated, and the integration is complicated. How confident are we that all of this works in real-world scenarios?

What are the failures?

::: services/sync/modules/browserid_identity.js
@@ +70,5 @@
> +  },
> +
> +  initialize: function() {
> +    Services.obs.addObserver(this, fxAccountsCommon.ONVERIFIED_NOTIFICATION, false);
> +    Services.obs.addObserver(this, fxAccountsCommon.ONLOGOUT_NOTIFICATION, false);

Also ONLOGIN, per ckarlof?

@@ +99,5 @@
> +        // Set the cluster data that we got from the token
> +        Weave.Service.clusterURL = this.clusterURL;
> +        this._shouldHaveSyncKeyBundle = true; // and we should actually have one...
> +        this._log.info("Background fetch for key bundle done - logging in");
> +        Weave.Service.login();

Error handling?

@@ +127,5 @@
> +    case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> +      Components.utils.import("resource://services-sync/main.js");
> +      this.username = null; // this calls resetCredentials which drops the key bundle.
> +      this._account = null;
> +      Weave.Service.logout();

Note that _shouldHaveSyncKeyBundle is also cleared by that side-effect.

@@ +308,5 @@
> +    ).then(token => {
> +      this._token = token;
> +      // Set the username to be the uid returned by the token server.
> +      // TODO: check here to see if the uid is different that the current
> +      // this.username. If so, we may need to reinit sync, detect if the new

Bug number.

@@ +327,5 @@
> +      // Set the clusterURI for this user based on the endpoint in the
> +      // token. This is a bit of a hack, and we should figure out a better
> +      // way of distributing it to components that need it.
> +      let clusterURI = Services.io.newURI(this._token.endpoint, null, null);
> +      clusterURI.path = "/";

This will break for 1.5 URLs.

Comment 30

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

Attachment: 0003-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

Thanks!

> So I think there are two more idiomatic ways to do this.
> 
> The first is: make this part of the identity.
> identity.whenReadyToAuthenticate.then(() => …). It can even fire immediately
> if it's already ready. This might be neater because you're in control of
> when readyToAuthenticate is assigned.

I went this this approach.  Only the bid_identity module supports it, and given it's just for tests, I'm comfortable with that.

> > +/* XXX - these are disabled for now.
> > +   TODO: if I can get away with this disabling, then we need a bug# :)
> 
> This didn't start off with much in the way of testing, so walking in the
> wrong direction worries me.

We made some good progress here - the tests all pass.  We made 2 main changes to get to this:

* Don't set .clusterURL, but implement our own ClusterManager.  Each identity now has a "factory" function to create the cluster manager - the one for the legacy provider is exactly the same as now.  The bid_identity cluster manager overrides _findCluster(), which just re-fetches the token and returns the cluster URL derived from that.

* No longer call Weave.Service.login() when bid_identity has finished its dance.  This actually causes some problems for our "preferences" pane, but it prevents a number of tests failing.  I'd prefer to look more into this as a followup.

> Also ONLOGIN, per ckarlof?

Yep, done.

> @@ +99,5 @@
> > +        // Set the cluster data that we got from the token
> > +        Weave.Service.clusterURL = this.clusterURL;
> > +        this._shouldHaveSyncKeyBundle = true; // and we should actually have one...
> > +        this._log.info("Background fetch for key bundle done - logging in");
> > +        Weave.Service.login();
> 
> Error handling?

Not sure what you mean here - but as above, this .login call has been removed.

> Note that _shouldHaveSyncKeyBundle is also cleared by that side-effect.

Done.

> @@ +308,5 @@
> > +    ).then(token => {
> > +      this._token = token;
> > +      // Set the username to be the uid returned by the token server.
> > +      // TODO: check here to see if the uid is different that the current
> > +      // this.username. If so, we may need to reinit sync, detect if the new
> 
> Bug number.

This comment was largely stale, but we re-tweaked the code a little so we don't set .username until after we've checked a different user hasn't signed in.

> @@ +327,5 @@
> > +      // Set the clusterURI for this user based on the endpoint in the
> > +      // token. This is a bit of a hack, and we should figure out a better
> > +      // way of distributing it to components that need it.
> > +      let clusterURI = Services.io.newURI(this._token.endpoint, null, null);
> > +      clusterURI.path = "/";
> 
> This will break for 1.5 URLs.

This code is now in the ClusterManager, but I'm not quite sure what you mean.  The path portion we truncate contains "1.1/username", and this gets tacked on again by the cluster code.  I guess you might be saying that the production tokenserver will return a "1.5" as the leading part of the path?  Or not? :)  Either way, clarification would be good.

Comment 31

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

Attachment: 0004-Bug-959222-part-2-allow-a-sync-reset-to-re-enable-fx.patch

Rebased

Comment 32

[Richard Newman [:rnewman]]

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

> * Don't set .clusterURL, but implement our own ClusterManager.  Each
> identity now has a "factory" function to create the cluster manager - the
> one for the legacy provider is exactly the same as now.  The bid_identity
> cluster manager overrides _findCluster(), which just re-fetches the token
> and returns the cluster URL derived from that.

This sounds sane to me -- matches the identity provider.

> * No longer call Weave.Service.login() when bid_identity has finished its
> dance.  This actually causes some problems for our "preferences" pane, but
> it prevents a number of tests failing.  I'd prefer to look more into this as
> a followup.

Probably a lot of tests aren't configuring their preconditions or test harness enough for a login at this stage to work, or they're going to test login themselves! Glad you found a workaround.

> This code is now in the ClusterManager, but I'm not quite sure what you
> mean.  The path portion we truncate contains "1.1/username", and this gets
> tacked on again by the cluster code.  I guess you might be saying that the
> production tokenserver will return a "1.5" as the leading part of the path? 
> Or not? :)  Either way, clarification would be good.

Yes, exactly. This is Bug 960816: you should use the cluster URL you get from the token verbatim, because it'll include path components like your storage UID (which can change) and a non-1.1 version.

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 8366432 [details] [diff] [review]
0004-Bug-959222-part-2-allow-a-sync-reset-to-re-enable-fx.patch

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

::: services/sync/modules/service.js
@@ +894,5 @@
> +    this._clusterManager = this.identity.createClusterManager(this);
> +
> +    // tell the new identity manager to initialize itself
> +    this.identity.initialize().then(() => {
> +      Svc.Obs.notify("weave:service:start-over:finish");

Can this initialization ever fail?

Comment 34

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

Landed part 1 - https://hg.mozilla.org/integration/fx-team/rev/974df2f038ae

Comment 35

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

(In reply to Richard Newman [:rnewman] from comment #33)
> Comment on attachment 8366432 [details] [diff] [review]
> 0004-Bug-959222-part-2-allow-a-sync-reset-to-re-enable-fx.patch
> 
> Review of attachment 8366432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/modules/service.js
> @@ +894,5 @@
> > +    this._clusterManager = this.identity.createClusterManager(this);
> > +
> > +    // tell the new identity manager to initialize itself
> > +    this.identity.initialize().then(() => {
> > +      Svc.Obs.notify("weave:service:start-over:finish");
> 
> Can this initialization ever fail?

In theory, no - but, for example, it will if getSignedInUser fails (which in theory will not happen, but might in the real world).  Are you suggesting a rejection handler that also sends the notification, or something else?

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/974df2f038ae

Comment 37

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

Please see comment 35

Comment 38

[Richard Newman [:rnewman]]

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

> In theory, no - but, for example, it will if getSignedInUser fails (which in
> theory will not happen, but might in the real world).  Are you suggesting a
> rejection handler that also sends the notification, or something else?

Yeah. I want to make sure we don't end up in a bad state, with an observer hanging around or something, if we know we totally failed to init.

It's always worth having an onError argument to .then().

Comment 39

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

Filed bug 965896 to cover "part 2" here.

Comment 40

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

This landed directly on 29

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8366430 [details] [diff] [review]
0003-Bug-959222-part-1-Make-browserid_identity-a-first-cl.patch

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

::: services/sync/modules/browserid_identity.js
@@ +100,5 @@
> +        this._shouldHaveSyncKeyBundle = true; // and we should actually have one...
> +        this.whenReadyToAuthenticate.resolve();
> +        this._log.info("Background fetch for key bundle done");
> +      }).then(null, err => {
> +        this._shouldHaveSyncKeyBundle = true; // but we probably don't have one...

Mark, shouldn't this have been set to false here?

Comment 42

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

(In reply to Henrik Skupin (:whimboo) from comment #41)

> Mark, shouldn't this have been set to false here?

Nope, that's correct.  _shouldHaveSyncKeyBundle is effectively flagging "am I still waiting for a sync key bundle".  In this code we are no longer waiting and don't have one - so it is an auth error state.