967015 - Sync datatype selections remembered from previous Firefox desktop user

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Ryan Feeley [:rfeeley]]

When a subsequent Firefox user signs into sync, the sync datatype selections of the previous user are retained; they should be reset as if this is a completely new person with a new browser.

Comment 1

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

Bug 951373 may affect this.

Comment 2

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

I guess we should explicitly reset the prefs on a new sign-in. As I understand it your account selections should eventually sync across even if we don't, but I don't fully understand how this stuff works...

Comment 3

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

Attachment: 0002-Bug-967015-arrange-for-a-Service.startOver-when-a-di.patch

This relatively simple patch arranges for Service.startOver() to be called when we detect a different user has signed in.  We explicitly do not (IIUC) want to perform a startOver() on every login or logout - only when a new user signs in.

This arranges for the existing about:accounts code that detects a new user to arrange for an observer notification to be sent, which the Sync Service responds to.  startOver() resets *all* prefs, including logging related prefs, which is probably a PITA for developers, but seems like the right thing to be doing.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8378793 [details] [diff] [review]
0002-Bug-967015-arrange-for-a-Service.startOver-when-a-di.patch

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

This will probably work, but it doesn't make me feel good.

Not starting over every time every user signs out is a bit of a hack, and might have unpleasant consequences. For example, try this:

* Sign out.
* Create a new bookmark.
* Sign in again, and wait for a sync to complete.
* Sync another device.

Does that new bookmark appear on the other device?

If not, you might need to sprinkle some resetClient fairy dust around the codebase, which is layering on more complexity. Simply always calling startOver on sign out will address this bug without additional fairy dust.


Another thing that worries me: you're now calling startOver immediately before doing other stuff. Some of startOver's effects don't occur until a subsequent event loop tick, so in a very real sense we'll still be starting over -- including initializing the identity manager -- while we're doing that post-setup work.

So: this worries me, and if you can find a better way, I'd be happy. But if you can't, and you're confident in this approach (and have banged on it hard to see if it breaks), then OK.

Comment 5

[Chris Karlof [:ckarlof]]

Comment on attachment 8378793 [details] [diff] [review]
0002-Bug-967015-arrange-for-a-Service.startOver-when-a-di.patch

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

:markh and I discussed over IRC, and here is the proposal:

1) Call startOver on each logout as rnewman suggests
2) Add startOver() to unlinkFirefoxAccount in browser/components/preferences/sync.js before the call to fxAccounts.signOut(). This is not awesome because we have a whole observer system for notifying listeners that the user signed out. However, sync makes authenticated calls to the sync server during logout, which requires the user to be logged, which suggests some beforeLogout thing, which we're not going to iron out before Fx29. However, "Disconnect" is currently in Sync UI so this isn't so terrible of an abstraction violation. :)
3) Remove out ONLOGOUT listener in browserid_identity.js
4) Make sure startOver() triggers everything that out ONLOGOUT listener previously did. 

Since startOver does a whole event loop spinning thing, and we have no guarantee how quickly it will finish, this raises another issue since the UI state might not update until some time later. :) 

An alternative is to implement a onBeforeLogout notification, but I don't like the complexity of that in this timeframe.

It also seems mean to give a f-, but I guess what this is. ;)

Comment 6

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

Attachment: 0002-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

This patch calls Service.startOver() on logout.  Of note:

* Given .startOver() creates a new identity manager, browserid_identity has formalized this concept - a single instance ensures it is only ever used with a single fxaccount identity.  fxAccounts.getSignedInUser() is only called once and the result saved in the identity manager - this shouldn't be too controversial as we already store all other authentication info (eg, token, syncKeyBundle) in the instance.  In addition, this means the right-thing happens when the ONLOGOUT_NOTIFICATION is fired - we already have the previously logged in user so can ensure the .startOver() call works as expected (eg, remove the client ID from the server).

An alternative discussed by Chris and I was to ensure all our entry-points into "disconnect" do the startOver *before* we do the logout(), but this seems undesirable - it's a short-term solution which would still eventually need the right thing to happen if a logout happens any other way.

* It keeps the optimization for using the token from bug 973749 - we try and reuse the existing token when possible, and if we need to fetch a new one we save it for next time.

* It introduces a promise-based 'finalize()' method to the identity objects.  Neither identity actually *needs* it to be promise-based, but it is consistent with .initialize().

* .startOver() now drops all sync credentials *after* the engine.removeClientData() calls, otherwise we fail to drop the client data with 401s.  rnewman - I'm flagging you for feedback particularly on this change - it works fine for me in my tests (and in the tests themselves), but you might have insights into something I don't!

* .startOver() also fixes a bug I introduced in the "reset sync should enable FxA" bug - the new identity had .initialize() called twice.

* I've disabled a test in test_browserid_identity.js - now that it's impossible for a single browserid_identity object to be used with multiple identities, the isTokenValid function no longer checks the email address is the same as what it was.  ckarlof, does it sound OK to you to remove that specific test?

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8380470 [details] [diff] [review]
0002-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

::: services/sync/modules/browserid_identity.js
@@ +43,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, 'OBSERVER_TOPICS', function() {
> +  return [
> +    fxAccountsCommon.ONLOGIN_NOTIFICATION,

This seems unnecessary; why not

  const OBSERVER_TOPICS = [
    fxAcc...
  ];

?

@@ +115,5 @@
>      return this.initializeWithCurrentIdentity();
>    },
>  
> +  finalize: function() {
> +    // After this is called, we can expect Service.identity != this.

Also blow away our fields?

@@ +226,5 @@
> +      // authentication info, which should cause auth errors everywhere that's
> +      // still alive. Note however it is not a *real* user- initiated logout,
> +      // so we don't drop the username or log the user out from FxA - the
> +      // ONLOGOUT_NOTIFICATION will handle things if that is the case.
> +      this.resetCredentials();

Might cause Bug 976149? Need to double-check when logout is called.

@@ +508,5 @@
>     */
>    _getAuthenticationHeader: function(httpObject, method) {
> +    let cb = Async.makeSpinningCallback();
> +    this._ensureValidToken().then(
> +      () => {

ensureValidToken becomes a fetch, which can throw. Could you make sure that _getAuthenticationHeader does the right thing in that case? (I expect: returns null)

::: services/sync/modules/service.js
@@ +870,5 @@
>  
> +    // We want let UI consumers of the following notification know as soon as
> +    // possible, so let's fake for the CLIENT_NOT_CONFIGURED status for now
> +    // by emptying the passphrase (we still need the password).
> +    this.identity.resetSyncKey();

Log here.

::: services/sync/tests/unit/test_browserid_identity.js
@@ +265,3 @@
>      identityConfig.fxaccount.user.email = "something@new";
>      do_check_false(bidUser.hasValidToken());
> +    **/

Call updateSignedInUser with a bogus bundle?

Comment 8

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

Attachment: 0002-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

Thanks!

(In reply to Richard Newman [:rnewman] from comment #7)
> Comment on attachment 8380470 [details] [diff] [review]
> 0002-Bug-967015-Have-bid_identity-call-Service.startOver-.patch
> 
> Review of attachment 8380470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/modules/browserid_identity.js
> @@ +43,5 @@
> >  });
> >  
> > +XPCOMUtils.defineLazyGetter(this, 'OBSERVER_TOPICS', function() {
> > +  return [
> > +    fxAccountsCommon.ONLOGIN_NOTIFICATION,
> 
> This seems unnecessary; why not
> 
>   const OBSERVER_TOPICS = [
>     fxAcc...
>   ];

Because fxAccountCommon is itself lazy - just having a top-level const will defeat this.  I'm open to the idea that fxAccountsCommon being lazy is a premature optimization, but for now I just added a comment indicating why OBSERVER_TOPICS is also lazy.  FWIW, fxAccountsCommon is lazy as it will only be used once an instance is instantiated - but that's going to be very soon after module import, hence my openness to it being a pointless optimization :)

> Also blow away our fields?

Good idea - done.

> @@ +226,5 @@
> > +      // authentication info, which should cause auth errors everywhere that's
> > +      // still alive. Note however it is not a *real* user- initiated logout,
> > +      // so we don't drop the username or log the user out from FxA - the
> > +      // ONLOGOUT_NOTIFICATION will handle things if that is the case.
> > +      this.resetCredentials();
> 
> Might cause Bug 976149? Need to double-check when logout is called.

I don't think this is the reason for bug 976149 - but bug 976149 will (correctly) hit this block after it has seen the failure to fetch the token.  The underlying issue in that bug is the token fetch failure (which I predict is time skew issues)

> @@ +508,5 @@
> >     */
> >    _getAuthenticationHeader: function(httpObject, method) {
> > +    let cb = Async.makeSpinningCallback();
> > +    this._ensureValidToken().then(
> > +      () => {
> 
> ensureValidToken becomes a fetch, which can throw. Could you make sure that
> _getAuthenticationHeader does the right thing in that case? (I expect:
> returns null)

The fetch happens in a promise, and the final .then(null, err ...) handler will pick that up.  However, it will (IIUC) cause the .wait call to fail given the error does cb(null, err) - so I've added an exception handler around the .wait and return null.  I think that covers this.

> ::: services/sync/modules/service.js
> @@ +870,5 @@
> >  
> > +    // We want let UI consumers of the following notification know as soon as
> > +    // possible, so let's fake for the CLIENT_NOT_CONFIGURED status for now
> > +    // by emptying the passphrase (we still need the password).
> > +    this.identity.resetSyncKey();
> 
> Log here.

It now reads:

    // by emptying the passphrase (we still need the password).
    this._log.trace("Service.startOver dropping sync key and logging out.");
    this.identity.resetSyncKey();

is that what you had in mind?

> ::: services/sync/tests/unit/test_browserid_identity.js
> @@ +265,3 @@
> >      identityConfig.fxaccount.user.email = "something@new";
> >      do_check_false(bidUser.hasValidToken());
> > +    **/
> 
> Call updateSignedInUser with a bogus bundle?

Sadly that isn't enough to invalidate the token - with this patch, the token validity is based only on the fact we have one, and it is still current.  Even before this patch a bogus bundle didn't invalidate the token - only a different user signing in does, which this test simulated.  So I'm still inclined to think this test can die - there is already a test for token expiry.

Only flagging Chris this time, but more feedback from rnewman is (as always!) welcome.

Comment 9

[Richard Newman [:rnewman]]

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

> Because fxAccountCommon is itself lazy - just having a top-level const will
> defeat this.

Unless you separate the constants out… worth considering.


> > Log here.
> 
> It now reads:
> 
>     // by emptying the passphrase (we still need the password).
>     this._log.trace("Service.startOver dropping sync key and logging out.");
>     this.identity.resetSyncKey();
> 
> is that what you had in mind?

Yeah, but I'd go for info, not trace!

Comment 10

[Chris Karlof [:ckarlof]]

Mark, to followup on our conversation today, another issue to consider is the "timeliness" of logout. One could argue that logout should be instant from a user's perspective, and any cleanup tasks should take place in the background. The approach we discussed today may imply that logout doesn't finish until a few network calls complete, which in the case of a bad connection, may take some time. I'll think about this more tomorrow.

Comment 11

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

I've discovered another problem with this patch - we don't drop the client data if you disconnect before the identity manager has fully initialized.  Eg:

* Start Fx already configured.
* Open prefs pane - identity starts initializing.
* Click disconnect before the auth dance is complete.

The process of dropping the client info relies on the service having a .storageURL, but it doesn't know this until after the identity has completed initializing (the storage URL is based on the userBaseURL, and we don't know this until we get a token).

Sadly, the only thing I can come up with (other than storing more of this in prefs, which sounds the wrong direction) is to have .startOver() spinningly block until identity.readyToAuthenticate becomes true - which is yet more of a delay which Chris is concerned about in comment 10.

Thoughts?

Comment 12

[Richard Newman [:rnewman]]

I suggest that it's fine to fail to delete server data if you manage to hit that button too quickly. After all, it can already fail due to network issues etc.

Indeed, it seems backward to have "clear my state" block on successful setup operations. But maybe not, if readyToAuthenticate is small enough in scope (i.e., no network operations).

Comment 13

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

(In reply to Richard Newman [:rnewman] from comment #12)
> I suggest that it's fine to fail to delete server data if you manage to hit
> that button too quickly. After all, it can already fail due to network
> issues etc.

What are the real-world implications of that?  My fear is that this device will hang around forever in, eg, "Tabs from other devices".

> Indeed, it seems backward to have "clear my state" block on successful setup
> operations.

Somewhat - although it isn't *that* surprising that credentials are necessary for that.

> But maybe not, if readyToAuthenticate is small enough in scope
> (i.e., no network operations).

Sadly it's not particularly small, as witnessed by the fact we can hit this way :(  It involves a couple of network trips (or maybe just 1 in a best case - some of this is still fuzzy in my head)

OTOH though, the fetching of auth stuff has certainly *started* by this time - just possibly not yet completed.  It wouldn't concern me much if this was happening in the background from the POV of the user (ie, after the UI etc has reflected a logged out state).  The main problem I see if when we need to wait on this before having the UI etc reflect the logout.

Comment 14

[Richard Newman [:rnewman]]

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

> What are the real-world implications of that?  My fear is that this device
> will hang around forever in, eg, "Tabs from other devices".

Theoretically so. Fortunately all of our per-client data -- namely tabs and the client record itself -- are TTLed, so this will only be wrong for a few weeks.

What we don't want to do is near-permanently store your credentials so we can keep trying to delete, periodically making a ping to your old storage server.


> OTOH though, the fetching of auth stuff has certainly *started* by this time
> - just possibly not yet completed.  It wouldn't concern me much if this was
> happening in the background from the POV of the user (ie, after the UI etc
> has reflected a logged out state).  The main problem I see if when we need
> to wait on this before having the UI etc reflect the logout.

Aye.

Comment 15

[Chris Karlof [:ckarlof]]

Mark, I'm thinking that for the scope of Fx29, we just do best effort to make sure the network calls to clean up server data succeed and if they fail, then so be it. I don't think we should make this more complicated that we need it to be to solve the major problem (making sure local state gets reset on logout), and we also shouldn't make logout "block" on remote network calls to the server to clean up.

Comment 16

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

Attachment: 0001-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

(In reply to Chris Karlof [:ckarlof] from comment #15)
> Mark, I'm thinking that for the scope of Fx29, we just do best effort to
> make sure the network calls to clean up server data succeed and if they
> fail, then so be it.

Chatting with Chris on IRC, this means we are staying with the original patch - keep enough state so we can *usually* clear the server data directly on the logout notification - we will only fail if (a) we haven't yet logged in or (b) we have logged in but the token has expired.

Addressed previous round of comments, and requesting review from rnewman on the change to service.js and ckarlof on the rest.

Comment 17

[Chris Karlof [:ckarlof]]

Comment on attachment 8383509 [details] [diff] [review]
0001-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

That's for pushing this through Mark. I've made some comments to try to unblock you, but I'd like to try to run this myself before signing off. I had trouble applying before, but I'll try again on my Monday (tomorrow) when I get back in the office.

::: services/sync/modules/browserid_identity.js
@@ +189,5 @@
> +    // This object should only ever be used for a single user.  It is an
> +    // error to update the data if the user changes (but updates are still
> +    // necessary, as each call may add more attributes to the user).
> +    // We start with no user, so an initial update is always ok.
> +    if (this._signedInUser && this._signedInUser.email != userData.email) {

When we move to using a first class user object, I hope we can just set the user in this module and its state can just evolve rather than this module needing to update this JSON blob manually every time. I'm concerned about the fragility here. E.g., we should check that all callers of this do something reasonable if this throws.

@@ +577,4 @@
>    _findCluster: function() {
> +    if (!this._fxaIdentityValid) {
> +      log.error("requesting cluster before authentication is ready");
> +      throw new Error("requesting cluster before authentication is ready");

We should make sure that callers handle this error properly.

::: services/sync/modules/service.js
@@ +869,5 @@
>      }
>  
> +    // We want let UI consumers of the following notification know as soon as
> +    // possible, so let's fake for the CLIENT_NOT_CONFIGURED status for now
> +    // by emptying the passphrase (we still need the password).

Moving all this code here may prevent the UI from changing to the logout state until the above server calls complete. However, this may only be an issue for legacy sync, because in browser/components/preferences/sync.js we do:

    fxAccounts.signOut().then(() => {
      this.updateWeavePrefs();
    });

on FxA signout. updateWeavePrefs tries to fetch user data, which is now empty and shows the logged out state.

@@ +901,5 @@
>        Svc.Obs.notify("weave:service:start-over:finish");
>        return;
>      }
>  
> +    this.identity.finalize().then(

Is it ok for this async code to be in this legacy sync function that callers can expect to run synchronously?

Comment 18

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

Attachment: 0001-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

(In reply to Chris Karlof [:ckarlof] from comment #17)
> I
> had trouble applying before, but I'll try again on my Monday (tomorrow) when
> I get back in the office.

Not sure why that would be, but this is updated against what will (hopefully) be in m-c tomorrow.

> ::: services/sync/modules/browserid_identity.js
> @@ +189,5 @@
> > +    // This object should only ever be used for a single user.  It is an
> > +    // error to update the data if the user changes (but updates are still
> > +    // necessary, as each call may add more attributes to the user).
> > +    // We start with no user, so an initial update is always ok.
> > +    if (this._signedInUser && this._signedInUser.email != userData.email) {
> 
> When we move to using a first class user object, I hope we can just set the
> user in this module and its state can just evolve rather than this module
> needing to update this JSON blob manually every time.

Agreed.

> I'm concerned about
> the fragility here. E.g., we should check that all callers of this do
> something reasonable if this throws.

Agreed in general - although in most cases we've just replaced an existing similar check with this function call.

> @@ +577,4 @@
> >    _findCluster: function() {
> > +    if (!this._fxaIdentityValid) {
> > +      log.error("requesting cluster before authentication is ready");
> > +      throw new Error("requesting cluster before authentication is ready");
> 
> We should make sure that callers handle this error properly.

This is actually better seen as an "assertion" - if we hit this we've screwed up somewhere.  IOW, we really don't expect to see it - maybe a comment along these lines would make this clearer?  I guess there is a very small risk that another user has logged back in before we've managed to complete the .startOver(), but the implementation of _findCluster on the legacy provider also throws, so I think this is ok.

IOW, I'm pretty sure sync will just fail with lots of smoke and noise in the logs.

> ::: services/sync/modules/service.js
> @@ +869,5 @@
> >      }
> >  
> > +    // We want let UI consumers of the following notification know as soon as
> > +    // possible, so let's fake for the CLIENT_NOT_CONFIGURED status for now
> > +    // by emptying the passphrase (we still need the password).
> 
> Moving all this code here may prevent the UI from changing to the logout
> state until the above server calls complete. However, this may only be an
> issue for legacy sync, because in browser/components/preferences/sync.js we
> do:
> 
>     fxAccounts.signOut().then(() => {
>       this.updateWeavePrefs();
>     });
> 
> on FxA signout. updateWeavePrefs tries to fetch user data, which is now
> empty and shows the logged out state.

Yeah, good catch.  It probably also wouldn't hurt to add FxAccountsCommon.ONLOGOUT_NOTIFICATION to the topics sync.js listens for - in this case it might mean .updateWeavePrefs is called multiple times on logout, but that doesn't worry me too much and it would be more robust.  Thoughts?

> 
> @@ +901,5 @@
> >        Svc.Obs.notify("weave:service:start-over:finish");
> >        return;
> >      }
> >  
> > +    this.identity.finalize().then(
> 
> Is it ok for this async code to be in this legacy sync function that callers
> can expect to run synchronously?

It should be, given that the legacy version of this function does nothing, and before this patch we were still doing something async - specifically:

-    // Tell the new identity manager to initialize itself
-    this.identity.initialize().then(() => {


But as I mentioned a few comments ago, it turns out the .initialize() was being done twice.  So I think this is fine.

Thanks!

Comment 19

[Chris Karlof [:ckarlof]]

Comment on attachment 8384425 [details] [diff] [review]
0001-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

::: services/sync/modules/browserid_identity.js
@@ +235,5 @@
> +      // authentication info, which should cause auth errors everywhere that's
> +      // still alive. Note however it is not a *real* user- initiated logout,
> +      // so we don't drop the username or log the user out from FxA - the
> +      // ONLOGOUT_NOTIFICATION will handle things if that is the case.
> +      this.resetCredentials();

Not sure if we can do this here vs just clearing the token. This handler here was to address 401s when interacting with the storage server, which can happen routinely if the token expires. Just clearing the token here was at the time a quick+dirty way to get the client to refresh its token with the token server.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8384425 [details] [diff] [review]
0001-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

::: services/sync/modules/browserid_identity.js
@@ +522,5 @@
>        }
> +    ).then(null,
> +      err => {
> +        cb(null, err);
> +      });

Either you got the argument order wrong here, or you got it wrong in the patch I just reviewed and down on line 612 in this patch...

@@ +575,5 @@
>  
>  BrowserIDClusterManager.prototype = {
>    __proto__: ClusterManager.prototype,
>  
> +  get _fxaIdentityValid() {

...IsValid

Comment 21

[Chris Karlof [:ckarlof]]

Comment on attachment 8384425 [details] [diff] [review]
0001-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

::: services/sync/modules/browserid_identity.js
@@ +42,5 @@
> +
> +const OBSERVER_TOPICS = [
> +  fxAccountsCommon.ONLOGIN_NOTIFICATION,
> +  fxAccountsCommon.ONLOGOUT_NOTIFICATION,
> +  "weave:service:logout:finish",

As discussed below, I think the observer for weave:service:logout:finish can be removed.

@@ +133,5 @@
>      Components.utils.import("resource://services-sync/main.js");
>  
>      // Reset the world before we do anything async.
>      this.whenReadyToAuthenticate = Promise.defer();
> +    this.username = null; // this also drops all auth info.

As we discussed, we shouldn't clear the username as a routine thing. Instead we should:
1) remove our overridden getters and setters for the account property in browserid_identity
2) add
  if (!this.account) {
    this.account = accountData.email;
  }
after we call getSignedInUser(). This will as side effect set the username as a Pref, which will enable Sync to do the right thing in future. Note that !this.account should also only be true during the initial login.

 instead relier on this.account (which as a side effect sets this.username). We also discussed overriding this.usernameFromAccount(val) to just return val.

@@ +235,5 @@
> +      // authentication info, which should cause auth errors everywhere that's
> +      // still alive. Note however it is not a *real* user- initiated logout,
> +      // so we don't drop the username or log the user out from FxA - the
> +      // ONLOGOUT_NOTIFICATION will handle things if that is the case.
> +      this.resetCredentials();

As we discussed, we should not listen to this notification anymore. Instead, we decided to change Service.logout() to call identity.logout(), which will only clear this._token. logout() on the legacy identity module should do nothing. The next time Sync runs, it will then notice it is missing a token and get a new one.

@@ +365,5 @@
>     * This essentially validates that enough credentials are available to use
>     * Sync.
>     */
>    get currentAuthState() {
> +    if (!this._shouldHaveSyncKeyBundle) {

When this client really isn't configured, I wonder if this addition is going to do the right thing.

Comment 22

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

Attachment: 0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

NOTE: this is on-top of the patch in bug 977502 - there will be conflicts otherwise.

All previous feedback addressed, but there were still significant changes Chris and I discussed, so worth another feedback round.

Comment 23

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

Attachment: 0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

An error in currentAuthState snuck in.

Comment 24

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

Attachment: 0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

Some fixes Chris and I discussed on IRC:

* In service.js, _updateCachedURLs no longer checks auth state at all.
* In browserid_identity, getUserBaseURL no longer checks auth state at all.
* In browserid_identity, _findCluster no longer checks auth state, just readyToAuthenticate.

Comment 25

[Chris Karlof [:ckarlof]]

Comment on attachment 8386503 [details] [diff] [review]
0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

Looks good and I did a fair bit of manual testing.

Comment 26

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

Comment on attachment 8386503 [details] [diff] [review]
0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

Let's keep this moving then, but still hoping to get review from both of you

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 8386503 [details] [diff] [review]
0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

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

Not the most thorough review of my life, but I think this has had enough eyes.

::: services/sync/modules/browserid_identity.js
@@ +438,5 @@
>        });
>      });
>    },
>  
> +  // Refresh the sync token for our.

for our sentence.

@@ +528,4 @@
>      }
> +    return this._fetchTokenForUser().then(
> +      token => {
> +        this._token = token

Nit: trailing ';'.

@@ +551,5 @@
>    _getAuthenticationHeader: function(httpObject, method) {
> +    let cb = Async.makeSpinningCallback();
> +    this._ensureValidToken().then(
> +      () => {
> +        cb();

Just 'cb'.

@@ +556,4 @@
>        }
> +    ).then(null,
> +      err => {
> +        cb(err);

… cb.

@@ +614,5 @@
> +    // This check is really more of an assertion to check we aren't called
> +    // while we are still doing an auth dance.
> +    if (!this.identity.readyToAuthenticate) {
> +      log.error("requesting cluster before authentication is ready");
> +      throw new Error("requesting cluster before authentication is ready");

This seems potentially risky. Any reason not to chain off whenReadyToAuthenticate?

@@ +638,5 @@
>      let cb = Async.makeSpinningCallback();
>      promiseClusterURL().then(function (clusterURL) {
> +      cb(null, clusterURL);
> +    }).then(null, err => {
> +      cb(err);

cb.

::: services/sync/modules/identity.js
@@ +93,5 @@
>      return Promise.resolve();
>    },
>  
> +  finalize: function() {
> +    // nothing to do for this identity provider

Nit: sentence.

Comment 28

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

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/409750b6699d

Comment 30

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

Comment on attachment 8386503 [details] [diff] [review]
0003-Bug-967015-Have-bid_identity-call-Service.startOver-.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fxa Sync
User impact if declined: Incorrect error state handling.
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Note to sheriffs or whoever lands these: These patches may conflict unless they are landed in the order of: Bug 977502, Bug 967015 and Bug 969528

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcc1a3b9741d