Closed Bug 1435929 Opened 3 years ago Closed 3 years ago

browserid_identity is confusing and error prone and should be refactored

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.
59 bytes, text/x-review-board-request
eoger
: review+
tcsc
: review+
Details 
browserid_identity had lots of hacks to make it work with the old sync. Since that landed, most of the reasons for those hacks have vanished but the hacks remain. It's very difficult to work out what happens and when.

Seeing I was responsible for many of the hacks, and I still have vague recollections about why they were made and why they are no longer relevant, I think I should take this.
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

Ed and Thom,
  This is almost ready for review, but I think I'll sit on it for a few days and see if I can think of other ways to clean it up further (eg, _ensureValidToken is now the one "entry-point" for doing the auth dance, which doesn't seem to be spelled quite correctly). But in the meantime I'd like some feedback - both on the patch I have up and any ideas you have to clean it up even further..
Attachment #8948601 - Flags: feedback?(tchiovoloni)
Attachment #8948601 - Flags: feedback?(eoger)
Assignee: nobody → markh
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review223804

This is a high level first pass at this patch.
Looks great! So happy to see initializeWithCurrentIdentity and whenReadyToAuthenticate disappear :)
In an ideal world bid_identity shouldn't know about Weave.Service/Weave.Status, it's a bit too late for that, but I think we can at least remove some calls to Weave.Service.

::: services/sync/modules/browserid_identity.js:224
(Diff revision 1)
>      // new one.
>      this._token = null;
>    },
>  
>    observe(subject, topic, data) {
> +    this._observe(subject, topic, data).catch(err => this._log.error(`Failed to observe ${topic}`, err));

We should prob use an asyncObserver

::: services/sync/modules/browserid_identity.js:269
(Diff revision 1)
> -        });
> -      }
> +    }
> -    } break;
>  
>      case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> -      Async.promiseSpinningly(Weave.Service.startOver());
> +      await Weave.Service.startOver();

I'd love to see this notification (and probably others) centralized in Service.js or policies.js. It doesn't make sense to me that bid_identity knows about Service.

::: services/sync/modules/browserid_identity.js:663
(Diff revision 1)
> + * The token server gives us the "cluster URL" (ie, the storage server this
> + * user should use.) This in turn implies that we must have a valid token
> + * before we can obtain that URL. That in-turn implies we have FxA keys.
>   */
>  
>  function BrowserIDClusterManager(service) {

Is this worth still having a ClusterManager at this point? ISTM that it gets called by Service and ends up mutating Service.clusterURL.
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review223804

> I'd love to see this notification (and probably others) centralized in Service.js or policies.js. It doesn't make sense to me that bid_identity knows about Service.

An issue with this is that Sync typically isn't loaded (or if it is, it's in a "not configured" state, so not listening for much) when we see these notifications. That does seem worth investigating though, but I think I'll do it in a followup.
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review223798

::: services/sync/modules/UIState.jsm:33
(Diff revision 1)
>    "weave:service:login:error",
>    "weave:service:ready",
>    "weave:service:sync:start",
>    "weave:service:sync:finish",
>    "weave:service:sync:error",
> +  "weave:service:start-over:finish",

FWIW, I believe this isn't strictly needed but fixes an unrelated bug. When an account is deleted on this device, a push notification is sent which immediately logs out of FxA then does service.startOver. Without this patch, we notice the logout notification but see that sync is still configured, so we enter a "please reconnect" state.

With this change we do still go into that state for a brief second, but then when the startover completes we re-query the state again and correctly see we aren't configured.
Attachment #8948601 - Flags: feedback?(tchiovoloni)
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review224566

Apologies in advance for all the nits, this is great work, but I think we should take the opportunity to go a bit further with it (maybe -- most of my nits are weakly held opinions).

Note: I didn't look very deeply into the test changes since this is just f?

::: services/sync/modules/browserid_identity.js:223
(Diff revision 2)
>      // authentication header, we will notice the lack of the token and fetch a
>      // new one.
>      this._token = null;
>    },
>  
>    observe(subject, topic, data) {

Maybe comment that we're intentionally not using asyncObserver? (If it is intentional?)

::: services/sync/modules/browserid_identity.js:264
(Diff revision 2)
> +      if (isFirstSync) {
> +        this._log.info("Doing initial sync actions");
> +        Svc.Prefs.set("firstSync", "resetClient");
> +      }
> -          Services.obs.notifyObservers(null, "weave:service:setup-complete");
> +      Services.obs.notifyObservers(null, "weave:service:setup-complete");
> -          return Async.promiseYield();
> +      await Weave.Service.sync({why: "login"});

Can you add the `didLogin` property to `TelemetryRecord.prototype.toJSON` in telemetry.js? It should be true if we know for sure it's a login sync, so if `this.why == "login"`, or something.

It's a property we documented and intended to support, but never got around to adding (partially due to how confusing this file is)

You can also file a follow up for us to do this, if you want.

::: services/sync/modules/browserid_identity.js:355
(Diff revision 2)
> -
> -  /**
>     * Deletes Sync credentials from the password manager.
>     */
>    deleteSyncCredentials() {
> -    for (let host of this._getSyncCredentialsHosts()) {
> +    for (let host of Utils.getSyncCredentialsHostsFxA()) {

This is a set of one item (`FxAccountsCommon.FXA_PWDMGR_HOST`), is there a reason we shouldn't just delete the family of functions from util.js, and just use `FxAccountsCommon.FXA_PWDMGR_HOST` as host here?

If we want to do this but not as part of this bug (it seems like passwords engine uses it too), should we file a bug for it?

::: services/sync/modules/browserid_identity.js:415
(Diff revision 2)
>    },
>  
>    /**
>     * Do we have a non-null, not yet expired token for the user currently
>     * signed in?
> +   * Used only by tests.

This isn't true, we use it in `__ensureValidToken`.

::: services/sync/modules/browserid_identity.js:455
(Diff revision 2)
>      // tokenServerURI is mis-named - convention is uri means nsISomething...
>      let tokenServerURI = this._tokenServerUrl;
>      let log = this._log;
>      let client = this._tokenServerClient;
>      let fxa = this._fxaService;
>      let userData = this._signedInUser;

While you're here, delete `userData` and and just use `this._signedInUser` directly. We immediately update `this._signedInUser` after setting `userData` in `maybeFetchKeys` (which is a bad name BTW, it always fetches the keys, no?)

Regardless confusing to `userData` as a local, especially when it's set before an exception we throw and catch (and for this kind of exception, rethrow).

Made me think that maybe it was intentionally being set before the exception, when that's not actually the case.

::: services/sync/modules/browserid_identity.js:484
(Diff revision 2)
>        const token = await client.getTokenFromBrowserIDAssertion(tokenServerURI, assertion, headers);
>        log.debug("Successfully got a sync token");
>        return token;
>      };
>  
>      let getAssertion = () => {

Nit (optional): We never call getAssertion without immediately passing the assertion to getToken. We might as well move the body of getAssertion into getToken (e.g. the first lines could be `let audience = ...; let assertion = await fxa.getAssertion(audience);` probably with the log line present as well, and it wouldn't take an argument)

Your call either way, I think it's simpler as one function but if you disagree just drop this

::: services/sync/modules/browserid_identity.js:515
(Diff revision 2)
> -      })
> +      }
> -      .then(token => {
> -        // TODO: Make it be only 80% of the duration, so refresh the token
> +      // TODO: Make it be only 80% of the duration, so refresh the token
> -        // before it actually expires. This is to avoid sync storage errors
> +      // before it actually expires. This is to avoid sync storage errors
> -        // otherwise, we get a nasty notification bar briefly. Bug 966568.
> +      // otherwise, we get a nasty notification bar briefly. Bug 966568.
> -        token.expiration = this._now() + (token.duration * 1000) * 0.80;
> +      token.expiration = this._now() + (token.duration * 1000) * 0.80;

bug 966568 is closed now that we have no more error bars. Is it safe to use 100% of the duration? If not, can you change the comment?

::: services/sync/modules/browserid_identity.js:523
(Diff revision 2)
> -        }
> +      }
> -        telemetryHelper.maybeRecordLoginState(telemetryHelper.STATES.SUCCESS);
> +      telemetryHelper.maybeRecordLoginState(telemetryHelper.STATES.SUCCESS);
> +      Weave.Status.login = LOGIN_SUCCEEDED;
> +      this._token = token;
> -        return token;
> +      return token;
> -      })
> +    } catch (caughtErr) {

Why not `catch (err)`?

::: services/sync/modules/browserid_identity.js:565
(Diff revision 2)
> +  // concepts could be decoupled, but there doesn't seem any value in that
> +  // currently.
> +  _ensureValidToken(forceNewToken = false) {
> +      // Re-use the same promise if we are already fetching one.
> +    if (!this._ensureValidTokenPromise) {
> +      this._ensureValidTokenPromise = this.__ensureValidToken(forceNewToken).finally(() => {

This is gross.

It also seems incorrect -- If you have a token already, `id._ensureValidToken(false); id._ensureValidToken(true);` will fail to force a new token. E.g. we don't force the new token if there's a promise out for a non-forced token. Consider that these might happen from disparate parts of the code (I think?)

I think the right way to do this is to move the 'guard logic' (e.g. the parts before `const notifyStateChanged`), from `__ensureValidToken` into this function (and optionally rename that one to `_updateToken` or something more accurate...).

Another option is just to await the promise (ignoring exceptions) if forceNewToken is true.

::: services/sync/modules/browserid_identity.js:603
(Diff revision 2)
> -        // UID for telemetry.
> +      // UID for telemetry.
> -        if (token) {
> +      if (token) {
> -          // We may not have a token if the master-password is locked.
> +        // We may not have a token if the master-password is locked.
> -          this._hashedUID = token.hashed_fxa_uid;
> +        this._hashedUID = token.hashed_fxa_uid;
> -        }
> +      }
> -        notifyStateChanged();
> +      notifyStateChanged();

Nit: IMO this is clearer as

```
  return token;
} finally {
  Services.obs.notifyObservers(null, "weave:service:login:change");
}
```

And then delete `const notifyStateChanged = ...`.

E.g. there's no reason to have duplicated calls to `notifyStateChanged` if we can use `finally` (well, now that Promise.prototype.finally exists there's no reason either way), and after it's not duplicated, we might as well inline it's body, since it's only obscuring what we're doing (it's not obscuring that much, admittedly).

::: services/sync/modules/browserid_identity.js:664
(Diff revision 2)
> + * user should use.) This in turn implies that we must have a valid token
> + * before we can obtain that URL. That in-turn implies we have FxA keys.
>   */
>  
>  function BrowserIDClusterManager(service) {
>    this._log = log;

Is there a reason to keep this as `this._log`? (Seems to be 'no', especially given that the code inside doesn't consistently use the one on `this`)

::: services/sync/modules/browserid_identity.js:674
(Diff revision 2)
>    /**
>     * Determine the cluster for the current user and update state.
> +   * Returns true if a new cluster URL was found and it is different from
> +   * the existing cluster URL, false otherwise.
>     */
>    async setCluster() {

TBH, since this is the main entry point of the `BrowserIDClusterManager` and instances of `BrowserIDClusterManager` don't maintain really any state, IMO we should consider just making this a function to avoid having an extra notion of a 'cluster manager' that really just boils down to a function we call to update the cluster URL.

Your call though.
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

F+ since I think the direction is the right one despite the many nits, and even as is it's a net improvement.
Attachment #8948601 - Flags: feedback?(tchiovoloni) → feedback+
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review224566

Thanks for the thorough review!

> Can you add the `didLogin` property to `TelemetryRecord.prototype.toJSON` in telemetry.js? It should be true if we know for sure it's a login sync, so if `this.why == "login"`, or something.
> 
> It's a property we documented and intended to support, but never got around to adding (partially due to how confusing this file is)
> 
> You can also file a follow up for us to do this, if you want.

I'm impressed you remember didLogin existed :) We already carry "why" in the payload, so TBH I think we might as well just drop the didLogin concept if it ends up meaning exactly `why=="login"` - WDYT? Either way, let's do whatever we decide in a followup.

> This is a set of one item (`FxAccountsCommon.FXA_PWDMGR_HOST`), is there a reason we shouldn't just delete the family of functions from util.js, and just use `FxAccountsCommon.FXA_PWDMGR_HOST` as host here?
> 
> If we want to do this but not as part of this bug (it seems like passwords engine uses it too), should we file a bug for it?

I dropped some of the now unused functions and constants, but did leave `getSyncCredentialsHosts()` because as you mentioned, the password engine uses it. But feel free to re-raise this in the review if you think we should go further.

> Why not `catch (err)`?

Because we sometimes change `err` to a different object to throw, lint complains with:

`Do not assign to the exception parameter.     no-ex-assign`

(and I changed the exception param to caughtErr as it touched less lines in the patch, but feel free to ask for another change in review if you like.)

> This is gross.
> 
> It also seems incorrect -- If you have a token already, `id._ensureValidToken(false); id._ensureValidToken(true);` will fail to force a new token. E.g. we don't force the new token if there's a promise out for a non-forced token. Consider that these might happen from disparate parts of the code (I think?)
> 
> I think the right way to do this is to move the 'guard logic' (e.g. the parts before `const notifyStateChanged`), from `__ensureValidToken` into this function (and optionally rename that one to `_updateToken` or something more accurate...).
> 
> Another option is just to await the promise (ignoring exceptions) if forceNewToken is true.

Yeah, and your suggestions are good. I didn't bother renaming the `__` version of the function name, but I think it's better now.

> TBH, since this is the main entry point of the `BrowserIDClusterManager` and instances of `BrowserIDClusterManager` don't maintain really any state, IMO we should consider just making this a function to avoid having an extra notion of a 'cluster manager' that really just boils down to a function we call to update the cluster URL.
> 
> Your call though.

I ended up putting those 2 clusterManager functions in the main identity object and killing the clusterManager concept entirely.
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review223804

> An issue with this is that Sync typically isn't loaded (or if it is, it's in a "not configured" state, so not listening for much) when we see these notifications. That does seem worth investigating though, but I think I'll do it in a followup.

I think this should be done in a followup
Priority: -- → P1
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review227634

This looks great, thanks!
Attachment #8948601 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review227728

Looks great thank you!

::: services/sync/modules/browserid_identity.js:657
(Diff revision 3)
> -      // exception - the message will appear in the logs and the error will be
> -      // treated as transient.
> -      if (!this.identity._token) {
> -        throw new Error("Can't get a cluster URL as we can't fetch keys.");
>        }
> -      let endpoint = this.identity._token.endpoint;
> +      let token = await this._ensureValidToken(forceNewToken);

IIUC this is the only method we are calling from bid_identity in the cluster finding code.
Once this patch lands we should try to do a follow-up to move the cluster code away from this file.
Attachment #8948601 - Flags: review?(eoger) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b07f7ea9d4f9e9ae10fd19c9338cff4ed52d1a6a -d 319210e2f742: rebasing 448794:b07f7ea9d4f9 "Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone. r=eoger,tcsc" (tip)
merging services/sync/modules-testing/fxa_utils.js
merging services/sync/modules-testing/utils.js
merging services/sync/modules/UIState.jsm
merging services/sync/modules/browserid_identity.js
merging services/sync/modules/constants.js
merging services/sync/modules/service.js
merging services/sync/modules/stages/enginesync.js
merging services/sync/modules/status.js
merging services/sync/modules/util.js
warning: conflicts while merging services/sync/modules/browserid_identity.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/2e7cc4dc999c
refactor browserid_identity.js to be less confusing and error prone. r=eoger,tcsc
Comment on attachment 8948601 [details]
Bug 1435929 - refactor browserid_identity.js to be less confusing and error prone.

https://reviewboard.mozilla.org/r/218018/#review228862


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/sync/modules/browserid_identity.js:168
(Diff revision 5)
> +
> +  this.asyncObserver = Async.asyncObserver(this, log);
> +  for (let topic of OBSERVER_TOPICS) {
> +    Services.obs.addObserver(this.asyncObserver, topic);
> -}
> +  }
> +};

Error: Unnecessary semicolon. [eslint: no-extra-semi]
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/de66caaad6da
refactor browserid_identity.js to be less confusing and error prone. r=eoger,tcsc
https://hg.mozilla.org/mozilla-central/rev/de66caaad6da
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1463624
You need to log in before you can comment on or make changes to this bug.