912188 - FxAccount module should use a BrowserID assertion to retrieve and persist token from Sync 2.0 token server

Status: VERIFIED INVALID

(Firefox :: Sync, defect)

Description

[Zachary Carter [:zaach]]

The Sync 2.0 token will be needed by the Sync 1.1 identity manager for authenticating storage requests. Access to the token should be synchronous, so we'll retrieve and persist it ahead of time along with other FxAccount credentials using the FxAccount module.

Comment 2

[Zachary Carter [:zaach]]

Attachment: Obtains a Sync 2.0 token using the BrowserID assertion from the jelly

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 800291 [details] [diff] [review]
Obtains a Sync 2.0 token using the BrowserID assertion from the jelly

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

General feedback: I don't like the direction of the account credential bundle having a top-level field fetched from the Sync token server. If this is strictly throwaway code, then maybe that's tolerable, but even so I'd appreciate a little bit more structure here.

It seems like what you're wanting to do is associate some kind of per-service info with an account; your hack is to include that per-service fetching/storage/etc. into the root account credential bundle.

My feeling is that this should instead live in the IdentityManager replacement: that thing being the adapter layer between a service-agnostic Firefox Account and the authentication work needed to talk to a Sync server.

The token doesn't even need to be persisted. The FxAccountIdentity object can do something as simple as:

  // Wrapped by the getRESTRequestAuthenticator etc. methods.
  myTokenAuthenticator: function (request) {
    if (!this.hasValidToken()) {
      this.fetchToken();          // Uses the info in the Account bundle to hit the token server.
    }
    request.addHeader(this.computeSomeHeader(this.token));
  },

The FxA code stays "pure", transient tokens don't end up hitting the disk (we fetch once on app launch), and Sync-specific identity code lives with the rest of the Sync-specific identity code.

::: browser/app/profile/firefox.js
@@ +1297,5 @@
>  // The URL where remote content that composes the UI for Firefox Accounts should
>  // be fetched.
>  pref("firefox.accounts.remoteUrl", "http://accounts.dev.lcip.org/flow");
> +
> +// The URL of the Sync token server

Closing period.

@@ +1298,5 @@
>  // be fetched.
>  pref("firefox.accounts.remoteUrl", "http://accounts.dev.lcip.org/flow");
> +
> +// The URL of the Sync token server
> +pref("firefox.accounts.tokenServerUrl", "http://auth.oldsync.dev.lcip.org");

Is this a stable value? It doesn't look like it.

::: services/fxaccounts/accounts.jsm
@@ +27,5 @@
>     *        {
>     *          email: The users email address
>     *          sessionToken: Session for the FxA server
>     *          assertion: A Persona assertion used to enable Sync
> +   *          syncToken: (optional) A Sync 2.0 token. A new one is fetched if omitted.

This seems like a really big crack in sane abstractions. That's OK if this is demo code, but it doesn't look like demo code.

@@ +89,4 @@
>    },
> +
> +  /**
> +   * Sign the current user out

Copypasta.

@@ +96,5 @@
> +   */
> +  getSyncToken: function getSyncToken() {
> +    return this.getSignedInUser().then(
> +      function(user) {
> +        return user ? user.syncToken : null;

A different light on the same crack. Presumably this is the case if a user isn't provisioned for Sync?

@@ +125,5 @@
> +    let deferred = Promise.defer();
> +    let client = new TokenServerClient();
> +    let uri = this._getTokenServerURI() + "/1.0/sync/1.1";
> +    client.getTokenFromBrowserIDAssertion(uri, assertion, function (err, token) {
> +      if (err) return deferred.reject(err);

Nit: always brace and use multi-line conditionals.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +2,5 @@
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);

No scope needed.

@@ +51,1 @@
>    run_next_test();

I think…

yield server.stop(run_next_test);

Comment 4

[Chris Karlof [:ckarlof]]

Richard, you're absolutely right that the sync token shouldn't be handled by the FxA service and it shouldn't be stored persistently. Some thoughts:

0) As a meta point, I'd prefer to sprint to a fully functional Milestone 1 as quickly as we can, while tracking any shortcuts we took, so we can fix them later.

1) Handling these tokens properly is one such detail I'd like to defer. Tokens should be short-lived and probably not stored persistently on the client. This requires refresh logic and error handling logic. During dev, expiring tokens will be annoying. Making them long-lived and stored persistently on the client is a way to shortcut these for now while we connect all the pieces.

2) We're still getting our heads around/accepting this spin-the-event-loop/pretend-async-is-sync-stuff. For example,

>  // Wrapped by the getRESTRequestAuthenticator etc. methods.
>  myTokenAuthenticator: function (request) {
>    if (!this.hasValidToken()) {
>      this.fetchToken();          // Uses the info in the Account bundle to hit the token server.
>    }
>    request.addHeader(this.computeSomeHeader(this.token));
>  },

looks reasonable, but what's your suggestion for addressing the fact that this.fetchToken() is async, but these authenticators are assumed to be sync? Our "hack" to shove it in the FxA service for now was to avoid adding new "spin the event loop" nonsense. Is your opinion that having a temporary leaky abstraction is worse than adding "new spin the event loop" calls? Is there an alternative solution?

Comment 5

[Sam Penrose]

    [This comment and my reply are re: 911378, but staying here for continuity's sake.]

    > ... The FxAccountIdentity object
    > can do something as simple as:
    > 
    >   // Wrapped by the getRESTRequestAuthenticator etc. methods.
    >   myTokenAuthenticator: function (request) {
    >     if (!this.hasValidToken()) {
    >       this.fetchToken();          // Uses the info in the Account bundle to
    > hit the token server.
    >     }
    >     request.addHeader(this.computeSomeHeader(this.token));
    >   },
    > 
    > The FxA code stays "pure", transient tokens don't end up hitting the disk
    > (we fetch once on app launch), and Sync-specific identity code lives with
    > the rest of the Sync-specific identity code.

    The longer I stare at the last 50 lines of identity.js, the more I want to change all the function names. The only client for the "get*Authenticator" family appears to be two methods from services.js in the same directory, one which takes a request and one which takes a resource. The family returns event handlers whose jobs are:

      for a request, set an authorization header
      for a resource, return a headers object {headers: {authorization: <value>}}

    <ignoramus suggestion>Ideally both would return the object, and the service.js method for the request (getStorageRequest) would wrap that in setHeader(). Really ideally we'd just skip the get*Authenticator methods, and have service.js bind the Manager event handlers directly; they could be null or return null as appropriate -- the latter makes more sense to me as it allows authorization setup after the handlers are bound but before they're called. Presumably this violates the change-as-little-as-possible dictum.</ignoramus suggestion>

    Failing that, It seems to me the names should be something like:

      getResourceAuthenticator
        _onLoadResourceGetAuthenticationHeader
        (better: _getResourceAuthenticationHeader)
      getRequestAuthenticator
         _onSendRequestGetAuthenticationHeader
        (better: _getRequestAuthenticationHeader)

    How much of this is fair game for renaming?

Comment 6

[Richard Newman [:rnewman]]

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

> Tokens should be short-lived and probably not stored persistently on the
> client. This requires refresh logic and error handling logic. During dev,
> expiring tokens will be annoying. Making them long-lived and stored
> persistently on the client is a way to shortcut these for now while we
> connect all the pieces.

And making them long-lived and *not* stored on the client is even better, no? If you don't store them, restarting the browser (or just always fetching a new token at the start of a sync) is your refresh logic.

(But the rudimentary refresh logic would be "abort sync and drop token", which for an in-memory field is trivial. This is in line with what we do now for auth failures.)

I think you might be over-estimating the cost of doing this logic in the right place. That's totally expected; I have a longer perspective on how all this stuff works together, and a lot of it is messy and resists clarity!


> looks reasonable, but what's your suggestion for addressing the fact that
> this.fetchToken() is async, but these authenticators are assumed to be sync?

fetchToken would be synchronous (or wrapped to make it so). At some point it ends up hitting the wire, which is already async-wrapped-in-spinning.


> Our "hack" to shove it in the FxA service for now was to avoid adding new
> "spin the event loop" nonsense. Is your opinion that having a temporary
> leaky abstraction is worse than adding "new spin the event loop" calls? Is
> there an alternative solution?

We already spin the event loop every time we make an HTTP request in a sync. Spinning to make token fetching synchronous doesn't worry me, and I'd much rather put crap like that in the IdentityManager layer (which is throwaway) than the FxA service (which is not).

One alternative is to add a "prep the identity manager" step in sync startup. This identity manager would fetch tokens at that point. That reduces the authenticator work to ~= what the basic auth one does: create a header for a request.


(In reply to Sam Penrose from comment #5)

>     How much of this is fair game for renaming?

If you do it in a Part 0 (i.e., a pure-rename patch; rename things and fix tests accordingly), I'm pretty open to changes here. The important part is that this still make sense for Sync 1.1, and doesn't regress anything -- we could land it right now on m-c. And that's what we'd do!

Comment 7

[Chris Karlof [:ckarlof]]

> fetchToken would be synchronous (or wrapped to make it so). At some point it ends up hitting the wire, which is already async-wrapped-in-spinning.

I may be missing something, but the existing TokenServerClient has a (thankfully) async interface, so it seems to me we'd have to convert this to a sync interface via spinning if we continue to use it.

Comment 8

[Chris Karlof [:ckarlof]]

> I think you might be over-estimating the cost of doing this logic in the right place. That's totally expected; I have a longer perspective on how all this stuff works together, and a lot of it is messy and resists clarity!

identity.js was the obvious place for managing the token, but we decided to move into the FxAccount manager as a shortcut for now. We'll try moving it back to our identity component.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Chris Karlof [:ckarlof] from comment #7)
> > fetchToken would be synchronous (or wrapped to make it so). At some point it ends up hitting the wire, which is already async-wrapped-in-spinning.
> 
> I may be missing something, but the existing TokenServerClient has a
> (thankfully) async interface, so it seems to me we'd have to convert this to
> a sync interface via spinning if we continue to use it.

Yes, and fortunately that's easy to do. E.g., this fetch-over-HTTP-and-do-something method is called at the start of each sync:

https://hg.mozilla.org/mozilla-central/file/default/services/sync/modules/service.js#l508

resource.get() wraps AsyncResource.get().

In this case, wherever we grab a token (either on first use or when prompted during sync startup) the identity manager just includes the three lines of code to spin until the grab succeeds or fails.

Comment 10

[Sam Penrose]

(In reply to Richard Newman [:rnewman] from comment #6)
> (In reply to Sam Penrose from comment #5)
> 
> >     How much of this is fair game for renaming?
> 
> If you do it in a Part 0 (i.e., a pure-rename patch; rename things and fix
> tests accordingly), I'm pretty open to changes here. The important part is
> that this still make sense for Sync 1.1, and doesn't regress anything -- we
> could land it right now on m-c. And that's what we'd do!

Darn that Richard, calling my bluff! Of the four methods I suggest renaming, one is not a rename (getResourceAuthenticator) and two are closures with unexposed names (_onR*). The remaining method (getRESTRequestAuthenticator) doesn't seem worth a patch that would drop a single term ("REST"). So for 911378 / FxAccountsManager I'll keep the two public names, drop getBasicResourceAuthenticator, and rename the callbacks (unless you don't want me to).

A refactor patch that changed the contract between service.js and IdentityManager per "ignoramus suggestion" above would seem worth it to make the API cleaner going forward, but I don't feel the need to push for it and therefore will leave IdentityManager untouched.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #10)

> Darn that Richard, calling my bluff!

:D


> The remaining method (getRESTRequestAuthenticator)
> doesn't seem worth a patch that would drop a single term ("REST"). 

Note that the "REST" actually isn't redundant! `getRESTRequestAuthenticator` gets an authenticator for a `RESTRequest` instance, just as `getResourceAuthenticator` gets an authenticator for a `Resource` instance...

https://mxr.mozilla.org/services-central/source/services-central/services/common/rest.js#83


> 911378 / FxAccountsManager I'll keep the two public names, drop
> getBasicResourceAuthenticator,

Note that this is used as a helper in two tests. You'll need to reword those tests.

https://mxr.mozilla.org/services-central/search?string=getBasicResourceAuthenticator&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=services-central


> A refactor patch that changed the contract between service.js and
> IdentityManager per "ignoramus suggestion" above would seem worth it to make
> the API cleaner going forward, but I don't feel the need to push for it and
> therefore will leave IdentityManager untouched.

You can totally do this, too (particularly an API unification, so long as you hit all of the users of the Resource-centric version); but again, do it in a separate part, and make sure that you keep the tests working.

N.B., if part of your refactor basically boils down to "I'm going to grab methods in a class and bind on them rather than asking the class to set them up for me", then I'd gently suggest keeping the encapsulation. Working code (with encapsulation) beats a scattershot patch of small changes to make something slightly simpler, and more so if the changes violate encapsulation.

Comment 12

[Sam Penrose]

(In reply to Richard Newman [:rnewman] from comment #11)

Thanks very much for the thoughtful feedback. Forgoing all proposed changes, except that when I said "drop getBasicResourceAuthenticator" I meant "not include it in FxAccountsManager." Unless we need it for testing that, of course.

Comment 13

[Chris Karlof [:ckarlof]]

This approach was abandoned in favor of bug 911378.

Comment 14

[James Bonacci [:jbonacci]]

OK.