911378 - Create BrowserID sync identity manager that retrieves a Sync 2.0 token for the currently logged in user and uses it to generate Hawk headers for storage requests

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Sam Penrose]

+++ This bug was initially created as a clone of Bug #907420 +++

For 1.1, Identity Manager uses HTTP Basic Auth and needs only the corresponding username/password pair. For FxA, it needs to send a Sync 2.0 token via Hawk. We will bind to the FxAccount Service and delegate token retrieval to it. For Hawk, we will add an _onResourceRequestHawk() method, and conditional blocks to the appropriate get*Authenticator() methods that bind it when we are using FxA.

Comment 1

[Sam Penrose]

Attachment: id_man_2_fxa20130903.patch

A simple sketch of the interface between IdentityManager and FxAccount Service. Includes a simplest-possible setter for creating the connection, which assumes that instantiation of an FxAccount object has been done externally.

Comment 2

[Zachary Carter [:zaach]]

Comment on attachment 798905 [details] [diff] [review]
id_man_2_fxa20130903.patch

>diff -r 8666b5e7e9d8 services/sync/modules/identity.js
>--- a/services/sync/modules/identity.js	Fri Aug 16 14:23:05 2013 -0700
>+++ b/services/sync/modules/identity.js	Tue Sep 03 09:03:32 2013 -0700

>@@ -478,11 +489,24 @@
>   },
> 
>   /**
>+   * FXAccount Hawk setup.
>+   */
>+  _onResourceRequestHawk: function _onResourceRequestHawk(resource) {

We may need to pass in `method`, like in the resourceRequestMAC signature

>+    if (this.fxaService == null) {
>+      return null;
>+    }
>+    let hawkHeader = this.fxaService.getHawkHeader(resource);

... because getHackHeader needs to know the request `method`.

Comment 3

[Sam Penrose]

Comment on attachment 798905 [details] [diff] [review]
id_man_2_fxa20130903.patch

diff -r 8666b5e7e9d8 services/sync/modules/identity.js
--- a/services/sync/modules/identity.js	Fri Aug 16 14:23:05 2013 -0700
+++ b/services/sync/modules/identity.js	Tue Sep 03 13:41:25 2013 -0700
@@ -67,6 +67,7 @@
   this._syncKeyAllowLookup = true;
   this._syncKeySet = false;
   this._syncKeyBundle = null;
+  this._fxaService = null;
 }
 IdentityManager.prototype = {
   _log: null,
@@ -108,6 +109,14 @@
     this.username = this.usernameFromAccount(value);
   },
 
+  get fxaService() {
+    return this._fxaService;
+  },
+
+  set fxaService(service) {
+    this._fxaService = service;
+  },
+
   get username() {
     return Svc.Prefs.get("username", null);
   },
@@ -446,6 +455,8 @@
   getResourceAuthenticator: function getResourceAuthenticator() {
     if (this.hasBasicCredentials()) {
       return this._onResourceRequestBasic.bind(this);
+    } else if (this.fxaService != null) {
+      return this._onResourceRequestHawk.bind(this);
     }
 
     return null;
@@ -478,11 +489,24 @@
   },
 
   /**
+   * FXAccount Hawk setup.
+   */
+  _onResourceRequestHawk: function _onResourceRequestHawk(resource, method) {
+    if (this.fxaService == null) {
+      return null;
+    }
+    let hawkHeader = this.fxaService.getHawkHeader(resource, method);
+    return {headers: {authorization: hawkHeader}};
+  },
+
+  /**
    * Obtain a function to be used for adding auth to RESTRequest instances.
    */
   getRESTRequestAuthenticator: function getRESTRequestAuthenticator() {
     if (this.hasBasicCredentials()) {
       return this.onRESTRequestBasic.bind(this);
+    } else if (this.fxaService != null) {
+      return this._onResourceRequestHawk.bind(this);
     }
 
     return null;

Comment 4

[Sam Penrose]

Apologies for the comment chaos above; I was attempting to change the patch to reflect your feedback.

Comment 5

[Chris Karlof [:ckarlof]]

> +    let hawkHeader = this.fxaService.getHawkHeader(resource, method);

Having the FxAccounts server compute the header seems wrong. It should probably return a token. I apologize I had previously suggested this interface.

Comment 6

[Chris Karlof [:ckarlof]]

>@@ -478,11 +489,24 @@
>   },
> 
>   /**
>+  _onResourceRequestHawk: function _onResourceRequestHawk(resource, method) {
>+    if (this.fxaService == null) {
>+      return null;
>+    }
>+    let hawkHeader = this.fxaService.getHawkHeader(resource, method);
>+    return {headers: {authorization: hawkHeader}};
>+  },

I think the structure of this should closely resemble _onResourceRequestMAC (calling out to a Util function to compute the HAWK header), except that it first fetches the token for the currently logged in user from the FxAccount service.

Comment 7

[Sam Penrose]

The latest. Please let me know if there is a cleaner way to update patches.

diff -r 8666b5e7e9d8 services/sync/modules/identity.js
--- a/services/sync/modules/identity.js	Fri Aug 16 14:23:05 2013 -0700
+++ b/services/sync/modules/identity.js	Tue Sep 03 16:03:39 2013 -0700
@@ -67,6 +67,7 @@
   this._syncKeyAllowLookup = true;
   this._syncKeySet = false;
   this._syncKeyBundle = null;
+  this._fxaService = null;
 }
 IdentityManager.prototype = {
   _log: null,
@@ -108,6 +109,14 @@
     this.username = this.usernameFromAccount(value);
   },
 
+  get fxaService() {
+    return this._fxaService;
+  },
+
+  set fxaService(service) {
+    this._fxaService = service;
+  },
+
   get username() {
     return Svc.Prefs.get("username", null);
   },
@@ -446,6 +455,8 @@
   getResourceAuthenticator: function getResourceAuthenticator() {
     if (this.hasBasicCredentials()) {
       return this._onResourceRequestBasic.bind(this);
+    } else if (this.fxaService != null) {
+      return this._onResourceRequestHawk.bind(this);
     }
 
     return null;
@@ -478,11 +489,26 @@
   },
 
   /**
+   * FXAccount Hawk setup.
+   */
+  _onResourceRequestHawk: function _onResourceRequestHawk(resource, method) {
+    if (this.fxaService == null) {
+      return null;
+    }
+    let storageServiceToken = this.fxaService.getToken(resource, method);
+    let hawkHeader = Utils.computeHawkHeader(resource, storageServiceToken, method,
+                                             'XXX get correct parameters');
+    return {headers: {authorization: hawkHeader}};
+  },
+
+  /**
    * Obtain a function to be used for adding auth to RESTRequest instances.
    */
   getRESTRequestAuthenticator: function getRESTRequestAuthenticator() {
     if (this.hasBasicCredentials()) {
       return this.onRESTRequestBasic.bind(this);
+    } else if (this.fxaService != null) {
+      return this._onResourceRequestHawk.bind(this);
     }
 
     return null;

Comment 8

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #7)
> The latest. Please let me know if there is a cleaner way to update patches.

1. Update it locally (hg qref, for example).
2. "Add an attachment".
3. Name it accordingly: "Part 1. v3"
4. Click the checkbox named "Obsolete" next to the old version of the same patch.
5. If you're obsoleting a patch that's already marked for f? or r?, transfer the flags to the new patch as you upload. The obsoleting will remove the flags from the old patch for you.

Comment 9

[Sam Penrose]

Attachment: id_man_2_fxa20130903.2.patch

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 799615 [details] [diff] [review]
id_man_2_fxa20130903.2.patch

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

::: services/sync/modules/identity.js
@@ +455,5 @@
>    getResourceAuthenticator: function getResourceAuthenticator() {
>      if (this.hasBasicCredentials()) {
>        return this._onResourceRequestBasic.bind(this);
> +    } else if (this.fxaService != null) {
> +      return this._onResourceRequestHawk.bind(this);

My feeling is that hooking in here (the failure case!) is going to cause more trouble than it's worth, for two reasons:

* If you manage to make the first conditional fail -- to make this class no longer think that it has Sync credentials -- you're going to have to screw around with all of the username etc. getters. You'll be doing OO-via-if-then.

* We might as well move in the direction of being less intrusive to the existing 1.1 codebase, which will help when we have to maintain both in parallel.

So how about instead:

* Optional, for clarity: rename IdentityManager to SyncIdentityManager.
* Create a new class (possibly with a shared prototype if there's logic to reuse): FxAccountIdentity.
* Adjust status.js, `new IdentityManager()`, to be less dumb -- look for a Firefox Account, and otherwise fall back to the old Sync credentials, creating the right instance as appropriate.

That'll give you a nice neat place to address open questions, like "what exactly is my Sync username in the FxA world?", or "when someone asks for my Sync Key, what do I return?".

That is: IdentityManager is your point of extension, but do it by reimplementing that interface rather than chopping up the existing code.

Comment 11

[Chris Karlof [:ckarlof]]

> That is: IdentityManager is your point of extension, but do it by reimplementing that interface rather than chopping up the existing code.

Let's try Richard's suggestion.

To start with, let's ignore supporting existing sync, so in status.js, let's just do 

  _authManager: new FxAccountIdentityManager(),

I'd also use the existing IdentityManager prototype as a starting point for the FxAccountIdentityManager.

We can then create a bug for fixing status.js to switch between the two depending on the circumstances. That logic will probably be subtle.

Comment 12

[Richard Newman [:rnewman]]

+1

Comment 13

[Sam Penrose]

Here's a quick-and-dirty grab of the existing IdentityManager API. I skipped methods beginning with "_".

function IdentityManager()
  account get/set
  username get/set
  basicPassword get/set
  syncKey get/set
  syncKeyBundle get
  currentAuthState get
  persistCredentials
  deleteSyncKey
  hasBasicCredentials
  deleteSyncCredentials
  usernameFromAccount
  getResourceAuthenticator // I assume we need just one

This looks insufficiently generic to me (note all the "basic"), but maybe we need some of that to avoid outraging our consumers. Any opinions on whether to cut it down, and if so how far?

Comment 14

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #13)

> This looks insufficiently generic to me (note all the "basic"), but maybe we
> need some of that to avoid outraging our consumers. Any opinions on whether
> to cut it down, and if so how far?

For M1, most of these can be ignored (stubbed out) or map quite easily.

"hasBasicCredentials" really means "hasCredentials" -- as opposed to when persistent storage doesn't have anything to use.

"basicPassword" is deceptive; it's only named that to distinguish it from what used to be called "passphrase".

"account" and "username" will be the email address and hashed address. That maps 1:1.

"syncKey" will be derived from the account provider.

"persistCredentials" is thorny; this is a touchpoint from password change UI. "deleteSyncCredentials" is for when we disassociate the account, so we can ignore that for now.

Comment 15

[Sam Penrose]

(In reply to Richard Newman [:rnewman] from comment #14)
> "hasBasicCredentials" really means "hasCredentials" -- as opposed to when
> persistent storage doesn't have anything to use.

And it is only used in this module. Looks like I can call it "_isAuthenticated" or "_canBeAuthenicated".

> "syncKey" will be derived from the account provider.

I believe this means "syncKey does not need to be part of the public interface."

Comment 16

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #15)

> I believe this means "syncKey does not need to be part of the public
> interface."

It does, for two reasons:

* Existing UI code will expect it to return something valid.
* Its sibling method, syncKeyBundle, returns the keys that are the root of the crypto chain. This is where you need to hook in kB.

Comment 17

[Sam Penrose]

Attachment: 911378.3.patch

Based on IdentityManager.prototype. Follows Hawk API from https://bugzilla.mozilla.org/show_bug.cgi?id=911384 .

Comment 18

[Sam Penrose]

Attachment: Depends on 909967 and 911384 patches. Not yet executed.

Per previous; fixed handling of computeHawk return value.

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 802700 [details] [diff] [review]
911378.3.patch

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

Just a quick drive-by.

N.B., when you've still got a bunch of XXX comments and such, you probably want f? not r?.

::: services/sync/modules/fxaidentity.js
@@ +27,5 @@
> +  this._syncKeyBundle = null;
> +
> +}
> +FxAccountsManager.prototype = IdentityManager.prototype;
> +FxAccountsManager.prototype._fxaService = null;

Ordinarily we just do this:

this.FxAccountsManager = function FxAccountsManager() {
}

this.FxAccountsManager.prototype = Object.freeze({
  __proto__: IdentityManager.prototype,

  _token: null,

  hasValidToken: function () {
    return this.token…
  },
  …
});

@@ +31,5 @@
> +FxAccountsManager.prototype._fxaService = null;
> +FxAccountsManager.prototype._token = null;
> +
> +FxAccountsManager.prototype.hasValidToken = function hasValidToken() {
> +    return this._token && XXXsomeTest(this._token) & true;

Nit: two-space indents.

@@ +39,5 @@
> +FxAccountsManager.prototype.hasBasicCredentials = FxAccountsManager.prototype.hasValidToken;
> +
> +FxAccountsManager.prototype.fetchToken = function fetchToken(assertion) {
> +  // XXX Read this from somewhere.
> +  let tokenServerHost = "http://auth.oldsync.dev.lcip.org";

Grep for "dataSubmissionPolicyMode" in

https://bug888052.bugzilla.mozilla.org/attachment.cgi?id=801753

for an idea of how you might do this.

@@ +75,5 @@
> +/**
> + * @return a Hawk HTTP Authorization Header, lightly wrapped, for the .uri
> + * of a RESTRequest or AsyncResponse object.
> + */
> +FxAccountsManager.prototype._getAuthenticationHeader = function _getAuthenticationHeader(httpObject, method) {

You don't need the function names. They'll be pulled from the prototype when required to print stacks.

Comment 20

[Sam Penrose]

Attachment: 911378.5.patch

Incorporate Richard's latest feedback. I suspect the prefs/constants handling of the tokenserver url is not what he had in mind. Also need to know how to test the token for validity.

Comment 21

[Sam Penrose]

I'm trying to decide what to test. I think the current tests are not a good model, and I could use some feedback.

1) Chris has observed that FxAccountsManager should notice when the token has expired and refresh it, and also that it should wrap TokenServerClient to do so. The docstring for TSC lists duration as the 5th property of its return value. The test for it does not include the string "duration". Instead it drops that property to the point that it tests to make sure that the return value has exactly 4 keys.

2) IdentityManager provides the get*Authenticator methods to service.js in the same directory, which assigns their return values to the ".authenticator" attribute of resource and request objects. Those methods are not evaluated in test_identity_manager.js. It looks like service.js has a bunch of tests, none of which include the string "authenticator". There are some tests for resource.js and rest.js which test that the attribute exists and can be set, but they don't appear to evaluate the functionality in IdentityManager.

I believe these areas should be among the first I write tests for. Thoughts?

Comment 22

[Chris Karlof [:ckarlof]]

(In reply to Sam Penrose from comment #21)

> 1) Chris has observed that FxAccountsManager should notice when the token
> has expired and refresh it, and also that it should wrap TokenServerClient
> to do so. The docstring for TSC lists duration as the 5th property of its
> return value. The test for it does not include the string "duration".
> Instead it drops that property to the point that it tests to make sure that
> the return value has exactly 4 keys.

You could fix the test in test_tokenserverclient.js to account for the duration. It appears the existing TokenServerClient handles it. I would think that the tests for your wrapped TokenServerClient would be very similar to what is already already in test_tokenserverclient.js minus the Async.makeSpinningCallback() stuff. And I would definitely fix your tests to account for duration.
 
> 2) IdentityManager provides the get*Authenticator methods to service.js in
> the same directory, which assigns their return values to the
> ".authenticator" attribute of resource and request objects. Those methods
> are not evaluated in test_identity_manager.js. It looks like service.js has
> a bunch of tests, none of which include the string "authenticator". There
> are some tests for resource.js and rest.js which test that the attribute
> exists and can be set, but they don't appear to evaluate the functionality
> in IdentityManager.

Yes, we should have tests to make sure the headers are being set. Not sure why those aren't tested in the existing IdentityManager.

Comment 23

[Sam Penrose]

(In reply to Chris Karlof [:ckarlof] from comment #22)
> (In reply to Sam Penrose from comment #21)
...
> You could fix the test in test_tokenserverclient.js to account for the
> duration. It appears the existing TokenServerClient handles it. I would
> think that the tests for your wrapped TokenServerClient would be very
> similar to what is already already in test_tokenserverclient.js minus the
> Async.makeSpinningCallback() stuff. And I would definitely fix your tests to
> account for duration.

OK. Can you tell me a bit more WRT "tests for your wrapped TokenServerClient would be very similar to what is already in test_tokenserverclient.js"? I think of FxAccountsManager (the object I'm writing/exporting) as a *consumer* of TSC, not e.g. a subclass, and that module has a lot of tests which don't apply to FAM's interfaces or for that matter code. I had planned to test the functionality provided here; e.g. when a token expires, automatically refresh. I started down the rathole of looking at the TSC tests because I wanted to see what expected values for duration I should handle (e.g. "300" vs 300 vs 300.0 null vs undefined, that sort of thing).

Comment 24

[Chris Karlof [:ckarlof]]

(In reply to Sam Penrose from comment #23)
> OK. Can you tell me a bit more WRT "tests for your wrapped TokenServerClient
> would be very similar to what is already in test_tokenserverclient.js"? I
> think of FxAccountsManager (the object I'm writing/exporting) as a
> *consumer* of TSC, not e.g. a subclass, and that module has a lot of tests
> which don't apply to FAM's interfaces or for that matter code. I had planned
> to test the functionality provided here; e.g. when a token expires,
> automatically refresh. I started down the rathole of looking at the TSC
> tests because I wanted to see what expected values for duration I should
> handle (e.g. "300" vs 300 vs 300.0 null vs undefined, that sort of thing).

I had previously assumed that you were writing an object that wrapped TSC to have a sync interface. But the tests for such an object are kind of dumb because it's just a pass through proxy. If you want to skip that object (like it sounds you were planning to do), then the wrapping can happen directly in the FAM where the TSC is invoked. Then you'd just have the unit tests on the FAM. Make sense?

Comment 25

[Sam Penrose]

Yes, sounds good.

Comment 26

[Sam Penrose]

The get*Authenticator methods in identity.js return null rather than an authenticator if the IdentityManager does not have credentials at the moment they are called. This seems to me a vector for false negatives (if credentials are set after get*Authenticator is called and before its return value would have been called). Since the return value does and must check for null credentials at the time it is called, I would like to remove the null checks, which would mean that the get*Authenticators simply bind and return the appropriate method.

Comment 27

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #26)

> This seems to me a vector for false negatives (if
> credentials are set after get*Authenticator is called and before its return
> value would have been called).

That shouldn't happen: get*Authenticator is called afresh for each new Resource or RESTRequest, and credentials shouldn't be changing half-way through the executing of a request.

And if they do, the worst thing that will happen is a 401, resulting in an aborted sync.

Mostly the omission of error handling here is due to the low likelihood of error. I'm not averse to improving matters there (particularly if it simplifies matters without introducing regressions), but it's not really a big deal.

Comment 28

[Richard Newman [:rnewman]]

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

> Yes, we should have tests to make sure the headers are being set. Not sure
> why those aren't tested in the existing IdentityManager.

Labs code -- you shoulda seen it before! :D

Comment 29

[Sam Penrose]

WRT Async.makeSpinningCallback and errors; at Async.js ll. 83-4 you can see where it throws errors rather than silently discarding them:

  http://mxr.mozilla.org/mozilla-central/source/services/common/async.js#83

Comment 30

[Sam Penrose]

WRT "how do the consumers of IdentityManager authenticator functions handle errors when accessing the storage layer?", there is no explicity error handling in identity.js, rest.js, or resource.js.

Comment 31

[Sam Penrose]

Attachment: 911378.09_26.patch

A complete rethink. Subclass IdentityManager and manage almost all state in FxAccounts at header construction time. Open issues:

 1) Should IdentityManager.account be set to the FxA .email or .uid?
 2) _normalizeAccountValue should be moved to identity.js
 3) Where do we read tokenServerURI from?

Comment 32

[Chris Karlof [:ckarlof]]

(In reply to Sam Penrose from comment #31)

>  1) Should IdentityManager.account be set to the FxA .email or .uid?

Since the current Sync 1.1 code is accustomed to having an email in there, let's stick with that for now.

>  3) Where do we read tokenServerURI from?

This URI should probably be retrieved from a pref. Check in with Zach if you need information on how to do this. Longer term, we're going to build a service discovery service, but it still might end up putting the value into a pref.

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 810903 [details] [diff] [review]
911378.09_26.patch

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

Looking good!

::: services/sync/Makefile.in
@@ +17,5 @@
>  # The set of core JavaScript modules for Sync. These are copied as-is.
>  sync_modules := \
>    addonsreconciler.js \
>    addonutils.js \
> +  browserid_identity.js \

Please also add the new module(s) you've created to test_load_modules.js.

::: services/sync/modules/browserid_identity.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// "use strict";

*eyebrows* :D

@@ +23,5 @@
> + */
> +
> +this.BrowserIDManager = function BrowserIDManager(fxaService, tokenServerClient) {
> +  this._fxaService = fxaService || new FxAccounts();
> +  this._tokenServerClient = tokenServerClient || new TokenServerClient();

It looks like you don't need the constructor fallbacks, or at least the first one.

@@ +25,5 @@
> +this.BrowserIDManager = function BrowserIDManager(fxaService, tokenServerClient) {
> +  this._fxaService = fxaService || new FxAccounts();
> +  this._tokenServerClient = tokenServerClient || new TokenServerClient();
> +  this._log = Log4Moz.repository.getLogger("Sync.Identity");
> +  this._log.Level = Log4Moz.Level[Svc.Prefs.get("log.logger.identity")];

Not a review comment, but a word of warning: because you're not installing a pref observer here, you'll need to re-instantiate this class for changes to this pref to take effect.

@@ +49,5 @@
> +  _normalizeAccountValue: function(value) {
> +    return value.toLowerCase();
> +  },
> +
> +  hasValidToken: function() {

Comment that callers are responsible for cleaning up invalid tokens by calling _clearUserData.

@@ +50,5 @@
> +    return value.toLowerCase();
> +  },
> +
> +  hasValidToken: function() {
> +    if (this._token === null) {

if (!this._token) {

... because presumably you want to catch other falsy values (e.g., `undefined`), too.

@@ +53,5 @@
> +  hasValidToken: function() {
> +    if (this._token === null) {
> +      return false;
> +    }
> +    if (this._token.expiration < new Date()) {

See comment below about Date.now().

@@ +61,5 @@
> +    if (!signedInUser) {
> +      return false;
> +    }
> +    // Does the signed in user match the user we retrieved the token for?
> +    if (this._normalizeAccountValue(signedInUser.email) !== this.account) {

Perhaps this can be better expressed by having a way to ask the FxA module if this user (this.account) is (still) signed in? Might be more efficient. Your call.

@@ +71,5 @@
> +  /**
> +   * Wrap and synchronize FxAccounts.getSignedInUser().
> +   *
> +   * @return credentials
> +   *         with fields email, uid, sessionToken, assertion, kA, and kB.

Comment-rot risk: this depends on the contents of _tokenServerClient.getTokenFromBrowserIDAssertion, rather than being 'owned' by this method.

@@ +75,5 @@
> +   *         with fields email, uid, sessionToken, assertion, kA, and kB.
> +   */
> +  _getSignedInUser: function() {
> +    let userBlob;
> +    let blockingCall = Async.makeSpinningCallback();

cb (see later comment).

@@ +93,5 @@
> +    }
> +    return userBlob;
> +  },
> +
> +  _getTokenForUser: function(user) {

I'd name this `fetchTokenForUser`, because it's not an accessor.

I'd also suggest using a default param for the token server URI (function(user, uri="http://...")). That'll give you some testability here.

@@ +95,5 @@
> +  },
> +
> +  _getTokenForUser: function(user) {
> +    let token;
> +    let blockingCall = Async.makeSpinningCallback();

We conventionally call this 'cb'.

@@ +100,5 @@
> +    let tokenServerURI = "http://auth.oldsync.dev.lcip.org/1.0/sync/1.1";
> +
> +    try {
> +      this._tokenServerClient.getTokenFromBrowserIDAssertion(
> +        tokenServerURI, user.assertion, blockingCall);

Is it possible for assertion to be undefined here?

@@ +107,5 @@
> +      this._log.info("TokenServerClient.getTokenFromBrowserIDAssertion() failed with: " + err.api_endpoint);
> +      return null;
> +    }
> +
> +    let expiration = new Date();

You might consider using `Date.now()` instead of `new Date()`: no need to allocate an object, and fewer confusing automatic coercions when doing comparisons. Just

  token.expiration = Date.now() + token.duration * 1000;
  return token;

@@ +108,5 @@
> +      return null;
> +    }
> +
> +    let expiration = new Date();
> +    expiration.setTime(expiration.valueOf() + token.duration * 1000);

(Alternatively, `let expiration = new Date(Date.now() + token.duration * 1000)` as one line, rather than using setTime.)

@@ +132,5 @@
> +      this._token = this._getTokenForUser(user);
> +      if (!this._token) {
> +        return null;
> +      }
> +      this.account = this._normalizeAccountValue(user.email);

Can we break out 126-137 (or 127-136) as a separate method? This is a pretty core chunk of logic: "prepareTokenForSignedInUser", optionally by checking whether we already have one for the current signed in user. I could imagine us wanting to call this speculatively, rather than during the first HTTP request.

@@ +135,5 @@
> +      }
> +      this.account = this._normalizeAccountValue(user.email);
> +    }
> +    let credentials = {key: this._token, id: this.username,
> +                       algorithm: "sha256"};

New line per field, plz, ideally sorted.

@@ +150,5 @@
> +  _addAuthenticationHeader: function(request, method) {
> +    let header = this._getAuthenticationHeader(request, method);
> +    if (header === null) {
> +      return null;
> +    }

Again, 

  if (!header) {
    return null;
  }

::: services/sync/tests/unit/test_browserid_identity.js
@@ +5,5 @@
> +Cu.import("resource://services-sync/rest.js");
> +Cu.import("resource://gre/modules/Promise.jsm");
> +
> +let mockUser = {email: 'email', uid: 'user_uid', assertion: 'assertion',
> +                sessionToken: 'sessionToken', kA: 'kA', kB: 'kB'};

Newline after this, and ideally one field per line, sorted, with trailing comma.

@@ +6,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +
> +let mockUser = {email: 'email', uid: 'user_uid', assertion: 'assertion',
> +                sessionToken: 'sessionToken', kA: 'kA', kB: 'kB'};
> +let _MockFXA = function(blob) { // FxAccounts

function _MockFXA(blob) {
    ...
  }
  _MockFXA.prototype = {
    __proto__: FxAccounts,
  }

? Not sure to what extent you want to be totally mocking vs 'subclassing' here.

@@ +23,5 @@
> +                 uid: "token_uid", duration: 300};
> +let mockTSC = { // TokenServerClient
> +  getTokenFromBrowserIDAssertion: function(uri, assertion, cb) {
> +    cb(null, mockToken);
> +  }

Trailing comma.

@@ +33,5 @@
> +  initTestLogging("Trace");
> +  Log4Moz.repository.getLogger("Sync.Identity").level = Log4Moz.Level.Trace;
> +  run_next_test();
> +  _("Verify initial state");
> +  do_check_eq(browseridManager._token, null);

You shouldn't do this after run_next_test(); move these to a separate test.

@@ +41,5 @@
> +
> +add_test(function test_getResourceAuthenticator() {
> +    _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header.");
> +    let authenticator = browseridManager.getResourceAuthenticator();
> +    do_check_neq(authenticator, null);

do_check_true(!!authenticator);

seems to be the dominant paradigm for this.

@@ +48,5 @@
> +               method: 'GET'};
> +    let output = authenticator(req, 'GET');
> +    do_check_true('headers' in output);
> +    do_check_true('authorization' in output.headers);
> +    do_check_eq('Hawk', output.headers.authorization.slice(0, 4));

String.startsWith()?

@@ +65,5 @@
> +    do_check_neq(authenticator, null);
> +    let output = authenticator(request, 'GET');
> +    do_check_eq(request.uri, output.uri);
> +    do_check_eq(output._headers.authorization.slice(0, 4), 'Hawk');
> +    do_check_true(output._headers.authorization.indexOf('nonce') > -1);

String.contains()

@@ +78,5 @@
> +      getTokenFromBrowserIDAssertion: function(uri, assertion, cb) {
> +        cb(null, {id: "id", key: "key",
> +            // XXX read from prefs or constant
> +            api_endpoint: "http://auth.oldsync.dev.lcip.org/1.0/sync/1.1",
> +            uid: "token_uid", duration: -10});

Should we actually be checking for invalid durations like this?

Comment 34

[Sam Penrose]

Richard: thanks very much! Responses inline; in a half-dozen cases I am receiving essentially opposed guidance from you and Chris; I have marked those "Karlof-Newman" and will defer to the two of you. Waiting to upload revised patch for your responses.

(In reply to Richard Newman [:rnewman] from comment #33)
> Comment on attachment 810903 [details] [diff] [review]
> 911378.09_26.patch
> 
> Review of attachment 810903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> ::: services/sync/Makefile.in
> @@ +17,5 @@
> >  # The set of core JavaScript modules for Sync. These are copied as-is.
> >  sync_modules := \
> >    addonsreconciler.js \
> >    addonutils.js \
> > +  browserid_identity.js \
> 
> Please also add the new module(s) you've created to test_load_modules.js.

Done.

> ::: services/sync/modules/browserid_identity.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// "use strict";
> 
> *eyebrows* :D

Fixed.
 
> @@ +23,5 @@
> > + */
> > +
> > +this.BrowserIDManager = function BrowserIDManager(fxaService, tokenServerClient) {
> > +  this._fxaService = fxaService || new FxAccounts();
> > +  this._tokenServerClient = tokenServerClient || new TokenServerClient();
> 
> It looks like you don't need the constructor fallbacks, or at least the
> first one.

I need it to accept a mock from the unit test. Karlof-Newman.

> @@ +25,5 @@
> > +this.BrowserIDManager = function BrowserIDManager(fxaService, tokenServerClient) {
> > +  this._fxaService = fxaService || new FxAccounts();
> > +  this._tokenServerClient = tokenServerClient || new TokenServerClient();
> > +  this._log = Log4Moz.repository.getLogger("Sync.Identity");
> > +  this._log.Level = Log4Moz.Level[Svc.Prefs.get("log.logger.identity")];
> 
> Not a review comment, but a word of warning: because you're not installing a
> pref observer here, you'll need to re-instantiate this class for changes to
> this pref to take effect.

I did not take action here. :ckarlof : want me to install a pref observer?

> @@ +49,5 @@
> > +  _normalizeAccountValue: function(value) {
> > +    return value.toLowerCase();
> > +  },
> > +
> > +  hasValidToken: function() {
> 
> Comment that callers are responsible for cleaning up invalid tokens by
> calling _clearUserData.

Done.

> 
> @@ +50,5 @@
> > +    return value.toLowerCase();
> > +  },
> > +
> > +  hasValidToken: function() {
> > +    if (this._token === null) {
> 
> if (!this._token) {
> 
> ... because presumably you want to catch other falsy values (e.g.,
> `undefined`), too.

Done.

> @@ +53,5 @@
> > +  hasValidToken: function() {
> > +    if (this._token === null) {
> > +      return false;
> > +    }
> > +    if (this._token.expiration < new Date()) {
> 
> See comment below about Date.now().

Done.

> @@ +61,5 @@
> > +    if (!signedInUser) {
> > +      return false;
> > +    }
> > +    // Does the signed in user match the user we retrieved the token for?
> > +    if (this._normalizeAccountValue(signedInUser.email) !== this.account) {
> 
> Perhaps this can be better expressed by having a way to ask the FxA module
> if this user (this.account) is (still) signed in? Might be more efficient.
> Your call.

Karlof-Newman

> @@ +71,5 @@
> > +  /**
> > +   * Wrap and synchronize FxAccounts.getSignedInUser().
> > +   *
> > +   * @return credentials
> > +   *         with fields email, uid, sessionToken, assertion, kA, and kB.
> 
> Comment-rot risk: this depends on the contents of
> _tokenServerClient.getTokenFromBrowserIDAssertion, rather than being 'owned'
> by this method.

Fixed.

> @@ +75,5 @@
> > +   *         with fields email, uid, sessionToken, assertion, kA, and kB.
> > +   */
> > +  _getSignedInUser: function() {
> > +    let userBlob;
> > +    let blockingCall = Async.makeSpinningCallback();
> 
> cb (see later comment).

Done.

> @@ +93,5 @@
> > +    }
> > +    return userBlob;
> > +  },
> > +
> > +  _getTokenForUser: function(user) {
> 
> I'd name this `fetchTokenForUser`, because it's not an accessor.

Karlof-Newman

> I'd also suggest using a default param for the token server URI
> (function(user, uri="http://...")). That'll give you some testability here.

Went with a pref.

> @@ +95,5 @@
> > +  },
> > +
> > +  _getTokenForUser: function(user) {
> > +    let token;
> > +    let blockingCall = Async.makeSpinningCallback();
> 
> We conventionally call this 'cb'.

Done.

> @@ +100,5 @@
> > +    let tokenServerURI = "http://auth.oldsync.dev.lcip.org/1.0/sync/1.1";
> > +
> > +    try {
> > +      this._tokenServerClient.getTokenFromBrowserIDAssertion(
> > +        tokenServerURI, user.assertion, blockingCall);
> 
> Is it possible for assertion to be undefined here?

It would violate FxAccounts's responsibility, and TokenServerClient will throw an Error if it happens.

> @@ +107,5 @@
> > +      this._log.info("TokenServerClient.getTokenFromBrowserIDAssertion() failed with: " + err.api_endpoint);
> > +      return null;
> > +    }
> > +
> > +    let expiration = new Date();
> 
> You might consider using `Date.now()` instead of `new Date()`: no need to
> allocate an object, and fewer confusing automatic coercions when doing
> comparisons. Just
> 
>   token.expiration = Date.now() + token.duration * 1000;
>   return token;

Done.
 
> @@ +108,5 @@
> > +      return null;
> > +    }
> > +
> > +    let expiration = new Date();
> > +    expiration.setTime(expiration.valueOf() + token.duration * 1000);
> 
> (Alternatively, `let expiration = new Date(Date.now() + token.duration *
> 1000)` as one line, rather than using setTime.)
> 
> @@ +132,5 @@
> > +      this._token = this._getTokenForUser(user);
> > +      if (!this._token) {
> > +        return null;
> > +      }
> > +      this.account = this._normalizeAccountValue(user.email);
> 
> Can we break out 126-137 (or 127-136) as a separate method? This is a pretty
> core chunk of logic: "prepareTokenForSignedInUser", optionally by checking
> whether we already have one for the current signed in user. I could imagine
> us wanting to call this speculatively, rather than during the first HTTP
> request.

Karlof-Newman

> @@ +135,5 @@
> > +      }
> > +      this.account = this._normalizeAccountValue(user.email);
> > +    }
> > +    let credentials = {key: this._token, id: this.username,
> > +                       algorithm: "sha256"};
> 
> New line per field, plz, ideally sorted.

Fixed.

> @@ +150,5 @@
> > +  _addAuthenticationHeader: function(request, method) {
> > +    let header = this._getAuthenticationHeader(request, method);
> > +    if (header === null) {
> > +      return null;
> > +    }
> 
> Again, 
> 
>   if (!header) {
>     return null;
>   }

Done.

> ::: services/sync/tests/unit/test_browserid_identity.js
> @@ +5,5 @@
> > +Cu.import("resource://services-sync/rest.js");
> > +Cu.import("resource://gre/modules/Promise.jsm");
> > +
> > +let mockUser = {email: 'email', uid: 'user_uid', assertion: 'assertion',
> > +                sessionToken: 'sessionToken', kA: 'kA', kB: 'kB'};
> 
> Newline after this, and ideally one field per line, sorted, with trailing
> comma.

Done.

> @@ +6,5 @@
> > +Cu.import("resource://gre/modules/Promise.jsm");
> > +
> > +let mockUser = {email: 'email', uid: 'user_uid', assertion: 'assertion',
> > +                sessionToken: 'sessionToken', kA: 'kA', kB: 'kB'};
> > +let _MockFXA = function(blob) { // FxAccounts
> 
> function _MockFXA(blob) {
>     ...
>   }
>   _MockFXA.prototype = {
>     __proto__: FxAccounts,
>   }
> 
> ? Not sure to what extent you want to be totally mocking vs 'subclassing'
> here.

Karlof-Newman.

> @@ +23,5 @@
> > +                 uid: "token_uid", duration: 300};
> > +let mockTSC = { // TokenServerClient
> > +  getTokenFromBrowserIDAssertion: function(uri, assertion, cb) {
> > +    cb(null, mockToken);
> > +  }
> 
> Trailing comma.

Fixed.

> @@ +33,5 @@
> > +  initTestLogging("Trace");
> > +  Log4Moz.repository.getLogger("Sync.Identity").level = Log4Moz.Level.Trace;
> > +  run_next_test();
> > +  _("Verify initial state");
> > +  do_check_eq(browseridManager._token, null);
> 
> You shouldn't do this after run_next_test(); move these to a separate test.

Done.

> @@ +41,5 @@
> > +
> > +add_test(function test_getResourceAuthenticator() {
> > +    _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header.");
> > +    let authenticator = browseridManager.getResourceAuthenticator();
> > +    do_check_neq(authenticator, null);
> 
> do_check_true(!!authenticator);
> 
> seems to be the dominant paradigm for this.

Done.
 
> @@ +48,5 @@
> > +               method: 'GET'};
> > +    let output = authenticator(req, 'GET');
> > +    do_check_true('headers' in output);
> > +    do_check_true('authorization' in output.headers);
> > +    do_check_eq('Hawk', output.headers.authorization.slice(0, 4));
> 
> String.startsWith()?

Done.

> @@ +65,5 @@
> > +    do_check_neq(authenticator, null);
> > +    let output = authenticator(request, 'GET');
> > +    do_check_eq(request.uri, output.uri);
> > +    do_check_eq(output._headers.authorization.slice(0, 4), 'Hawk');
> > +    do_check_true(output._headers.authorization.indexOf('nonce') > -1);
> 
> String.contains()

Done.

> @@ +78,5 @@
> > +      getTokenFromBrowserIDAssertion: function(uri, assertion, cb) {
> > +        cb(null, {id: "id", key: "key",
> > +            // XXX read from prefs or constant
> > +            api_endpoint: "http://auth.oldsync.dev.lcip.org/1.0/sync/1.1",
> > +            uid: "token_uid", duration: -10});
> 
> Should we actually be checking for invalid durations like this?

Chris has made it clear that we should be checking for expiration. I chose this approach as a simple way to simulate expiration that seems to work. It's certainly a hack. The alternative is me getting timers working in xpcshell, which hasn't gone well so far :-).

Comment 35

[Richard Newman [:rnewman]]

> > It looks like you don't need the constructor fallbacks, or at least the
> > first one.
> 
> I need it to accept a mock from the unit test. Karlof-Newman.

My point was that in tests you could call

  new BrowserIDManager(mockService, mockClient);

and in real code you should do:

  this.whatever = new BrowserIDManager(theRealFxAccounts, new TokenServerClient());

It's not the constructor arguments I'm poking at -- it's the fallbacks.


> I did not take action here. :ckarlof : want me to install a pref observer?

Probably not super valuable, but you might consider doing so for production-ready code. This note is mainly for when QA asks you "why am I not seeing log messages?", so you can say "did you relaunch Firefox after changing that pref?".


> Chris has made it clear that we should be checking for expiration. I chose
> this approach as a simple way to simulate expiration that seems to work.
> It's certainly a hack. The alternative is me getting timers working in
> xpcshell, which hasn't gone well so far :-).

There's another alternative, which is to define a `now()` method and use that instead of Date.now(). A test-only subclass can override `now()`. This is what we do for the data reporting notification tests and FHR.

Comment 36

[Chris Karlof [:ckarlof]]

(In reply to Richard Newman [:rnewman] from comment #35)
> > > It looks like you don't need the constructor fallbacks, or at least the
> > > first one.
> > 
> > I need it to accept a mock from the unit test. Karlof-Newman.
> 
> My point was that in tests you could call
> 
>   new BrowserIDManager(mockService, mockClient);
> 
> and in real code you should do:
> 
>   this.whatever = new BrowserIDManager(theRealFxAccounts, new
> TokenServerClient());
> 
> It's not the constructor arguments I'm poking at -- it's the fallbacks.

I'm happy with removing the fallbacks.

> > I did not take action here. :ckarlof : want me to install a pref observer?
> 
> Probably not super valuable, but you might consider doing so for
> production-ready code. This note is mainly for when QA asks you "why am I
> not seeing log messages?", so you can say "did you relaunch Firefox after
> changing that pref?".

Sam, we can delay on the pref observers. We'll likely need some for other prefs as well.

> > Chris has made it clear that we should be checking for expiration. I chose
> > this approach as a simple way to simulate expiration that seems to work.
> > It's certainly a hack. The alternative is me getting timers working in
> > xpcshell, which hasn't gone well so far :-).
> 
> There's another alternative, which is to define a `now()` method and use
> that instead of Date.now(). A test-only subclass can override `now()`. This
> is what we do for the data reporting notification tests and FHR.

That's a nice approach

Comment 37

[Chris Karlof [:ckarlof]]

(In reply to Sam Penrose from comment #34)
> > @@ +25,5 @@
> > > +this.BrowserIDManager = function BrowserIDManager(fxaService, tokenServerClient) {
> > > +  this._fxaService = fxaService || new FxAccounts();
> > > +  this._tokenServerClient = tokenServerClient || new TokenServerClient();
> > > +  this._log = Log4Moz.repository.getLogger("Sync.Identity");
> > > +  this._log.Level = Log4Moz.Level[Svc.Prefs.get("log.logger.identity")];
> > 
> > Not a review comment, but a word of warning: because you're not installing a
> > pref observer here, you'll need to re-instantiate this class for changes to
> > this pref to take effect.
> 
> I did not take action here. :ckarlof : want me to install a pref observer?
> 

Not now. I'll file a bug.

> > @@ +61,5 @@
> > > +    if (!signedInUser) {
> > > +      return false;
> > > +    }
> > > +    // Does the signed in user match the user we retrieved the token for?
> > > +    if (this._normalizeAccountValue(signedInUser.email) !== this.account) {
> > 
> > Perhaps this can be better expressed by having a way to ask the FxA module
> > if this user (this.account) is (still) signed in? Might be more efficient.
> > Your call.

Not now, but I like the idea. I'll file a bug.

> > @@ +93,5 @@
> > > +    }
> > > +    return userBlob;
> > > +  },
> > > +
> > > +  _getTokenForUser: function(user) {
> > 
> > I'd name this `fetchTokenForUser`, because it's not an accessor.

I take your point. I also think it's fine as it is and not a big deal. Sam, change it to "fetch" if you prefer or if Richard feels strongly about it.
 
> > @@ +132,5 @@
> > > +      this._token = this._getTokenForUser(user);
> > > +      if (!this._token) {
> > > +        return null;
> > > +      }
> > > +      this.account = this._normalizeAccountValue(user.email);
> > 
> > Can we break out 126-137 (or 127-136) as a separate method? This is a pretty
> > core chunk of logic: "prepareTokenForSignedInUser", optionally by checking
> > whether we already have one for the current signed in user. I could imagine
> > us wanting to call this speculatively, rather than during the first HTTP
> > request.

I agree it should be refactored into a method if it's needed independently, which you make a good case for. I also think it's a awkward method to name, but your proposal's not bad. I asked Sam leave it inline for now for clarity's sake, but I don't feel strongly about it.

> > @@ +6,5 @@
> > > +Cu.import("resource://gre/modules/Promise.jsm");
> > > +
> > > +let mockUser = {email: 'email', uid: 'user_uid', assertion: 'assertion',
> > > +                sessionToken: 'sessionToken', kA: 'kA', kB: 'kB'};
> > > +let _MockFXA = function(blob) { // FxAccounts
> > 
> > function _MockFXA(blob) {
> >     ...
> >   }
> >   _MockFXA.prototype = {
> >     __proto__: FxAccounts,
> >   }
> > 
> > ? Not sure to what extent you want to be totally mocking vs 'subclassing'
> > here.
> 
> Karlof-Newman.

Richard, are you saying to use the "subclass to mock" strategy? That's fine with me.

Great job Sam, and thanks Richard! Let's get this thing landed!

Comment 38

[Chris Karlof [:ckarlof]]

(In reply to Chris Karlof [:ckarlof] from comment #37)
> > > @@ +61,5 @@
> > > > +    if (!signedInUser) {
> > > > +      return false;
> > > > +    }
> > > > +    // Does the signed in user match the user we retrieved the token for?
> > > > +    if (this._normalizeAccountValue(signedInUser.email) !== this.account) {
> > > 
> > > Perhaps this can be better expressed by having a way to ask the FxA module
> > > if this user (this.account) is (still) signed in? Might be more efficient.
> > > Your call.
> 
> Not now, but I like the idea. I'll file a bug.

I changed my mind. We'll eventually replace this functionality (detecting changes in the logged in state) with an observer. See bug 915453.

Comment 40

[Sam Penrose]

Attachment: 911378.10_02.patch

I believe this incorporates all the feedback. Closest call was not extracting a method at the start of _getAuthenticationHeader(); if Richard feels that's best I will do so. Thanks very much for the helpful and patient feedback.

Comment 41

[Richard Newman [:rnewman]]

Comment on attachment 813103 [details] [diff] [review]
911378.10_02.patch

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

::: services/sync/modules/browserid_identity.js
@@ +119,5 @@
> +      this._log.info("TokenServerClient.getTokenFromBrowserIDAssertion() failed with: " + err.api_endpoint);
> +      return null;
> +    }
> +
> +    token.expiration = Date.now() + (token.duration * 1000);

Shouldn't this use this._now() ? Would require minor test amendments, of course.

Comment 42

[Sam Penrose]

(In reply to Richard Newman [:rnewman] from comment #41)
> Comment on attachment 813103 [details] [diff] [review]
> 911378.10_02.patch
> 
> Review of attachment 813103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/modules/browserid_identity.js
> @@ +119,5 @@
> > +      this._log.info("TokenServerClient.getTokenFromBrowserIDAssertion() failed with: " + err.api_endpoint);
> > +      return null;
> > +    }
> > +
> > +    token.expiration = Date.now() + (token.duration * 1000);
> 
> Shouldn't this use this._now() ? Would require minor test amendments, of
> course.

Maybe I'm missing something here, but if we use this._now() to both set and inspect then overriding it in the unittests doesn't alter the relationship between "now" and "expiration": it just offsets them both identically. Alternately I could monkeypatch this._now() AFTER retrieving the token/setting token.expiration and before calling hasValidToken() (I maybe have tried and failed -- can't remember). Is the latter what you would like me to do?

Comment 43

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #42)

> Alternately I could monkeypatch this._now() AFTER retrieving the
> token/setting token.expiration and before calling hasValidToken() (I maybe
> have tried and failed -- can't remember). Is the latter what you would like
> me to do?

That's the spirit, yeah. The main reason for being consistent in this regard is that, some time down the road, some sucker is going to try to use _now to test something else (e.g., verifying a fixed date input produces the same output), and encounter misery.

See `defineNow` in services/datareporting/tests/xpcshell/test_policy.js:

function defineNow(policy, now) {
  print("Adjusting fake system clock to " + now);
  Object.defineProperty(policy, "now", {
    value: function customNow() {
      return now;
    },
    writable: true,
  });
}

which is used thusly:

  defineNow(policy, policy._futureDate(-24 * 60 * 60 * 1000));
  policy.recordUserAcceptance();
  defineNow(policy, policy.nextDataSubmissionDate);

If it looks hairy to alter the definition of _now at the right spot, then just leave a warning comment for the unwary.

Comment 44

[Sam Penrose]

OK, Ready to go. Do I upload a new patch flagged r?, or has acceptance of the previous moved us to a different node on the development state machine?

Comment 45

[Richard Newman [:rnewman]]

(In reply to Sam Penrose from comment #44)
> OK, Ready to go. Do I upload a new patch flagged r?, or has acceptance of
> the previous moved us to a different node on the development state machine?

If r+ was given on a previous patch, the changes aren't hairy (with a little more leeway given for test code), and there's no question about whether they satisfy the reviewer's last comments, one typically uploads a fresh patch with a "carry-forward" r+.

If you're in doubt, it's always acceptable to throw it at the reviewer again, typically mentioning that it should be a rubber-stamp review.

If, in the course of addressing the comments, you find that things aren't as trivial as the reviewer might have thought, either go through with it and r?, or push back, because the reviewer probably expected that it was a small change.

Comment 46

[Sam Penrose]

Attachment: 911378.10_02.2.patch

One-expression tweak to module relative to previous patch, plus Object.defineProperty()-based changes to xpcshell test per r+.

Comment 47

[Zachary Carter [:zaach]]

https://hg.mozilla.org/projects/elm/rev/205a579f88a9

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/205a579f88a9

Comment 49

[James Bonacci [:jbonacci]]

Cleaning up Resolved/Fixed bugs from December's first release.
Verified that we now have a working first-release of FxA to Desktop/Android Nightly.
Re-open as needed.