929386 - BrowserID DOM API extension to support using Firefox Accounts issuer

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

For signing into Firefox Accounts (FXA), we want to force navigator.id.* calls always to zoom to the issuer for FXA, not persona.org and not a primary IdP (gmail.com, etc).

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Two possibilities under discussion:

1. Have RPs set a <meta> tag in the dom as a high-level switch
2. Add properties to the watch() and request() calls

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Current thinking is that the meta tag is wrong for two reasons: It prevents a caller from using both the Persona and the FXA APIs at the same time, and it requires the caller to be able to modify the dom when it really just wants to use a JS API.

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: wip

Having trouble building for unagi today, so this patch is as yet untested.

It should send a mozChromeEvent up with the DOM request and wait for a responding mozContentEvent.  The responding event should share the same uuid as the chrome event.

https://github.com/jedp/mozilla-central/compare/929386-fxa-api

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Here's a test page for use until there are unit tests and a test_app in gaia:
https://people.mozilla.org/~jparsons/test_fxa.html

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 929386.diff

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Pass RP watch() and request() from chrome to system content (wip)

The same, in github: https://github.com/jedp/mozilla-central/tree/929386-fxa-api

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Pass RP watch() and request() from chrome to system content (wip)

Added documentation.

When an RP calls the DOM API methods watch() or request(), sends those messages up to system content in a mozChromeEvent.

Chrome events look like this:

  {
    type: 'identity-firefox-accounts-request',
    rpid: <unique id of the calling RP>,
    uuid: <unique id of this message>
  }

System app should send responses back of the form:

  {
    method: 'login',
    assertion: <text content of assertion; login messages only>,
    uuid: <uuid of corresponding chrome event
  }

On receipt of a valid content message, calls the corresponding method on the Identity Service for the RP caller.

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Pass RP watch() and request() from chrome to system content (wip)

(oops - the previous patch breaks persona sign-in)

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

If request() is being called from a certified app, we should signal to the FXA manager that the caller is trusted.  The FXA manager can use this to determine whether or not to present UI in front of the user.

Comment 10

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Pass RP watch() and request() from chrome to system content (wip)

Added appStatus of the DOM caller to the observer messages.

appStatus is a number.  It means:

   0: APP_STATUS_NOT_INSTALLED
   1: APP_STATUS_INSTALLED
   2: APP_STATUS_PRIVILEGED
   3: APP_STATUS_CERTIFIED

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Pass RP watch() and request() from chrome to system content (wip)

Oops.  Forgot to add appStatus to the mozChromeEvent message.  Must be getting tired :)

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Pass RP watch() and request() from chrome to system content (wip)

Fixed patch headers so it can be applied properly

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (early WIP)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue (early WIP)

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (early WIP)

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Attachment: FxAccounts.jsm mock

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue (WIP)

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (WIP)

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (WIP)

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: FxAccounts.jsm mock

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0.1: Turn FxAccountsClient into a component

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0.2: FxAccountsClient temporary fixes

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (WIP)

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0.2: FxAccountsClient and FxAccounts temporary fixes

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (early WIP)

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (WIP)

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (WIP)

This should work on top of the latest patches from bug 935232 and bug 909967. We can create accounts, sign in and get the account data.

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Now the good one...

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service (WIP)

B2G service using the FxAccountsManager

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Now with getAssertion

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Check for email verification added

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (early WIP)

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue (WIP)

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (WIP)

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue (WIP)

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service

Comment 42

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 0: FxAccountsManager

FxAccountsClient is a jsm again

Comment 43

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API (WIP)

I think the RP should only specify wantIssuer once, when watch() is invoked.  Otherwise it's possible that they could fail to add the parameter to request() as well and suddenly find themselves teleported into an alternate Persona universe, with RP flow broken.

Also added (commented out) a pref to enable/disable firefox-accounts by always forcing the persona flow.

Comment 44

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

ferjm, getAssertion is working beautifully.  The problem was in our HAWK code: We were missing a property in the header.

So be sure you have this applied:

  https://bug935232.bugzilla.mozilla.org/attachment.cgi?id=8335696

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

\o/ Thanks Jed!

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (early WIP)

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8336178 [details] [diff] [review]
Part 0: FxAccountsManager

Since we are close to have most of the features already done. I'd like to start getting some feedback while I write tests for this before asking for a final review.

:zaach, this is a first approach for the abstraction layer that we talked about. I'd like to evolve this to use something like a FxAccountsSignInStrategy interface that products will need to implement as we agreed, but since b2g is the only one using this abstraction layer so far, I guess we are good as it is for now.

Thanks!

Comment 49

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

"Part 2: UI Glue (WIP)" needs a teensy rebase to stay current with m-c.

Comment 50

[Zachary Carter [:zaach]]

Comment on attachment 8336178 [details] [diff] [review]
Part 0: FxAccountsManager

># HG changeset patch
># Parent 78096567f7afe1dedd3cfbcbc28ace327275466b
># User Fernando Jiménez <[e-mail]>
>Bug 929386: BrowserID DOM API extension to support using Firefox Accounts issuer. Part 0: FxAccountsManager. r=zaach
>
>diff --git a/services/fxaccounts/FxAccountsManager.jsm b/services/fxaccounts/FxAccountsManager.jsm
>new file mode 100644
>--- /dev/null
>+++ b/services/fxaccounts/FxAccountsManager.jsm
>@@ -0,0 +1,279 @@
>+/* 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/. */
>+
>+/**
>+ * Temporary abstraction layer for common Fx Accounts operations.
>+ * For now, we will be using this module only from B2G but in the end we might
>+ * want this to be merged with FxAccounts.jsm and let other products also use
>+ * it.
>+ */
>+
>+"use strict";
>+
>+this.EXPORTED_SYMBOLS = ["FxAccountsManager"];
>+
>+const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/FxAccounts.jsm");
>+Cu.import("resource://gre/modules/Promise.jsm");
>+
>+XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsClient",
>+  "resource://gre/modules/FxAccountsClient.jsm");
>+
>+this.FxAccountsManager = {
>+
>+  // We keep the session details here so consumers don't need to deal with
>+  // session tokens and are only required to handle the email.
>+  _activeSession: null,
>+
>+  // We only expose the email and the verified status so far.
>+  get _user() {
>+    if (!this._activeSession || !this._activeSession.email) {
>+      return null;
>+    }
>+
>+    return {
>+      accountId: this._activeSession.email,

Perhaps we should call this something different ("email" ?) to avoid confusion with the actual account ID.

>+      verified: this._activeSession.verified
>+    }
>+  },
>+
>+  _signInSignUp: function(aMethod, aAccountId, aPassword) {
>+    if (!aAccountId) {
>+      return Promise.reject({
>+        error: 'INVALID_ACCOUNTID'
>+      });
>+    }
>+
>+    if (!aPassword) {
>+      return Promise.reject({
>+        error: 'INVALID_PASSWORD'
>+      });
>+    }
>+
>+    let client = new FxAccountsClient();
>+    if (!client) {
>+      return Promise.reject({
>+        error: 'INTERNAL_ERROR_NO_CLIENT'
>+      });
>+    }
>+
>+    // Check that there is no signed in account first.
>+    if (this._activeSession) {
>+      return Promise.reject({
>+        error: 'ALREADY_SIGNED_IN_USER',
>+        details: {
>+          user: this._user
>+        }
>+      });
>+    }
>+
>+    return fxAccounts.getSignedInUser().then(
>+      user => {
>+        if (user) {
>+          return Promise.reject({
>+            error: 'ALREADY_SIGNED_IN_USER',
>+            details: {
>+              user: this._user
>+            }
>+          });
>+        }
>+
>+        return client[aMethod](aAccountId, aPassword);
>+      }
>+    ).then(
>+      user => {
>+        if (!user || !user.uid || !user.sessionToken) {
>+          return Promise.reject({
>+            error: 'INTERNAL_ERROR_INVALID_USER',
>+            details: {
>+              user: user
>+            }
>+          });
>+        }
>+
>+        // Save the credentials of the signed in user.
>+        user.email = aAccountId;
>+        return fxAccounts.setSignedInUser(user, false).then(
>+          () => {
>+            this._activeSession = user;
>+            return Promise.resolve({
>+              accountCreated: aMethod === 'signUp',
>+              user: this._user
>+            });
>+          }
>+        );
>+      }
>+    );
>+  },
>+
>+  _getAssertion: function(aAudience) {
>+    return fxAccounts.getAssertion(aAudience);
>+  },
>+
>+  // -- API --
>+
>+  signIn: function(aAccountId, aPassword) {
>+    return this._signInSignUp('signIn', aAccountId, aPassword);
>+  },
>+
>+  signUp: function(aAccountId, aPassword) {
>+    return this._signInSignUp('signUp', aAccountId, aPassword);
>+  },
>+
>+  signOut: function() {
>+    if (!this._activeSession) {
>+      return Promise.resolve();
>+    }
>+
>+    return fxAccounts.signOut(this._activeSession.sessionToken).then(
>+      () => {
>+        this._activeSession = null;
>+        return Promise.resolve();
>+      }
>+    );

fxAccounts.signOut will remove the local session data, but we need to also call client.signOut in order to delete the session on the auth server.

>+  },
>+
>+  getAccount: function() {
>+    // We check first if we have session details cached.
>+    if (this._activeSession) {
>+      // If our cache says that the account is not yet verified, we check that
>+      // this information is correct, and update the cached data if not.
>+      if (this._activeSession && !this._activeSession.verified) {
>+        return this.verificationStatus(this._activeSession);
>+      }
>+
>+      return Promise.resolve(this._user);
>+    }
>+
>+    // If no cached information, we try to get it from the persistent storage.

FWIW, fxAccounts also caches the current user, so the call won't always touch persistent storage.

>+    return fxAccounts.getSignedInUser().then(
>+      user => {
>+        if (!user || !user.email) {
>+          return Promise.resolve(null);
>+        }
>+
>+        this._activeSession = user;
>+        // If we get a stored information of a not yet verified account,
>+        // we check this information with the server, update the stored
>+        // data if needed and finally return the account details.
>+        if (!user.verified) {
>+          return this.verificationStatus(user);
>+        }
>+
>+        return Promise.resolve(this._user);
>+      }
>+    );
>+  },
>+
>+  queryAccount: function(aAccountId) {
>+    let deferred = Promise.defer();
>+
>+    let client = new FxAccountsClient();
>+    if (!client) {
>+      return Promise.reject({
>+        error: 'INTERNAL_ERROR_NO_CLIENT'
>+      });
>+    }
>+
>+    return client.accountExist(aAccountId).then(
>+      result => {
>+        // Getting a 200 response per se indicates that the account exists.
>+        return Promise.resolve({
>+          registered: true
>+        });
>+      },
>+      error => {
>+        if (error.code === 400 && error.errno === 102) {
>+          return Promise.resolve({
>+            registered: false
>+          });
>+        }
>+
>+        return Promise.reject({
>+          error: 'SERVER_ERROR',
>+          details: error
>+        });
>+      }
>+    );
>+  },

The client.accountExist method has changed to client.accountExists and will abstract away handling the "account doesn't exist" error, so you'll get either true/false or a server error. This code can be:

return client.accountExists(aAccountId).then(
    result => { registered: result },
    error => Promise.reject({
      error: 'SERVER_ERROR',
      details: error
    })
  );

>+
>+  verificationStatus: function() {
>+    if (!this._activeSession || !this._activeSession.sessionToken) {
>+      return Promise.reject({
>+        error: 'NO_TOKEN_SESSION'
>+      });
>+    }
>+
>+    // There is no way to unverify an already verified account, so we just
>+    // return the account details of a verified account
>+    if (this._activeSession.verified) {
>+      return Promise.resolve(this._user);
>+    }
>+
>+    let client = new FxAccountsClient();
>+    if (!client) {
>+      return Promise.reject({
>+        error: 'INTERNAL_ERROR_NO_CLIENT'
>+      });
>+    }
>+
>+    return client.recoveryEmailStatus(this._activeSession.sessionToken).then(
>+      data => {
>+        // If the verification status is different from the one that we have
>+        // stored, we update it and return the session data. If not, we simply
>+        // return the session data.
>+        if (this._activeSession.verified != data.verified) {
>+          this._activeSession.verified = data.verified;
>+          return fxAccounts.setSignedInUser(this._activeSession).then(
>+            () => {
>+              return Promise.resolve(this._user);
>+            }
>+          );
>+        } else {
>+          return Promise.resolve(this._user);
>+        }
>+      }
>+    );
>+  },
>+
>+  getAssertion: function(aAudience) {
>+    if (!aAudience) {
>+      return Promise.reject({
>+        error: 'INVALID_AUDIENCE'
>+      });
>+    }
>+
>+    // If there is no currently signed in user, we trigger the signIn UI flow.
>+    return this.getAccount().then(
>+      user => {
>+        if (!user) {
>+          let ui = Cc['@mozilla.org/fxaccounts/fxaccounts-ui-glue;1']
>+                     .createInstance(Ci.nsIFxAccountsUIGlue);
>+          if (!ui) {
>+            return Promise.reject({
>+              error: 'INTERNAL_ERROR_NO_UI_GLUE'
>+            });
>+          }
>+
>+          return ui.signInFlow().then(
>+            result => {
>+              return this._getAssertion(aAudience);
>+            },
>+            error => {
>+              return Promise.reject({
>+                error: 'UI_ERROR',
>+                details: error
>+              });
>+            }
>+          );
>+        }
>+
>+        return this._getAssertion(aAudience);
>+      }
>+    );
>+  }
>+
>+};
>diff --git a/services/fxaccounts/moz.build b/services/fxaccounts/moz.build
>--- a/services/fxaccounts/moz.build
>+++ b/services/fxaccounts/moz.build
>@@ -3,8 +3,12 @@
> # 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/.
> 
> TEST_DIRS += ['tests']
> 
> EXTRA_JS_MODULES += ['FxAccounts.jsm',
>                      'FxAccountsClient.jsm']
>+
>+# For now, we will only be using the FxA manager in B2G.
>+if CONFIG['MOZ_B2G']:
>+  EXTRA_JS_MODULES += ['FxAccountsManager.jsm']

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Thanks for your feedback Zac!

This is almost ready for review, I just need to do a final pass to report some common server errors instead of directly return a SERVER_ERROR with the details coming from the server. This way the UI will have it easier.

Tim, I would really appreciate if you could also give me your feedback in the mean time.

Thanks!

Comment 52

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API

Jed, you know this patch pretty well :). I think is ready now. I need to write some tests for it in another patch though.

Olli, this patch changes the public navigator.id DOM API (B2G only) [1] by allowing RPs to request a Firefox Accounts assertion specifying the "firefox-accounts" as the value of the "wantIssuer" .watch() option.


Tests for this part will be coming in another patch.

Thanks!

[1] https://developer.mozilla.org/en-US/Persona/The_navigator.id_API

Comment 53

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue

Fabrice, this patch adds the FxAccountsUIGlue interface and B2G specific implementation. The plan is that, at some point, other products will also implement this interface.

The counter part of this patch is Gaia FxAccountsManager from Bug 929388.

Thanks!

Comment 54

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service

This patch introduces a service that acts like a proxy for Firefox Accounts related requests that comes from the Gaia System app and FxAccountsManager from Part 0.

The Gaia side of this is FxAccountsClient from Bug 929388.

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: FxAccountsManager tests

These are the tests for FxAccountsManager. I am still missing some error handling, but I would really appreciate some feedback in the meantime. Thanks!

Comment 56

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8339408 [details] [diff] [review]
Part 0: FxAccountsManager

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

drive-by feedback

::: services/fxaccounts/FxAccountsManager.jsm
@@ +52,5 @@
> +
> +  _signInSignUp: function(aMethod, aAccountId, aPassword) {
> +    if (!aAccountId) {
> +      return Promise.reject({
> +        error: 'INVALID_ACCOUNTID'

I'm assuming these (eventually) can't be truly arbitrary, so these should probably be constants?

@@ +63,5 @@
> +      });
> +    }
> +
> +    let client = this._createFxAccountsClient();
> +    if (!client) {

I'm not sure how worthwhile this is - by inspection, it seems more likely we will fail due to an exception rather than just getting null.  I'd be inclined to remove this check and just let things blow up.

@@ +85,5 @@
> +        if (user) {
> +          return Promise.reject({
> +            error: 'ALREADY_SIGNED_IN_USER',
> +            details: {
> +              user: this._user

is this._user going to be set here?  And is this really an error condition?  eg, maybe the request should be silently ignored, or we should just re-attempt a login anyway?

@@ +98,5 @@
> +        if (!user || !user.uid || !user.sessionToken) {
> +          return Promise.reject({
> +            error: 'INTERNAL_ERROR_INVALID_USER',
> +            details: {
> +              user: user

is it a problem that the 'user' object here might have more fields than when this._user is used?  eg, IIUC, it seems like sync keys etc will be passed.

@@ +137,5 @@
> +    if (!this._activeSession) {
> +      return Promise.resolve();
> +    }
> +
> +    let client = this._createFxAccountsClient();

ditto above (and a number of other places later) - handling null here doesn't seem that useful

@@ +281,5 @@
> +
> +        // If there is no currently signed in user, we trigger the signIn UI flow.
> +        let ui = Cc['@mozilla.org/fxaccounts/fxaccounts-ui-glue;1']
> +                   .createInstance(Ci.nsIFxAccountsUIGlue);
> +        if (!ui) {

will ui ever be null here? IIUC, an exception will be thrown on error.

@@ +293,5 @@
> +            // Even if we get a successful result from the UI, the account will
> +            // most likely be unverified, so we cannot get an assertion.
> +            if (result && result.verified) {
> +              return this._getAssertion(aAudience);
> +            } else {

given the return above, drop the else block.

Comment 57

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8339417 [details] [diff] [review]
Part 2: UI Glue

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

another drive-by.  I'm assuming this must be an xpcom component due to the DOM bindings in a different patch.

::: b2g/components/FxAccountsUIGlue.js
@@ +19,5 @@
> +}
> +
> +FxAccountsUIGlue.prototype = {
> +
> +  _browser: Services.wm.getMostRecentWindow("navigator:browser"),

I don't have much context, so this might be meaningless - but it seems slightly strange for this instance variable to be obtained in this way.  It implies an instance of the component must be very short lived (ie, you wouldn't want to store it anywhere incase the browser window is closed).  Further, how can you be sure the most recent navigator:browser is actually the desired one?

IOW, it looks like _browser should either be (most likely) explicitly passed in as a param or (less likely) a getter?

@@ +30,5 @@
> +
> +    let event = content.document.createEvent('CustomEvent');
> +    event.initCustomEvent('mozFxAccountsRPChromeEvent', true, true,
> +                          aMsg ? ObjectWrapper.wrap(aMsg, content) : {});
> +    content.dispatchEvent(event);

Assuming the event handler is in the content window, I wonder how this (and other event magic below) will go with desktop e10s?

Comment 58

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1, refactored: DOM API

Fernando, in response to your r? for the DOM patch, I had some ideas for improvements we could make.  But rather than just make a bunch of comments on the code (and most of the stuff I wanted to change was code I wrote myself earlier), I thought it would be better to express them in a new patch.  Maybe this is weird bugzilla behavior, but I'd like to ask you for r on this patch now instead :)

Essentially, this patch makes the following changes:

Removes a bunch of dead code from the Persona code path (the IdP functions, well-known file fetching, etc.).

Adds tests for Firefox Accounts watch() and request() (with mocked FXAManager)

Factors out the Persona and Firefox Accounts implementations into separate delegates.  I think this makes things cleaner.  In particular, it removes the if/else tests from each API method, and it replaces multiple tests for fxa-enabled with a single test in the getDelegate method.

What do you think?

Comment 59

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the feedback Mark!

(In reply to Mark Hammond [:markh] from comment #56)
> ::: services/fxaccounts/FxAccountsManager.jsm
> @@ +52,5 @@
> > +
> > +  _signInSignUp: function(aMethod, aAccountId, aPassword) {
> > +    if (!aAccountId) {
> > +      return Promise.reject({
> > +        error: 'INVALID_ACCOUNTID'
> 
> I'm assuming these (eventually) can't be truly arbitrary, so these should
> probably be constants?
> 

Yes, we can add constants for this.

 > @@ +85,5 @@
> > +        if (user) {
> > +          return Promise.reject({
> > +            error: 'ALREADY_SIGNED_IN_USER',
> > +            details: {
> > +              user: this._user
> 
> is this._user going to be set here?  And is this really an error condition? 
> eg, maybe the request should be silently ignored, or we should just
> re-attempt a login anyway?
>

I'm not sure that I am understanding your question. this._user is a getter, so it won't be set.

wrt to the error condition, I'd prefer to report to the consumer the failure attempt to sign in. What if the already logged user is different that the one that is trying to log? I prefer to keep things simple in this case and force the consumer to sign out before requesting a new sign in. We also avoid extra network consumption this way.
 
> @@ +98,5 @@
> > +        if (!user || !user.uid || !user.sessionToken) {
> > +          return Promise.reject({
> > +            error: 'INTERNAL_ERROR_INVALID_USER',
> > +            details: {
> > +              user: user
> 
> is it a problem that the 'user' object here might have more fields than when
> this._user is used?  eg, IIUC, it seems like sync keys etc will be passed.
> 

The problem is not with extra fields, but with missing ones.

> @@ +137,5 @@
> > +    if (!this._activeSession) {
> > +      return Promise.resolve();
> > +    }
> > +
> > +    let client = this._createFxAccountsClient();
> 
> ditto above (and a number of other places later) - handling null here
> doesn't seem that useful
> 

Ok, I'll remove this checks.

> @@ +281,5 @@
> > +
> > +        // If there is no currently signed in user, we trigger the signIn UI flow.
> > +        let ui = Cc['@mozilla.org/fxaccounts/fxaccounts-ui-glue;1']
> > +                   .createInstance(Ci.nsIFxAccountsUIGlue);
> > +        if (!ui) {
> 
> will ui ever be null here? IIUC, an exception will be thrown on error.
> 

I guess that depends on the implementation of nsIFxAccountsUIGlue. It can be null, so I feel safer keeping this check :)

> @@ +293,5 @@
> > +            // Even if we get a successful result from the UI, the account will
> > +            // most likely be unverified, so we cannot get an assertion.
> > +            if (result && result.verified) {
> > +              return this._getAssertion(aAudience);
> > +            } else {
> 
> given the return above, drop the else block.

Ok

Thanks again!

Comment 60

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mark Hammond [:markh] from comment #57)
> Comment on attachment 8339417 [details] [diff] [review]
> Part 2: UI Glue
> 
> Review of attachment 8339417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> another drive-by.  I'm assuming this must be an xpcom component due to the
> DOM bindings in a different patch.
> 
> ::: b2g/components/FxAccountsUIGlue.js
> @@ +19,5 @@
> > +}
> > +
> > +FxAccountsUIGlue.prototype = {
> > +
> > +  _browser: Services.wm.getMostRecentWindow("navigator:browser"),
> 
> I don't have much context, so this might be meaningless - but it seems
> slightly strange for this instance variable to be obtained in this way.  It
> implies an instance of the component must be very short lived (ie, you
> wouldn't want to store it anywhere incase the browser window is closed).

FWIW the instances of this component are supposed to be very short lived.

> Further, how can you be sure the most recent navigator:browser is actually
> the desired one?
> 

If I am not wrong, for B2G we only have one navigator:browser, so it will always be the same.

> IOW, it looks like _browser should either be (most likely) explicitly passed
> in as a param or (less likely) a getter?
> 

wrt the param option, since this is an implementation for an UI abstraction, I would prefer to let the specific implementers to get the appropriate value when needed. The core implementation of FxAccounts (which is the one using this component) does not need to know the specifics of each platform UIs.

And wrt the getter option, as I mentioned before, for B2G it does not make sense.

> @@ +30,5 @@
> > +
> > +    let event = content.document.createEvent('CustomEvent');
> > +    event.initCustomEvent('mozFxAccountsRPChromeEvent', true, true,
> > +                          aMsg ? ObjectWrapper.wrap(aMsg, content) : {});
> > +    content.dispatchEvent(event);
> 
> Assuming the event handler is in the content window, I wonder how this (and
> other event magic below) will go with desktop e10s?

FWIW this won't run in desktop, only in B2G :). But in any case, the event handler lives in the Gaia System app which runs in the chrome process.

Thanks Mark!

Comment 61

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8339622 [details] [diff] [review]
Part 1, refactored: DOM API

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

Thanks for the patch Jed! :)

Just a few comments.

We'll also need a DOM peer review.

::: toolkit/identity/MinimalIdentity.jsm
@@ +101,5 @@
> +  // methods on the IdentityService.
> +}
> +
> +PersonaDelegate.prototype = {
> +  watch: function personaWatch(aRp) {

nit: no need to name function properties anymore. Here and in the rest of the patch.

@@ +118,5 @@
> +    log("sending identity-controller-unwatch for id",
> +        options.id, options.origin);
> +    Services.obs.notifyObservers({ wrappedJSObject: options },
> +                                 "identity-controller-unwatch", null);
> +

uber-nit: remove extra line

@@ +175,5 @@
> +
> +  logout: function fxAccountsLogout(aRp) {
> +    // For now, it makes no sense to logout from an specific RP in
> +    // Firefox Accounts, so we just directly call the onlogout callback.
> +    this.doLogout(aRp.id);

this.context.doLogout?

@@ +202,5 @@
> +  try {
> +    this._fxaEnabled =
> +      Services.prefs.getPrefType(PREF_FXA_ENABLED) == Ci.nsIPrefBranch.PREF_BOOL
> +      && Services.prefs.getBoolPref(PREF_FXA_ENABLED);
> +    log("Firefox Accounts is " + (this._fxaEnabled ? "" : "not") + " enabled");

Bail out here if fxa is not enabled

@@ +225,5 @@
> +  }
> +
> +  // RPs are required to specify the 'wantIssuer: <fxaissuer>' option to get
> +  // Firefox Accounts assertions. The specific required issuer can be changed
> +  // via identity.fxaccounts.issuer preference.

I wasn't sure about the need of this pref, so feel free to get rid of it if you consider that it is not needed.

@@ +260,3 @@
>      }
> +
> +    if (rp.wantIssuer == "firefox-accounts") {

if (rp.wantIssuer == this._fxaIssuer)

even if you decide to get rid of the identity.fxaccounts.issuer pref, that should be a const

@@ +271,5 @@
> +        throw new Error("Non certified apps are not allowed to get " +
> +                        "Firefox Accounts assertions");
> +      }
> +      return this._delegates["firefox-accounts"];
> +    } 

nit: trailing white space

::: toolkit/identity/tests/unit/head_identity.js
@@ +121,5 @@
> +  mockedDoc.wantIssuer = "firefox-accounts";
> +
> +  mockedDoc.doReady = partial(aDoFunc, "ready");
> +  mockedDoc.doLogin = partial(aDoFunc, "login");
> +

It would be great if we could also test logout

::: toolkit/identity/tests/unit/test_minimalidentity.js
@@ +169,5 @@
> + */
> +
> +function test_fxa_watch() {
> +  do_test_pending();
> +  

nit: white space

@@ +189,5 @@
> +    // callback.  Now we can call request();
> +    MinimalIDService.RP.request(mockedRP.id);
> +  });
> +
> +  // Mock the Firefox Accounts Manager

This is not a mock for FxAccountsManager, but for FirefoxAccountsDelegate

@@ +203,5 @@
> +  }
> +
> +  // Replace the firefox accounts delegate in the Identity Service.
> +  // This will obviously have side-effects for any subsequent tests.
> +  MinimalIDService._delegates["firefox-accounts"] = new MockFXA();

Can you restore the previous FirefoxAccountsDelegate?

Comment 62

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #61)
> Comment on attachment 8339622 [details] [diff] [review]
> Part 1, refactored: DOM API
> 
> Review of attachment 8339622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch Jed! :)

Thank you!  And thanks for the review

> Just a few comments.
> 
> We'll also need a DOM peer review.

Yes.

> ::: toolkit/identity/MinimalIdentity.jsm
> @@ +101,5 @@
> > +  // methods on the IdentityService.
> > +}
> > +
> > +PersonaDelegate.prototype = {
> > +  watch: function personaWatch(aRp) {
> 
> nit: no need to name function properties anymore. Here and in the rest of
> the patch.

I know, and a while back I started removing all my internal function names, but I was still getting meaningless tracebacks in some builds.  I can't remember which, now.  I'll get along with the times and remove the internal names.

> @@ +118,5 @@
> > +    log("sending identity-controller-unwatch for id",
> > +        options.id, options.origin);
> > +    Services.obs.notifyObservers({ wrappedJSObject: options },
> > +                                 "identity-controller-unwatch", null);
> > +
> 
> uber-nit: remove extra line

ok

> @@ +175,5 @@
> > +
> > +  logout: function fxAccountsLogout(aRp) {
> > +    // For now, it makes no sense to logout from an specific RP in
> > +    // Firefox Accounts, so we just directly call the onlogout callback.
> > +    this.doLogout(aRp.id);
> 
> this.context.doLogout?

Oops.  Thanks.

> @@ +202,5 @@
> > +  try {
> > +    this._fxaEnabled =
> > +      Services.prefs.getPrefType(PREF_FXA_ENABLED) == Ci.nsIPrefBranch.PREF_BOOL
> > +      && Services.prefs.getBoolPref(PREF_FXA_ENABLED);
> > +    log("Firefox Accounts is " + (this._fxaEnabled ? "" : "not") + " enabled");
> 
> Bail out here if fxa is not enabled

ok

> @@ +225,5 @@
> > +  }
> > +
> > +  // RPs are required to specify the 'wantIssuer: <fxaissuer>' option to get
> > +  // Firefox Accounts assertions. The specific required issuer can be changed
> > +  // via identity.fxaccounts.issuer preference.
> 
> I wasn't sure about the need of this pref, so feel free to get rid of it if
> you consider that it is not needed.

I think we want to be able to change it so that we can point the device at a different fxa server for testing purposes.

> @@ +260,3 @@
> >      }
> > +
> > +    if (rp.wantIssuer == "firefox-accounts") {
> 
> if (rp.wantIssuer == this._fxaIssuer)
> 
> even if you decide to get rid of the identity.fxaccounts.issuer pref, that
> should be a const

ok

> @@ +271,5 @@
> > +        throw new Error("Non certified apps are not allowed to get " +
> > +                        "Firefox Accounts assertions");
> > +      }
> > +      return this._delegates["firefox-accounts"];
> > +    } 
> 
> nit: trailing white space

Oops.

> ::: toolkit/identity/tests/unit/head_identity.js
> @@ +121,5 @@
> > +  mockedDoc.wantIssuer = "firefox-accounts";
> > +
> > +  mockedDoc.doReady = partial(aDoFunc, "ready");
> > +  mockedDoc.doLogin = partial(aDoFunc, "login");
> > +
> 
> It would be great if we could also test logout

Yes, it would have caught that bug you found above :)  I'll add it.

> ::: toolkit/identity/tests/unit/test_minimalidentity.js
> @@ +169,5 @@
> > + */
> > +
> > +function test_fxa_watch() {
> > +  do_test_pending();
> > +  
> 
> nit: white space

ok

> @@ +189,5 @@
> > +    // callback.  Now we can call request();
> > +    MinimalIDService.RP.request(mockedRP.id);
> > +  });
> > +
> > +  // Mock the Firefox Accounts Manager
> 
> This is not a mock for FxAccountsManager, but for FirefoxAccountsDelegate

Indeed.  Thanks.

> @@ +203,5 @@
> > +  }
> > +
> > +  // Replace the firefox accounts delegate in the Identity Service.
> > +  // This will obviously have side-effects for any subsequent tests.
> > +  MinimalIDService._delegates["firefox-accounts"] = new MockFXA();
> 
> Can you restore the previous FirefoxAccountsDelegate?

Yes.  I'll see about restoring it if the test crashes, as well.  I thought there was some kind of 'finally' clause you could invoke in xpcshell tests.  I'll have a look.

Thanks for the review, ferjm!  New patch on its way.

Comment 63

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #62)
> (In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC,
> please) from comment #61)
> > Comment on attachment 8339622 [details] [diff] [review]
> > @@ +225,5 @@
> > > +  }
> > > +
> > > +  // RPs are required to specify the 'wantIssuer: <fxaissuer>' option to get
> > > +  // Firefox Accounts assertions. The specific required issuer can be changed
> > > +  // via identity.fxaccounts.issuer preference.
> > 
> > I wasn't sure about the need of this pref, so feel free to get rid of it if
> > you consider that it is not needed.
> 
> I think we want to be able to change it so that we can point the device at a
> different fxa server for testing purposes.

I'm sorry, you are quite right.  I was thinking about the server url, which is completely different.

Yes, I think we can remove the pref for this.

Comment 64

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

smaug, this is the DOM module that exposes the Firefox Accounts API for b2g apps.  Would you mind reviewing it?  Thank you!

Comment 65

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #59)
>  > @@ +85,5 @@
> > > +        if (user) {
> > > +          return Promise.reject({
> > > +            error: 'ALREADY_SIGNED_IN_USER',
> > > +            details: {
> > > +              user: this._user
> > 
> > is this._user going to be set here?  And is this really an error condition? 
> > eg, maybe the request should be silently ignored, or we should just
> > re-attempt a login anyway?
> >
> 
> I'm not sure that I am understanding your question. this._user is a getter,
> so it won't be set.

Exactly - so rejecting the promise with {user: this._user} doesn't seem the right thing to do here.

> > @@ +281,5 @@
> > > +
> > > +        // If there is no currently signed in user, we trigger the signIn UI flow.
> > > +        let ui = Cc['@mozilla.org/fxaccounts/fxaccounts-ui-glue;1']
> > > +                   .createInstance(Ci.nsIFxAccountsUIGlue);
> > > +        if (!ui) {
> > 
> > will ui ever be null here? IIUC, an exception will be thrown on error.
> > 
> 
> I guess that depends on the implementation of nsIFxAccountsUIGlue. It can be
> null, so I feel safer keeping this check :)

I don't understand how this can be the case.  If the component is registered and it implements nsIFxAccountsUIGlue, the result will be non-null.  If either of those things is *not* true, an exception will be raised (either one complaining the component couldn't be created, or one saying no such interface.)

All your other comments make sense, thanks.

Comment 66

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mark Hammond [:markh] from comment #65)
> (In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC,
> please) from comment #59)
> >  > @@ +85,5 @@
> > > > +        if (user) {
> > > > +          return Promise.reject({
> > > > +            error: 'ALREADY_SIGNED_IN_USER',
> > > > +            details: {
> > > > +              user: this._user
> > > 
> > > is this._user going to be set here?  And is this really an error condition? 
> > > eg, maybe the request should be silently ignored, or we should just
> > > re-attempt a login anyway?
> > >
> > 
> > I'm not sure that I am understanding your question. this._user is a getter,
> > so it won't be set.
> 
> Exactly - so rejecting the promise with {user: this._user} doesn't seem the
> right thing to do here.
> 

Oh, I think I know what you mean! "details.user" is being properly set with the value returned by the "this._user" getter.

> > > @@ +281,5 @@
> > > > +
> > > > +        // If there is no currently signed in user, we trigger the signIn UI flow.
> > > > +        let ui = Cc['@mozilla.org/fxaccounts/fxaccounts-ui-glue;1']
> > > > +                   .createInstance(Ci.nsIFxAccountsUIGlue);
> > > > +        if (!ui) {
> > > 
> > > will ui ever be null here? IIUC, an exception will be thrown on error.
> > > 
> > 
> > I guess that depends on the implementation of nsIFxAccountsUIGlue. It can be
> > null, so I feel safer keeping this check :)
> 
> I don't understand how this can be the case.  If the component is registered
> and it implements nsIFxAccountsUIGlue, the result will be non-null.  If
> either of those things is *not* true, an exception will be raised (either
> one complaining the component couldn't be created, or one saying no such
> interface.)
> 

The implementation of nsIFxAccountsUIGlue could return null in the constructor. But I agree that it is quite unlikely. I'll remove the check for null.

> All your other comments make sense, thanks.

Thanks Mark! I'll update the patch and ask for your r? in a few minutes

Comment 67

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Previous feedback addressed, fixed some bugs and added a more detailed error handling based on https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format

Comment 68

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue

Comment 69

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service

Comment 70

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: FxAccountsManager tests

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8340053 [details] [diff] [review]
Part 1: DOM API

>-    // Check for required callbacks
>-    let requiredCallbacks = ["onlogin", "onlogout"];
>-    for (let cbName of requiredCallbacks) {
>-      if ((!(cbName in aOptions))
>-          || typeof(aOptions[cbName]) !== "function") {
>-           throw new Error(cbName + " callback is required.");
>-         }
>-    }
>-
>-    // Optional callback "onready"
>-    if (aOptions["onready"]
>-        && typeof(aOptions['onready']) !== "function") {
>-      throw new Error("onready must be a function");
>-    }

As discussed on IRC, do this change in a followup. It isn't really needed in this bug

r+ for the dom/*

Comment 72

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

Thank you for your review, smaug!

Reintroduced checks for required params to watch() per Comment 71.  Opened follow-up bug 945363 to track API changes that may evolve in the coming months.

This patch now needs a toolkit review.

Comment 74

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8341052 [details] [diff] [review]
Part 4: FxAccountsManager tests

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

LTGM

Comment 75

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8341049 [details] [diff] [review]
Part 0: FxAccountsManager

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

LGTM

::: services/fxaccounts/FxAccountsManager.jsm
@@ +151,5 @@
> +    return this._signInSignUp("signUp", aAccountId, aPassword);
> +  },
> +
> +  signOut: function() {
> +    if (Services.io.offline) {

It seems a shame we can't signOut while offline - can't we drop the session anyway?

Comment 76

[Fernando Jiménez Moreno [:ferjm]]

Thanks Mark!

(In reply to Mark Hammond [:markh] from comment #75)
> 
> ::: services/fxaccounts/FxAccountsManager.jsm
> @@ +151,5 @@
> > +    return this._signInSignUp("signUp", aAccountId, aPassword);
> > +  },
> > +
> > +  signOut: function() {
> > +    if (Services.io.offline) {
> 
> It seems a shame we can't signOut while offline - can't we drop the session
> anyway?

According to comment 50, we need to use the client to delete the server auth session. I am not sure if removing the local data will leave us in an unstable situation. Zack might know.

Comment 77

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #76)
> According to comment 50, we need to use the client to delete the server auth
> session. I am not sure if removing the local data will leave us in an
> unstable situation. Zack might know.

Hopefully that is more of a "should" - it would seem a problem if we (eg) started up having lost the local auth store and we were in an unstable situation.

Comment 78

[Zachary Carter [:zaach]]

(In reply to Mark Hammond [:markh] from comment #77)
> (In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC,
> please) from comment #76)
> > According to comment 50, we need to use the client to delete the server auth
> > session. I am not sure if removing the local data will leave us in an
> > unstable situation. Zack might know.
> 
> Hopefully that is more of a "should" - it would seem a problem if we (eg)
> started up having lost the local auth store and we were in an unstable
> situation.

Right, it should be fine. The client can create new sessions up to the limit (100?). Orphaned tokens on the server will eventually be garbage collected.

Comment 79

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: FxAccountsManager

Ok, thanks!

We can now sign out in offline mode.

r=markh

Comment 80

[[:fabrice] Fabrice Desré]

Comment on attachment 8341050 [details] [diff] [review]
Part 2: UI Glue

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

I'd like to see an updated version.

::: b2g/components/FxAccountsUIGlue.js
@@ +30,5 @@
> +
> +    let event = content.document.createEvent("CustomEvent");
> +    event.initCustomEvent("mozFxAccountsRPChromeEvent", true, true,
> +                          aMsg ? ObjectWrapper.wrap(aMsg, content) : {});
> +    content.dispatchEvent(event);

Please use https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#538 instead.

@@ +40,5 @@
> +    let deferred = Promise.defer();
> +
> +    let content = this._browser.getContentWindow();
> +    if (!content) {
> +      deferred.reject("INTERNAL_ERROR_NO_CONTENT");

CamelCase is preferred to CAPITAL_SNAKE.

@@ +49,5 @@
> +
> +    content.addEventListener("mozFxAccountsRPContentEvent", function(result) {
> +      let msg = result.detail;
> +      if (!msg || !msg.id) {
> +        deferred.reject("INTERNAL_ERROR_NO_RESULT");

Here too.

@@ +61,5 @@
> +      if (msg.error) {
> +        deferred.reject(msg);
> +      } else {
> +        deferred.resolve(msg.result);
> +      }

Why do we never remove the event listener?

Comment 81

[[:fabrice] Fabrice Desré]

Comment on attachment 8341051 [details] [diff] [review]
Part 3: B2G FxA Service

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

Nothing major to fix, but I'd like to take a second look.

::: b2g/chrome/content/shell.js
@@ +681,5 @@
>      window.addEventListener("ContentStart", (function(evt) {
>        let content = shell.contentBrowser.contentWindow;
>        content.addEventListener("mozContentEvent", this, false, true);
> +      content.addEventListener("mozFxAccountsContentEvent", FxAccountsMgmtService,
> +                               false, true);

Can you move that to  FxAccountsMgmtService.jsm?

::: b2g/components/FxAccountsMgmtService.jsm
@@ +38,5 @@
> +    let event = content.document.createEvent("CustomEvent");
> +    event.initCustomEvent("mozFxAccountsChromeEvent", true, true,
> +                          aMsg ? ObjectWrapper.wrap(aMsg, content) : {});
> +    content.dispatchEvent(event);
> +  },

That can be replaced by https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#538

@@ +92,5 @@
> +          },
> +          reason => {
> +            this._onReject(msg.id, reason);
> +          }
> +        );

why no .then(null, Components.utils.reportError) here?

@@ +103,5 @@
> +          },
> +          reason => {
> +            this._onReject(msg.id, reason);
> +          }
> +        );

and here?

Comment 82

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: UI Glue

Comment 83

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service

Comment 84

[Matthew N. [:MattN]]

Comment on attachment 8341204 [details] [diff] [review]
Part 1: DOM API

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

::: dom/identity/nsDOMIdentity.js
@@ +381,5 @@
>  
>      // Store window and origin URI.
>      this._window = aWindow;
>      this._origin = aWindow.document.nodePrincipal.origin;
> +    this._appStatus = aWindow.document.nodePrincipal.appStatus;

Technically you should get a DOM peer to review this although it seems straightforward and the same as how Webapps.js does this.

@@ +544,5 @@
> +    // APP_STATUS_NOT_INSTALLED = 0
> +    // APP_STATUS_INSTALLED     = 1
> +    // APP_STATUS_PRIVILEGED    = 2
> +    // APP_STATUS_CERTIFIED     = 3
> +    message.appStatus = this._appStatus;

This comment may become stale so just reference nsIPrincipal.appStatus and/or nsIPrincipal.APP_STATUS_* for people to find out more.

::: toolkit/identity/MinimalIdentity.jsm
@@ +14,5 @@
>   */
>  
>  "use strict";
>  
> +this.EXPORTED_SYMBOLS = ["IdentityService", "FirefoxAccountsDelegate"];

It seems like this module could be left alone to handle Persona and a new module could be created for FxA. The logic to choose which JSM to use could be in DOMIdentity.jsm. The new FxA module could import (Minimal)Identity.jsm like how DOMIdentity.jsm does to get access to the "context" that is being passed to FirefoxAccountsDelegate below. 

This would provide a cleaner separation for FxA DOM API calls from Persona ones. It also gets rid of the extra layer of indirection that PersonaDelegate adds. Also, with this approach, this module won't diverge from Identity.jsm (non-minimal) which I believe is still a goal to switch to (see bug 929243).

I was going to prototype this but I didn't see tests using the DOM APIs that would help confirm that the prototype was equivalent and this is a concern. Something like bug 805602's test_relying_party.html would be good for FxA to ensure things don't break.

Let me know if you have issues with this approach.

@@ +36,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this,
> +                                  "FxAccountsManager",
> +                                  "resource://gre/modules/FxAccountsManager.jsm");
> +
> +const PREF_ALLOW_FXA = "identity.fxaccounts.allow-non-certified";

This constant name doesn't indicate that it's related to non-certified apps. e.g. PREF_FXA_ALLOW_NON_CERTIFIED would be better.

@@ +39,5 @@
> +
> +const PREF_ALLOW_FXA = "identity.fxaccounts.allow-non-certified";
> +const PREF_FXA_ENABLED = "identity.fxaccounts.enabled";
> +const FXA_ISSUER = "firefox-accounts";
> +const PERSONA = "persona";

I'm not sure the last two constants are really necessary but if you want to keep them, give them a common prefix to describe the purpose. e.g. ISSUER_FXA/ISSUER_PERSONA (or should it be BROWSERID?) or DELEGATE_FXA/DELEGATE_….

Nit: move the consts up with the others (leaving a new line to separate these from the Components.* aliases)?

@@ +266,5 @@
>     * @param aCaller
>     *        (Object)  an object that represents the caller document, and
>     *                  is expected to have properties:
>     *                  - id (unique, e.g. uuid)
>     *                  - loggedInUser (string or null)

I think you want to delete this line since it's duplicated below

@@ +298,1 @@
>      this._rpFlows[aRpCaller.id] = aRpCaller;

Doesn't this mean that FXA and Persona flows will clobber each-other when they're from the same window? It seems like there could be potential for security issues with the fact that the rpFlows are stored together. This is another reason why I think DOMIdentity.jsm should decide whether a FXA or Persona JSM should handle the nav.id method calls.

::: toolkit/identity/tests/unit/head_identity.js
@@ +225,5 @@
>  // Switch debug messages on by default
>  Services.prefs.setBoolPref("toolkit.identity.debug", true);
> +
> +// Enable Firefox Accounts
> +Services.prefs.setBoolPref("identity.fxaccounts.enabled", true);

You should use do_register_cleanup do register a cleanup function to reset this change afterwards with clearUserPref. Optionally do the same for toolkit.identity.debug which should be doing this too.

Comment 85

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G FxA Service

Fabrice, this should address your previous review feedback. I've filed Bug 948453 for what we spoke about shell.js
Thanks!

Comment 86

[[:fabrice] Fabrice Desré]

Comment on attachment 8344978 [details] [diff] [review]
Part 2: UI Glue

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

r=me with nits addressed. Feel free to not change the method name.

::: b2g/components/FxAccountsUIGlue.js
@@ +36,5 @@
> +    content.addEventListener("mozFxAccountsRPContentEvent",
> +                             function onContentEvent(result) {
> +      let msg = result.detail;
> +      if (!msg || !msg.id) {
> +        deferred.reject("InternalErrorNoResult");

You need to remove the event listener.

@@ +40,5 @@
> +        deferred.reject("InternalErrorNoResult");
> +        return;
> +      }
> +
> +      if (msg.id != id) {

You need to reject the promise here too, and remove the event listener.

::: services/fxaccounts/interfaces/nsIFxAccountsUIGlue.idl
@@ +7,5 @@
> +[scriptable, uuid(5805ac8b-7cbe-4fbd-97ad-d3ae8cd29f79)]
> +interface nsIFxAccountsUIGlue : nsISupports
> +{
> +  // Returns a Promise.
> +  jsval signInFlow();

maybe startSignInFlow() ? No big deal anyway

Comment 87

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Matthew N. [:MattN] from comment #84)
> Comment on attachment 8341204 [details] [diff] [review]
> Part 1: DOM API

Thanks for your review, MattN, and for talking yesterday.

> Review of attachment 8341204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/identity/nsDOMIdentity.js
> @@ +381,5 @@
> >  
> >      // Store window and origin URI.
> >      this._window = aWindow;
> >      this._origin = aWindow.document.nodePrincipal.origin;
> > +    this._appStatus = aWindow.document.nodePrincipal.appStatus;
> 
> Technically you should get a DOM peer to review this although it seems
> straightforward and the same as how Webapps.js does this.

:smaug reviewed the dom/identity part of this patch in Comment 71.  But with the changes introduced in this revision, we will need to ask for another DOM review.

> @@ +544,5 @@
> > +    // APP_STATUS_NOT_INSTALLED = 0
> > +    // APP_STATUS_INSTALLED     = 1
> > +    // APP_STATUS_PRIVILEGED    = 2
> > +    // APP_STATUS_CERTIFIED     = 3
> > +    message.appStatus = this._appStatus;
> 
> This comment may become stale so just reference nsIPrincipal.appStatus
> and/or nsIPrincipal.APP_STATUS_* for people to find out more.

Agreed.  Fixed.

> ::: toolkit/identity/MinimalIdentity.jsm
> @@ +14,5 @@
> >   */
> >  
> >  "use strict";
> >  
> > +this.EXPORTED_SYMBOLS = ["IdentityService", "FirefoxAccountsDelegate"];
> 
> It seems like this module could be left alone to handle Persona and a new
> module could be created for FxA. The logic to choose which JSM to use could
> be in DOMIdentity.jsm. The new FxA module could import (Minimal)Identity.jsm
> like how DOMIdentity.jsm does to get access to the "context" that is being
> passed to FirefoxAccountsDelegate below. 
> 
> This would provide a cleaner separation for FxA DOM API calls from Persona
> ones. It also gets rid of the extra layer of indirection that
> PersonaDelegate adds. Also, with this approach, this module won't diverge
> from Identity.jsm (non-minimal) which I believe is still a goal to switch to
> (see bug 929243).
> 
> I was going to prototype this but I didn't see tests using the DOM APIs that
> would help confirm that the prototype was equivalent and this is a concern.
> Something like bug 805602's test_relying_party.html would be good for FxA to
> ensure things don't break.
> 
> Let me know if you have issues with this approach.

This sounds reasonable to me.  I've created a new dom/identity/FirefoxAccounts.jsm.  The DOMIdentity.jsm module now switches between Persona or Firefox Accounts by looking for the 'wantIssuer' flag.  I've added a small object for maintaining state on behalf of the different RPs, and indexing against the message managers as well as the RP ids (which are uuids).  This way we should be able to have the same RP use both Persona and Firefox Accounts if it wants to.

> @@ +36,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this,
> > +                                  "FxAccountsManager",
> > +                                  "resource://gre/modules/FxAccountsManager.jsm");
> > +
> > +const PREF_ALLOW_FXA = "identity.fxaccounts.allow-non-certified";
> 
> This constant name doesn't indicate that it's related to non-certified apps.
> e.g. PREF_FXA_ALLOW_NON_CERTIFIED would be better.

I've removed this pref.  We don't need it.

> @@ +39,5 @@
> > +
> > +const PREF_ALLOW_FXA = "identity.fxaccounts.allow-non-certified";
> > +const PREF_FXA_ENABLED = "identity.fxaccounts.enabled";
> > +const FXA_ISSUER = "firefox-accounts";
> > +const PERSONA = "persona";
> 
> I'm not sure the last two constants are really necessary but if you want to
> keep them, give them a common prefix to describe the purpose. e.g.
> ISSUER_FXA/ISSUER_PERSONA (or should it be BROWSERID?) or
> DELEGATE_FXA/DELEGATE_….

I've also removed many of these consts.  The DOMIdentity.jsm switch makes them unnecessary.

> Nit: move the consts up with the others (leaving a new line to separate
> these from the Components.* aliases)?

Ok

> @@ +266,5 @@
> >     * @param aCaller
> >     *        (Object)  an object that represents the caller document, and
> >     *                  is expected to have properties:
> >     *                  - id (unique, e.g. uuid)
> >     *                  - loggedInUser (string or null)
> 
> I think you want to delete this line since it's duplicated below

Ok

> @@ +298,1 @@
> >      this._rpFlows[aRpCaller.id] = aRpCaller;
> 
> Doesn't this mean that FXA and Persona flows will clobber each-other when
> they're from the same window? It seems like there could be potential for
> security issues with the fact that the rpFlows are stored together. This is
> another reason why I think DOMIdentity.jsm should decide whether a FXA or
> Persona JSM should handle the nav.id method calls.

Yes, this was a good catch.  Thank you.  I think the current approach should fix it.

> ::: toolkit/identity/tests/unit/head_identity.js
> @@ +225,5 @@
> >  // Switch debug messages on by default
> >  Services.prefs.setBoolPref("toolkit.identity.debug", true);
> > +
> > +// Enable Firefox Accounts
> > +Services.prefs.setBoolPref("identity.fxaccounts.enabled", true);
> 
> You should use do_register_cleanup do register a cleanup function to reset
> this change afterwards with clearUserPref. Optionally do the same for
> toolkit.identity.debug which should be doing this too.

Fixed.  Thanks.

Comment 88

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API (revised; WIP)

Hi, MattN,
Here is a work-in-progress as described in Comment 87.

The main change is the introduction of toolkit/identity/FirefoxAccounts.jsm, and the logic in dom/identity/DOMIdentity.jsm to select either Persona or Firefox Accounts.

Is this going in the direction you had in mind?  Thanks for looking, j

Comment 89

[Fernando Jiménez Moreno [:ferjm]]

The DOM API changes are not directly related with the other patches attached to this bug. I filed bug 949526 for them in order to land this as soon as possible to unblock the Gaia side landing.

Comment 90

[Fernando Jiménez Moreno [:ferjm]]

Just landed https://bugzilla.mozilla.org/show_bug.cgi?id=949526#c3

Comment 91

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 2.1-929386-fxa.alt.part0.revised.patch

Needed to add do_get_profile() to xpcshell tests.  Otherwise, FxAccounts.jsm crashes when trying to get its signedInUser.json file.

Comment 92

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API (revised)

Added tests for child process shutdown

Comment 93

[Matthew N. [:MattN]]

Comment on attachment 8347429 [details] [diff] [review]
Part 1: DOM API (revised)

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

::: dom/identity/DOMIdentity.jsm
@@ +23,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "objectCopy",
>                                    "resource://gre/modules/identity/IdentityUtils.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Persona",

Can we just leave this as IdentityService as I believe Identity.jsm is technically BrowserID and not really Persona the service.

@@ +163,5 @@
> +
> +  // If we receive a child-process-shutdown message, we will only know the
> +  // target message manager associated with it, so we have to keep an extra
> +  // mapping of message managers to RP ids.
> +  this.mms = new Map();

I'm still trying to figure out if this can be simplified. I'll come back to this later.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["IdentityService"];

Why not just FirefoxAccounts?

Comment 94

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Matthew N. [:MattN] from comment #93)
> Comment on attachment 8347429 [details] [diff] [review]
> Part 1: DOM API (revised)
> 
> Review of attachment 8347429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/identity/DOMIdentity.jsm
> @@ +23,5 @@
> >  
> >  XPCOMUtils.defineLazyModuleGetter(this, "objectCopy",
> >                                    "resource://gre/modules/identity/IdentityUtils.jsm");
> >  
> > +XPCOMUtils.defineLazyModuleGetter(this, "Persona",
> 
> Can we just leave this as IdentityService as I believe Identity.jsm is
> technically BrowserID and not really Persona the service.

Certainly

> @@ +163,5 @@
> > +
> > +  // If we receive a child-process-shutdown message, we will only know the
> > +  // target message manager associated with it, so we have to keep an extra
> > +  // mapping of message managers to RP ids.
> > +  this.mms = new Map();
> 
> I'm still trying to figure out if this can be simplified. I'll come back to
> this later.

Maybe I've got this wrong, but my thinking was that if an app dies or is killed, I'll get a child-process-shutdown message with the message manager attached but not the RP ID.  Since it's conceivable that the same RP could have both a Persona and a Firefox Accounts flow, I need to be able to look up the flows associated with the message manager and clean them up.

I could make this simpler by iterating through all the contexts and checking the message manager on shutdown.  That would be less code and probably easier to deal with.

> ::: toolkit/identity/FirefoxAccounts.jsm
> @@ +3,5 @@
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +this.EXPORTED_SYMBOLS = ["IdentityService"];
> 
> Why not just FirefoxAccounts?

Ok

Comment 95

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API (revised)

Comment 96

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

- added a test to show that browserid and fxa flows can coexist in the same RP
- took a stab at cleaning up the message manager question from Comment 93

Comment 97

[Matthew N. [:MattN]]

Comment on attachment 8347498 [details] [diff] [review]
Part 1: DOM API (revised)

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

::: dom/identity/DOMIdentity.jsm
@@ +169,5 @@
> +    this.contexts.set(message.id, context);
> +    return context;
> +  },
> +
> +  getServicesForMM: function(targetMM) {

This is returning an array of contexts so shouldn't it be getContextsForMM?

@@ +179,5 @@
> +    }
> +    return services;
> +  },
> +
> +  deleteServicesForMM: function(targetMM) {

Likewise

@@ +321,5 @@
>  
>    _childProcessShutdown: function DOMIdentity__childProcessShutdown(targetMM) {
> +    this.idService.getServicesForMM(targetMM).forEach((service) => {
> +      try {
> +        service.RP.childProcessShutdown(targetMM);

If getServicesForMM is actually returning a context, I guess this won't work?
Would it be a problem to call childProcessShutdown on both services? Then we can get rid of IdentityServiceState except for getService which can just live on DOMIdentity which is what I originally had in mind.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +167,5 @@
> +      if (this._rpFlows.get(key)._mm === messageManager) {
> +        this._rpFlows.delete(key);
> +      }
> +    }
> +    Services.obs.notifyObservers({wrappedJSObject: options}, "identity-child-process-shutdown", null);

This is the same notification as in MinimalIdentity so can't this notifyObservers call be done once in DOMIdentity__childProcessShutdown?

Comment 98

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

Comment 99

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Matthew N. [:MattN] from comment #97)
> Comment on attachment 8347498 [details] [diff] [review]
> Part 1: DOM API (revised)

Thanks for your comments, MattN

> ::: dom/identity/DOMIdentity.jsm
> @@ +169,5 @@
> > +    this.contexts.set(message.id, context);
> > +    return context;
> > +  },
> > +
> > +  getServicesForMM: function(targetMM) {
> 
> This is returning an array of contexts so shouldn't it be getContextsForMM?

By addressing your other comments, I was able to remove this.

> @@ +179,5 @@
> > +    }
> > +    return services;
> > +  },
> > +
> > +  deleteServicesForMM: function(targetMM) {
> 
> Likewise

Likewise :)

> @@ +321,5 @@
> >  
> >    _childProcessShutdown: function DOMIdentity__childProcessShutdown(targetMM) {
> > +    this.idService.getServicesForMM(targetMM).forEach((service) => {
> > +      try {
> > +        service.RP.childProcessShutdown(targetMM);
> 
> If getServicesForMM is actually returning a context, I guess this won't work?
> Would it be a problem to call childProcessShutdown on both services? Then we
> can get rid of IdentityServiceState except for getService which can just
> live on DOMIdentity which is what I originally had in mind.

I've moved the state-keeping back into DOMIdentity, according to your original plan.  I feel much better with it there, too.

I did, nevertheless, keep one map for {windowID: context} and another for {messageManager: windowID}.  Since the childProcessShutdown message does not carry the window ID with it, this still seems necessary.

> ::: toolkit/identity/FirefoxAccounts.jsm
> @@ +167,5 @@
> > +      if (this._rpFlows.get(key)._mm === messageManager) {
> > +        this._rpFlows.delete(key);
> > +      }
> > +    }
> > +    Services.obs.notifyObservers({wrappedJSObject: options}, "identity-child-process-shutdown", null);
> 
> This is the same notification as in MinimalIdentity so can't this
> notifyObservers call be done once in DOMIdentity__childProcessShutdown?

Done.

Everything is vastly simplified.

Having spoken to :spenrose, we've determined that we do not want to make it possible to call watch() for both BrowserID and Firefox Accounts at this stage.  We merely want to support certified apps on FirefoxOS, which will all use Firefox Accounts for authentication.  At some point in the future, we may want to support the choice for packaged apps, but that's not in the MVP we're trying to build at this stage.

Our highest priority is to have this DOM patch functional Marketplace and Wheres-My-Fox.  We can iterate at greater leisure from there.

Thanks for taking a look!
j

Comment 100

[Matthew N. [:MattN]]

Comment on attachment 8361408 [details] [diff] [review]
Part 1: DOM API

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

I not a DOM peer so this may need one to review it although the changes are mostly just identity team related.

Please refine the commit message to make it more descriptive/accurate as this isn't really about a FxA DOM Component.

::: dom/identity/tests/moz.build
@@ +3,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/.
>  
> +TEST_DIRS += []

I think you can revert this file?

::: toolkit/identity/IdentityUtils.jsm
@@ +12,5 @@
>    "checkDeprecated",
>    "checkRenamed",
>    "getRandomId",
> +  "objectCopy",
> +  "makeMessageObject"

Nit: Add the trailing comma to avoid changing blame whenever a test gets appended.

::: toolkit/identity/moz.build
@@ +30,5 @@
>      'Sandbox.jsm',
>  ]
>  
> +EXTRA_PP_JS_MODULES += [
> +    'FirefoxAccounts.jsm'

Nit: Add the trailing comma to avoid changing blame whenever a test gets appended.

::: toolkit/identity/tests/unit/head_identity.js
@@ +113,5 @@
>    mockedDoc.doCancel = partial(aDoFunc, 'cancel');
>    mockedDoc.doCoffee = partial(aDoFunc, 'coffee');
> +  mockedDoc.childProcessShutdown = partial(aDoFunc, 'child-process-shutdown');
> +
> +  mockedDoc.RP = mockedDoc; 

Nit: trailing whitespace

::: toolkit/identity/tests/unit/tail_identity.js
@@ +1,5 @@
>  
>  // pre-emptively shut down to clear resources
>  if (typeof IdentityService !== "undefined") {
> +  if (typeof IdentityService.shutdown == "function") {
> +    IdentityService.shutdown();

What's this about?

::: toolkit/identity/tests/unit/test_firefox_accounts.js
@@ +1,1 @@
> +"use strict";

This test file is missing the creative commons header.

@@ +22,5 @@
> +
> +let originalManager = FirefoxAccounts.fxAccountsManager;
> +FirefoxAccounts.fxAccountsManager = new MockFXAManager();
> +do_register_cleanup(() => {
> +  dump("restoring fxaccountsmanager");

Convert to info(…) or remove this? (It's missing a newline for dump)

@@ +154,5 @@
> +  test_mock,
> +  test_watch,
> +  test_request,
> +  test_logout,
> +  test_child_process_shutdown

Nit: Add the trailing comma to avoid changing blame whenever a test gets appended.

::: toolkit/identity/tests/unit/test_minimalidentity.js
@@ +1,1 @@
>  "use strict";

Btw, this test file is missing the creative commons header.

@@ +209,5 @@
>    test_logout,
>    test_logoutBeforeWatch,
>    test_requestBeforeWatch,
> +  test_unwatchBeforeWatch,
> +  test_childProcessShutdown

Nit: Add the trailing comma to avoid changing blame whenever a test gets appended.

Comment 101

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Matthew N. [:MattN] from comment #100)

Thanks for the speedy review, MattN!

> Comment on attachment 8361408 [details] [diff] [review]
> Part 1: DOM API
> 
> Review of attachment 8361408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I not a DOM peer so this may need one to review it although the changes are
> mostly just identity team related.

Ok.  I'll check with :smaug again.  He kindly reviewed the dom bits before.

> Please refine the commit message to make it more descriptive/accurate as
> this isn't really about a FxA DOM Component.

Ok

> ::: dom/identity/tests/moz.build
> @@ +3,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/.
> >  
> > +TEST_DIRS += []
> 
> I think you can revert this file?

Oops.  yes.

> ::: toolkit/identity/IdentityUtils.jsm
> @@ +12,5 @@
> >    "checkDeprecated",
> >    "checkRenamed",
> >    "getRandomId",
> > +  "objectCopy",
> > +  "makeMessageObject"
> 
> Nit: Add the trailing comma to avoid changing blame whenever a test gets
> appended.

Ok

> ::: toolkit/identity/moz.build
> @@ +30,5 @@
> >      'Sandbox.jsm',
> >  ]
> >  
> > +EXTRA_PP_JS_MODULES += [
> > +    'FirefoxAccounts.jsm'
> 
> Nit: Add the trailing comma to avoid changing blame whenever a test gets
> appended.

Yes

> ::: toolkit/identity/tests/unit/head_identity.js
> @@ +113,5 @@
> >    mockedDoc.doCancel = partial(aDoFunc, 'cancel');
> >    mockedDoc.doCoffee = partial(aDoFunc, 'coffee');
> > +  mockedDoc.childProcessShutdown = partial(aDoFunc, 'child-process-shutdown');
> > +
> > +  mockedDoc.RP = mockedDoc; 
> 
> Nit: trailing whitespace

Fixed

> ::: toolkit/identity/tests/unit/tail_identity.js
> @@ +1,5 @@
> >  
> >  // pre-emptively shut down to clear resources
> >  if (typeof IdentityService !== "undefined") {
> > +  if (typeof IdentityService.shutdown == "function") {
> > +    IdentityService.shutdown();
> 
> What's this about?

Ah.  There wasn't a shutdown() method in MinimalIdentity.  I've added it so there's parity now.

> ::: toolkit/identity/tests/unit/test_firefox_accounts.js
> @@ +1,1 @@
> > +"use strict";
> 
> This test file is missing the creative commons header.

Added

> @@ +22,5 @@
> > +
> > +let originalManager = FirefoxAccounts.fxAccountsManager;
> > +FirefoxAccounts.fxAccountsManager = new MockFXAManager();
> > +do_register_cleanup(() => {
> > +  dump("restoring fxaccountsmanager");
> 
> Convert to info(…) or remove this? (It's missing a newline for dump)

Oops.  yes.

> @@ +154,5 @@
> > +  test_mock,
> > +  test_watch,
> > +  test_request,
> > +  test_logout,
> > +  test_child_process_shutdown
> 
> Nit: Add the trailing comma to avoid changing blame whenever a test gets
> appended.

Fixed

> ::: toolkit/identity/tests/unit/test_minimalidentity.js
> @@ +1,1 @@
> >  "use strict";
> 
> Btw, this test file is missing the creative commons header.

Fixed

> @@ +209,5 @@
> >    test_logout,
> >    test_logoutBeforeWatch,
> >    test_requestBeforeWatch,
> > +  test_unwatchBeforeWatch,
> > +  test_childProcessShutdown
> 
> Nit: Add the trailing comma to avoid changing blame whenever a test gets
> appended.

Fixed

Comment 102

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

r=MattN

:smaug, thanks for looking at this in an earlier revision.  It's changed a bit and we'd like to get a DOM review before landing.

Thanks!
j

Comment 103

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8361988 [details] [diff] [review]
Part 1: DOM API

># HG changeset patch
># User Jed Parsons <[e-mail]>
># Date 1386977483 28800
>#      Fri Dec 13 15:31:23 2013 -0800
># Node ID bddd8c31128e0eb130967efef04fd54284144eb8
># Parent  bbe08810e87b1afa84299d15c5e06539fb736b58
>Bug 929386 - BrowserID DOM API extension for Firefox Accounts; r=MattN,smaug
>
>diff --git a/dom/identity/DOMIdentity.jsm b/dom/identity/DOMIdentity.jsm
>--- a/dom/identity/DOMIdentity.jsm
>+++ b/dom/identity/DOMIdentity.jsm
>@@ -1,15 +1,24 @@
> /* 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";
> 
> const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>+const PREF_FXA_ENABLED = "identity.fxaccounts.enabled";
>+let _fxa_enabled = false;
>+try {
>+  _fxa_enabled =
>+    Services.prefs.getPrefType(PREF_FXA_ENABLED) === Ci.nsIPrefBranch.PREF_BOOL
>+    && Services.prefs.getBoolPref(PREF_FXA_ENABLED);
&& to the end up the previous line


>@@ -99,16 +114,18 @@ function RPWatchContext(aOptions, aTarge
>   }
> 
>   // default for no loggedInUser is undefined, not null
>   this.loggedInUser = aOptions.loggedInUser;
> 
>   // Maybe internal
>   this._internal = aOptions._internal;
> 
>+  this.audience = this.origin;
>+
I don't see this.audience used anywhere. What is it, and why we have this.origin and this.audience pointing
to the same value?

> this.DOMIdentity = {
>+  _serviceContexts: new Map(),
>+  _mmContexts: new Map(),
What are these contexts. This needs some comments.


>+
>+  newContext: function(message, targetMM) {
>+    let context = new RPWatchContext(message, targetMM);
>+    this._serviceContexts.set(message.id, context);
>+    this._mmContexts.set(targetMM, message.id);
>+  },
>+
>+  getContext: function(message) {
I think "context" is too vague here. What context?

>+  getContextForMM: function(targetMM) {
>+    return this._serviceContexts.get(this._mmContexts.get(targetMM));
>+  },
>+
>+  deleteContextForMM: function(targetMM) {
>+    this._serviceContexts.delete(this._mmContexts.get(targetMM));
>+    this._mmContexts.delete(targetMM);
>+  },
Same here.
>+      // For the initial release of Firefox Accounts, we support callers who
>+      // invoke watch() either for Firefox Acccounts, or Persona, but not both.
Acccounts

>+      // In the future, we may wish to support the dual invocation (say, for
>+      // packaged apps so they can sign users in who reject the app's request
>+      // to sign in with their Firefox Accounts identity).
>       throw new Error("navigator.id.watch was already called");
>     }
> 
>     if (!aOptions || typeof(aOptions) !== "object") {
>       throw new Error("options argument to watch is required");
>     }
> 
>     // Check for required callbacks
>+    //
>+    // XXX Bug 945363 - The requirement of the "onlogout"
>+    // callback may change in Q1 2015.
2015? really?
>diff --git a/toolkit/identity/FirefoxAccounts.jsm b/toolkit/identity/FirefoxAccounts.jsm
This stuff need toolkit peer review

Comment 104

[Matthew N. [:MattN]]

(In reply to Olli Pettay [:smaug] from comment #103)
> Comment on attachment 8361988 [details] [diff] [review]
> Part 1: DOM API
>
> >diff --git a/toolkit/identity/FirefoxAccounts.jsm b/toolkit/identity/FirefoxAccounts.jsm
> This stuff need toolkit peer review

I did the toolkit review (dolske assigned it to me) so according to the toolkit review rules, that was fine.

Comment 105

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Olli Pettay [:smaug] from comment #103)
> Comment on attachment 8361988 [details] [diff] [review]
> Part 1: DOM API

Thank you for your comments, smaug.

I agree that the code needed much more explanation in comments.  I have added many comments to explain what is going on with the API and flows.

> >diff --git a/dom/identity/DOMIdentity.jsm b/dom/identity/DOMIdentity.jsm
> >--- a/dom/identity/DOMIdentity.jsm
> >+++ b/dom/identity/DOMIdentity.jsm
> >@@ -1,15 +1,24 @@
> > /* 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";
> > 
> > const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> >+const PREF_FXA_ENABLED = "identity.fxaccounts.enabled";
> >+let _fxa_enabled = false;
> >+try {
> >+  _fxa_enabled =
> >+    Services.prefs.getPrefType(PREF_FXA_ENABLED) === Ci.nsIPrefBranch.PREF_BOOL
> >+    && Services.prefs.getBoolPref(PREF_FXA_ENABLED);
> && to the end up the previous line

I've broken the expression into an if-statement and an assignment in order to make the lines slightly shorter.

> 
> >@@ -99,16 +114,18 @@ function RPWatchContext(aOptions, aTarge
> >   }
> > 
> >   // default for no loggedInUser is undefined, not null
> >   this.loggedInUser = aOptions.loggedInUser;
> > 
> >   // Maybe internal
> >   this._internal = aOptions._internal;
> > 
> >+  this.audience = this.origin;
> >+
> I don't see this.audience used anywhere. What is it, and why we have
> this.origin and this.audience pointing
> to the same value?

Currently, that value is used by a hosted js shim on b2g.  

They are the same value by default because the assertion audience will normally be the same as the origin of an RP.  In the future, however, certified apps will be able to specify their own origins; hence we have begun to make the distinction here.

> > this.DOMIdentity = {
> >+  _serviceContexts: new Map(),
> >+  _mmContexts: new Map(),
> What are these contexts. This needs some comments.

I completely agree.  I've provided comments.

> 
> >+
> >+  newContext: function(message, targetMM) {
> >+    let context = new RPWatchContext(message, targetMM);
> >+    this._serviceContexts.set(message.id, context);
> >+    this._mmContexts.set(targetMM, message.id);
> >+  },
> >+
> >+  getContext: function(message) {
> I think "context" is too vague here. What context?

You're right.  I've renamed getContext to getService, since it returns either the IdentityService or FirefoxAccounts service, depending on the RPs invocation of watch().

> >+  getContextForMM: function(targetMM) {
> >+    return this._serviceContexts.get(this._mmContexts.get(targetMM));
> >+  },
> >+
> >+  deleteContextForMM: function(targetMM) {
> >+    this._serviceContexts.delete(this._mmContexts.get(targetMM));
> >+    this._mmContexts.delete(targetMM);
> >+  },
> Same here.

Comments added

> >+      // For the initial release of Firefox Accounts, we support callers who
> >+      // invoke watch() either for Firefox Acccounts, or Persona, but not both.
> Acccounts

Thank you, fixed

> >+      // In the future, we may wish to support the dual invocation (say, for
> >+      // packaged apps so they can sign users in who reject the app's request
> >+      // to sign in with their Firefox Accounts identity).
> >       throw new Error("navigator.id.watch was already called");
> >     }
> > 
> >     if (!aOptions || typeof(aOptions) !== "object") {
> >       throw new Error("options argument to watch is required");
> >     }
> > 
> >     // Check for required callbacks
> >+    //
> >+    // XXX Bug 945363 - The requirement of the "onlogout"
> >+    // callback may change in Q1 2015.
> 2015? really?

Thank you for the catch.  It was indeed 2014.  I've added comments explaining the slightly differing requirements for api usage by regular old persona RPs and our Firefox Accounts RPs.  I've also amended the comment to point to the BrowserID spec (which the persona team is still working on revising).

> >diff --git a/toolkit/identity/FirefoxAccounts.jsm b/toolkit/identity/FirefoxAccounts.jsm
> This stuff need toolkit peer review

Yes.  And as MattN says in Comment 104, he has given the toolkit parts r+, having beenn assigned to  review them by dolske.

Thank you,
j

Comment 106

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

Addresses issues from Comment 103

Comment 107

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8363130 [details] [diff] [review]
Part 1: DOM API


>+  /*
>+   * Create a new RPWatchContext, and update the context maps.
>+   */
>+  newContext: function(message, targetMM) {
>+    let context = new RPWatchContext(message, targetMM);
>+    this._serviceContexts.set(message.id, context);
>+    this._mmContexts.set(targetMM, message.id);
>+  },
So we call newContext and take its return value, yet newContext doesn't return anything.
The return value will be undefined and I don't understand how _watch can do anything useful.
So, please return context.



>diff --git a/toolkit/identity/FirefoxAccounts.jsm b/toolkit/identity/FirefoxAccounts.jsm
toolkit/ needs a review from toolkit peer

Comment 108

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Olli Pettay [:smaug] from comment #107)
> Comment on attachment 8363130 [details] [diff] [review]
> Part 1: DOM API

Thanks for your review, smaug

> >+  /*
> >+   * Create a new RPWatchContext, and update the context maps.
> >+   */
> >+  newContext: function(message, targetMM) {
> >+    let context = new RPWatchContext(message, targetMM);
> >+    this._serviceContexts.set(message.id, context);
> >+    this._mmContexts.set(targetMM, message.id);
> >+  },
> So we call newContext and take its return value, yet newContext doesn't
> return anything.
> The return value will be undefined and I don't understand how _watch can do
> anything useful.
> So, please return context.

Holy cow! How did that get dropped?  Fixed.
 
> >diff --git a/toolkit/identity/FirefoxAccounts.jsm b/toolkit/identity/FirefoxAccounts.jsm
> toolkit/ needs a review from toolkit peer

MattN has already given us his r+ on behalf of the toolkit peers.

Thanks again,
j

Comment 109

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I want to run one last round of tests with the gaia UI Tests app, but I can't get today's m-c and gaia master to play well together (even with my patch popped off my queue).  I want that to work before checking this in.

Comment 110

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

Small tweak.  Fixed broken syntax in conditional test

Comment 111

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Identity tests are currently broken on m-c.  See Bug 963674.

Comment 112

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Part 1: DOM API

rebased

Comment 113

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8366792 [details] [diff] [review]
Part 1: DOM API

https://hg.mozilla.org/integration/fx-team/rev/9ed86b468f73

Comment 114

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ed86b468f73

Comment 115

[Fernando Jiménez Moreno [:ferjm]]

\o/

Comment 116

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I just discovered that http://mxr.mozilla.org/mozilla-central/source/dom/identity/DOMIdentity.jsm#8 checks for a preference "identity.fxaccounts.enabled".  Note that on Fx Desktop, this pref is no longer checked, so FxAccounts is enabled everywhere except in this module.  Given this seems to be all about FxOS, I don't think this is a problem, but I thought it worth explicitly calling this out.

Comment 117

[Sam Penrose]

Thanks for the heads up, Mark. The pref should not be an issue for more than another week or two, but you may well have saved one of us a couple hours of debugging.