Closed Bug 1022064 Opened 10 years ago Closed 10 years ago

Create privileged API for launching a FxA Oauth flow

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.1

People

(Reporter: ckarlof, Assigned: vladikoff)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [fxa])

Attachments

(2 files, 39 obsolete files)

38.19 KB, patch
Details | Diff | Splinter Review
16.06 KB, patch
Details | Diff | Splinter Review
Basic idea would be to create a FxAccountsOAuth module with a single function:

launchWebFlow(options)

This would launch the FxA Oauth flow in a new tab with the specified options. It would add an HTTP observer that captures the redirect URL before it submits to the relying server, parses out the Oauth parameters, and returns them to the caller via promise resolution.

The first customer here is anticipated to be Loop.
Vlad, our intern is preparing a patch for this. Gavin, who would be most appropriate to give us some feedback on this?
Flags: needinfo?(gavin.sharp)
Tim can probably help out with a first feedback pass.
Flags: needinfo?(gavin.sharp)
Attached patch 0001-Adds-FxA-OAuth.patch (obsolete) — Splinter Review
Attachment #8437834 - Flags: feedback?
Attachment #8437856 - Flags: feedback?
Required Loop Server endpoints are in this loop-server PR: https://github.com/mozilla-services/loop-server/pull/98/files
Comment on attachment 8437856 [details] [diff] [review]
0002-Add-FxA-OAuth-to-the-Loop-panel.patch

This is a proof of concept patch that makes the use of patch 0001-Adds-FxA-OAuth.patch
Attachment #8437834 - Flags: feedback? → feedback?(ckarlof)
Bug 996494 might also be relevant.
Comment on attachment 8437834 [details] [diff] [review]
0001-Adds-FxA-OAuth.patch

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

A drive-by.  Note this needs tests.

::: services/fxaccounts/FxAccountsOAuth.jsm
@@ +17,5 @@
> +  launchWebFlow: function (options) {
> +    // TODO: validation
> +    // TODO: config.oauth_uri could be the production oauth/v1 server from the prefs. identity.fxaccounts.oauth.uri
> +    // TODO: action optional : signup or signin?
> +    let deferred = Promise.defer();

I don't think you need this manual deferred here - it looks like:

  return OAuth.launchOathFlow().then( () => { ... return {code: code, state: state}; } );

would do what you want, and would avoid the need for the rejection handler and the explicit deferred.

@@ +27,5 @@
> +      "&action=" + config.action;
> +
> +    OAuth.launchOAuthFlow(fxaUrl, config.redirect_uri)
> +      .then(
> +        function (capturedRedirectUrl) {

We tend to use fat-arrow functions here, and my personal style is to indent like:

  OAuth.launchOAuthFlow(fxaUrl, config.redirect_uri).then(
    capturedResult => {
      ...
    }
  );

(but AFAIK there is no agreement on indentation, so take that for what it cost you ;)

::: services/fxaccounts/OAuth.jsm
@@ +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/. */
> +
> +this.EXPORTED_SYMBOLS = ["OAuth"];

I'm not sure OAuth makes sense as the module name and exported symbol name (although I'm struggling to think of a better one)

@@ +24,5 @@
> +   *        Returns a promise that resolves to the String of the captured OAuth redirect url,
> +   *        or is rejected with an error.
> +   */
> +  launchOAuthFlow: function (oauthUrl, redirectUrl) {
> +    this.deferred = Promise.defer();

I'd think you'd want to reject an existing deferred before creating a new one?

@@ +25,5 @@
> +   *        or is rejected with an error.
> +   */
> +  launchOAuthFlow: function (oauthUrl, redirectUrl) {
> +    this.deferred = Promise.defer();
> +    this.observerService = Cc["@mozilla.org/observer-service;1"]

These should probably use Services.obs and Services.wm - which are short-and-simple enough that you probably don't need to bother storing them in |this| - just use them directly.

@@ +52,5 @@
> +  _createNewFlow: function (url) {
> +    // open a new tab with the OAuth url
> +    this.tab = this.browser.addTab(url);
> +    this.browser.selectedTab = this.tab;
> +    this.tab.launchedOAuthFlow = true;

duplicate line

@@ +58,5 @@
> +
> +    this.observerService.addObserver(this, "http-on-modify-request", false);
> +    this.observerService.addObserver(this, "browser-lastwindow-close-requested", false);
> +    this.tabHandler = this._tabClose.bind(this);
> +    this.container.addEventListener("TabClose", this.tabHandler, true);

it looks like we might also want some way to detect when the tab is navigated to some other URL - eg, if the user types something into the address bar, or selects a bookmark etc.

Also, note that the user could drag/drop a tab to a different window.  That *might* cause a TabClose() event on the container even though the tab remains open in a different container - but even if it doesn't, I doubt it would fire when the tab is actually closed, as the dragged tab is no longer attached to the same container.

@@ +73,5 @@
> +    if (topic === "http-on-modify-request") {
> +      var httpChannel = subject.QueryInterface(Ci.nsIHttpChannel);
> +      var currentUrl = httpChannel.URI.spec;
> +
> +      if (currentUrl.indexOf(this.redirectUrl) === 0) { // TODO: filter by window for security

I'd have expected a simple equality check here - eg, if there was a chance that redirectUrl == "http://foo/bar" and currentUrl is "http://foo/bar/etc" or "http://foo/barrel", I don't think we'd want to match?

@@ +81,5 @@
> +        this.browser.removeTab(this.tab);
> +        // resolve the captured url
> +        this.deferred.resolve(currentUrl);
> +      }
> +    }

else should go on the same line as the trailing '}'

@@ +97,5 @@
> +   * @private
> +   */
> +  _tabClose: function (event) {
> +    if (this.tab === event.target) {
> +      this.observerService.removeObserver(this, "http-on-modify-request", false);

removeObserver only takes 2 params.

@@ +120,5 @@
> +   *
> +   * @private
> +   */
> +  _cleanUpStaleFlows: function () {
> +    var tabs = this.browser.tabContainer.childNodes;

this worries me a little - if this function ever found stale tabs, that would imply the observers etc have been alive ever since those stale tabs were initially opened.  So while this might be reasonable as a "just in case" measure, it should never find a stale tab in practice - so maybe a Cu.reportError() would be worthwhile if it did, as it implies we aren't cleaning up correctly.

Alternatively, if another tab really is still alive and attempting to perform oauth flows, I wonder if we shouldn't just reuse that tab?

Also note that there might be many windows open, and each call to launchOAuthFlow might end up using a different one (as getMostRecentWindow() might return a different window each time).  This function doesn't account for that.
Comment on attachment 8437856 [details] [diff] [review]
0002-Add-FxA-OAuth-to-the-Loop-panel.patch

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

another drive-by (and another note that tests will be necessary)

::: browser/components/loop/MozLoopAPI.jsm
@@ +173,5 @@
> +            // build an OAuth login request, capture the OAuth redirect
> +            let oauthOptions = {
> +              oauthParameters: oauthParameters,
> +              redirectUrl: captureRedirectUrl
> +            };

you should probably avoid nesting of these promises - so something like:

  return MozLoopService.getFxaOauthParameters().then(
    oauthParameters => {
       ...
       return {
         oauthParameters: oauthParameters,
	 redirectUrl: captureRedirectUrl.
       }
    }
  ).then(
    oauthOptions => {return FxAccountsOAuth.launchWebFlow(oauthOptions);}
  ).then(
    tokenData => {return MozLoopService.getFxaOauthToken(tokenData);}
  ).then(
    oauthTokenData => {
      ...; 
      return MozLoopService.getFxaOauthProfile(token, profileServerUrl);
    }
  ).then(
    profileData => {
      callback(null, profileData);
    }
  ).then(
    null,
    err => callback(err, null)
  );

::: browser/components/loop/MozLoopService.jsm
@@ +657,5 @@
> +    tokenRequest.open("GET", profileServerUrl + "/profile", true);
> +    tokenRequest.setRequestHeader("Authorization", "Bearer " + token);
> +    tokenRequest.onload = function() {
> +      try {
> +        deferred.resolve(tokenRequest.responseText);

I'd expect the JSON.parse to be done here - the name of the method implies it return an object rather than a string.

::: browser/components/loop/content/js/panel.js
@@ +119,5 @@
> +      this.notifier.clear();
> +      event.preventDefault();
> +      navigator.mozLoop.fxaSignIn(function (err, data) {
> +        console.log(err);
> +        var profileData = JSON.parse(data);

see other comment - I'd expect the parse to be done before it is passed here.
(In reply to Mark Hammond [:markh] from comment #9)
> you should probably avoid nesting of these promises - so something like:

Alternatively, consider using Task.jsm - this would then be written more like:
  try {
    let oauthParameters = yield MozLoopService.getFxaOauthParameters();
    let oauthOptions = yield FxAccountsOAuth.launchWebFlow(...));
    let tokenData = yield MozLoopService.getFxaOauthToken(...);
    ...
    let profileData = yield MozLoopService.getFxaOauthProfile(token, profileServerUrl)
    callback(null, profileData);
  } catch (ex) {
    callback(ex, null);
  }
Thanks for the feedback Mark.
Interesting note re: 

> I'd expect the JSON.parse to be done here - the name of the method implies
> it return an object rather than a string.
> 
> ::: browser/components/loop/content/js/panel.js
> @@ +119,5 @@
> > +      this.notifier.clear();
> > +      event.preventDefault();
> > +      navigator.mozLoop.fxaSignIn(function (err, data) {
> > +        console.log(err);
> > +        var profileData = JSON.parse(data);
> 
> see other comment - I'd expect the parse to be done before it is passed here.

For some strange reason if data returns an object, then I cannot access the properties, even though it logs them. Screenshot: http://v14d.com/i/539166ab0b4b5.jpg
(In reply to Vlad Filippov from comment #11)
> For some strange reason if data returns an object, then I cannot access the
> properties, even though it logs them. Screenshot:
> http://v14d.com/i/539166ab0b4b5.jpg

I've not tried this, but I *think* something like:

  callback(null, Cu.cloneInto(profileData, targetWindow));

would do the right thing (assuming profileData is the already parsed object).
Updated
Attachment #8437834 - Attachment is obsolete: true
Attachment #8437856 - Attachment is obsolete: true
Attachment #8437834 - Flags: feedback?(ckarlof)
Attachment #8437856 - Flags: feedback?
Attachment #8438138 - Flags: feedback?(mhammond)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
(In reply to Mark Hammond [:markh] from comment #8)
> Comment on attachment 8437834 [details] [diff] [review]
> 0001-Adds-FxA-OAuth.patch
> 
> Review of attachment 8437834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure OAuth makes sense as the module name and exported symbol name
> (although I'm struggling to think of a better one)

Chris suggested "Identity.jsm". We were planning to go with that. Thoughts?

> @@ +24,5 @@
> +   *        Returns a promise that resolves to the String of the captured OAuth redirect url,
> +   *        or is rejected with an error.
> +   */
> +  launchOAuthFlow: function (oauthUrl, redirectUrl) {
> +    this.deferred = Promise.defer();
>I'd think you'd want to reject an existing deferred before creating a new one?

It creates a new window while destroying old ones. See current behaviour: http://v14d.com/g/fxa-oauth-windows.gif 
So the existing deferred won't be global.

>it looks like we might also want some way to detect when the tab is navigated to some other URL - eg, if the user types 
>something into the address bar, or selects a bookmark etc.

Not sure what to do about that yet.

> Also, note that the user could drag/drop a tab to a different window.  That *might* cause a TabClose() event on the container 
> even though the tab remains open in a different container - but even if it doesn't, I doubt it would fire when the tab is 
> actually closed, as the dragged tab is no longer attached to the same container.

Yeah, the updated PR adds multi-window support, but does not catch the `drag/drop` case yet.

> this worries me a little - if this function ever found stale tabs,
> that would imply the observers etc have been alive ever since those
> stale tabs were initially opened.  So while this might be 
> reasonable as a "just in case" measure, it should never find a 
> stale tab in practice - so maybe a Cu.reportError() would be
> worthwhile if it did, as it implies we aren't cleaning up  
> correctly.
> Alternatively, if another tab really is still alive and attempting 
> to perform oauth flows, I wonder if we shouldn't just reuse that 
> tab?
> Also note that there might be many windows open, and each call to
> launchOAuthFlow might end up using a different one (as 
> getMostRecentWindow() might return a different window each time). 
> This function doesn't account for that.


Hmm currently the code forces to only have one `launchOAuthFlow` at a time (see earlier GIF link). It destroys the existing flow if a new one starts. 
I will talk to Chris to provide more detail on this concern.
Comment on attachment 8438138 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

I think this is improved - thanks - but see the comments below.

::: services/fxaccounts/OAuth.jsm
@@ +64,5 @@
> +
> +    this.observerService.addObserver(this, "http-on-modify-request", false);
> +    this.observerService.addObserver(this, "browser-lastwindow-close-requested", false);
> +    this.tabHandler = this._tabClose.bind(this);
> +    this.container.addEventListener("TabClose", this.tabHandler);

I'm still not too keen on the fact that these observers may live forever if the tab never gets a close event - eg, if the tab is reused.

I wonder is using an nsIWebProgressListener would be more robust - I'd expect that in that case, you could be sure to get a STATE_STOP notification which you could use to clean up.  That may let you avoid all observers and avoid _cleanupStaleFlows completely.

@@ +79,5 @@
> +    if (topic === "http-on-modify-request") {
> +      var httpChannel = subject.QueryInterface(Ci.nsIHttpChannel);
> +      var currentUrl = httpChannel.URI.spec;
> +
> +      if (this.redirectUrl === currentUrl.substring(0, this.redirectUrl.length)) { // TODO: filter by window for security

this still seems wrong - if this.redirectUrl is http://foo/bar and currentUrl is http://foo/bar/etc I'm not sure you want this to be true.
Attachment #8438138 - Flags: feedback?(mhammond) → feedback+
Thanks for your feedback and pointing out `nsIWebProgressListener`. I have started refactoring the OAuth tab capture using that.
Attachment #8438138 - Attachment is obsolete: true
Attachment #8438139 - Attachment is obsolete: true
Attachment #8438221 - Flags: feedback?(mhammond)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Comment on attachment 8438221 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

::: services/fxaccounts/FxAccountsOAuth.jsm
@@ +25,5 @@
> +      "&action=" + action;
> +
> +    return OAuth.launchOAuthFlow(fxaUrl, config.redirect_uri).then(
> +      capturedRedirectUrl => {
> +        let code = capturedRedirectUrl.match(/code=([^&]+)/)[1];

I just learnt about the hot, new URLSearchParams :)  So instead of the regex, something like:

let uri = Services.io.newURI(capturedRedirectUrl, null, null);
uri.QueryInterface(Ci.nsIURL);
let searchParams = new URLSearchParams(uri.query);
let code = searchParams.get("code");

https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams for more.

::: services/fxaccounts/OAuth.jsm
@@ +64,5 @@
> +   * @param aRequest    nsIRequest aRequest
> +   * @param aLocation   nsIURI aLocation
> +   */
> +  onLocationChange: function(aProgress, aRequest, aLocation) {
> +    var toCapture = this.redirectUrl.substring(0, this.redirectUrl.length);

use let

@@ +74,5 @@
> +      // resolve the captured url
> +      this.deferred.resolve(aLocation.spec);
> +      // TODO: Currently without a timeout this gets `this.mBrowser is undefined` on removeTab
> +      setTimeout(function() {
> +        this.browser.removeTab(this.tab);

I think you can get the tab's current browser via something like tab.ownerDocument.defaultView or something - I don't have time to try and find it now, but this would mean you can lose the .browser getter entirely, and avoid the risk of the tab's owner changing during the load.

But this is looking much nicer.
Attachment #8438221 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 8438221 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

::: services/fxaccounts/OAuth.jsm
@@ +76,5 @@
> +      // TODO: Currently without a timeout this gets `this.mBrowser is undefined` on removeTab
> +      setTimeout(function() {
> +        this.browser.removeTab(this.tab);
> +      }.bind(this), 500);
> +    }

A couple more thoughts:

* you also want to handle STATE_STOP incase an error means your URL never arrived.
* It might even be better to have the observer be a closure over onLocationChange - in which case you are down to a single function, so can avoid the OAuthFlow object entirely - at which point rolling it up into FxAccountsOAuth might even make sense.
Mark mentioned it would be a good idea to ask ttaubert for feedback on what we have so far. 

To add: close handlers (STATE_STOP + window close), tests, better comments, input validation.  

The UX is still the same http://v14d.com/g/fxa-oauth-11.gif except multiple tabs can be opened that will start a new OAuth flow with a new state parameter.
Attachment #8438982 - Flags: feedback?(ttaubert)
Attachment #8438221 - Attachment is obsolete: true
Attachment #8438222 - Attachment is obsolete: true
Attachment #8438982 - Attachment is obsolete: true
Attachment #8438982 - Flags: feedback?(ttaubert)
Attachment #8440233 - Flags: feedback?(mhammond)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Updated with a sign up flow: http://v14d.com/g/fxa-oauth-signup.gif
Comment on attachment 8440233 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

This is starting to look good.

::: services/fxaccounts/FxAccountsOAuth.jsm
@@ +20,5 @@
> +   */
> +  launchWebFlow: function (options) {
> +    try {
> +      let deferred = Promise.defer();
> +      this._validateOptions(options);

this._validateOptions result isn't used, so I don't think invalid options will actually cause an error here.

@@ +21,5 @@
> +  launchWebFlow: function (options) {
> +    try {
> +      let deferred = Promise.defer();
> +      this._validateOptions(options);
> +      this._cleanUpOldTabs();

I'm not sure we really need _cleanupOldTabs.  IIUC, the only real time this will happen is if the user drags the tab to a new window *during* the flow, which seems an edge-case.  It also doesn't work on browser restart.  less-is-more and all that :)

@@ +24,5 @@
> +      this._validateOptions(options);
> +      this._cleanUpOldTabs();
> +
> +      let config = options.oauthParameters;
> +      let server = config.oauth_uri || this._getOAuthServerPref();

_getOauthServerPref() can return null, but you don't check that.  Given we assume the pref must exist, I'd be inclined to just inline the getCharPref and not handle it not existing (ie, to remove _getOauthServerPref())

@@ +38,5 @@
> +        fxaUrl: fxaUrl,
> +        redirectUri: config.redirect_uri
> +      };
> +
> +      new OAuthFlow(oauthFlowOptions, deferred);

I think we might as well now have OAuthFlow return the deferred instead of passing it in - so it just reads like:

return new OAuthFlow(oauthFlowOptions).then(....);

@@ +131,5 @@
> + * @constructor
> + */
> +function OAuthFlow(options, deferred) {
> +  if (!options || !options.fxaUrl || !options.redirectUri || !options.signupUri) {
> +    return Promise.reject(new Error("The OAuth flow is missing parameters"));

I think you want to reject |deferred| here - although this would be correct if we move to having OAuthFlow returning the promise as I suggest above

@@ +162,5 @@
> +      aRequest.cancel(Cr.NS_BINDING_ABORTED);
> +      // resolve the captured url
> +      deferred.resolve(aLocation.spec);
> +      // set OAuth completed for the TabClose event
> +      tab._oauthCompleted = true;

instead of this flag, I'd say just remove the tab close listener - just name the listener as you add it (tab.addEventListener("TabClose", function onTabClose(event)...)

@@ +163,5 @@
> +      // resolve the captured url
> +      deferred.resolve(aLocation.spec);
> +      // set OAuth completed for the TabClose event
> +      tab._oauthCompleted = true;
> +      tab.linkedBrowser.contentWindow.close();

I think we should call removeTab instead of a close on the contentWindow

@@ +166,5 @@
> +      tab._oauthCompleted = true;
> +      tab.linkedBrowser.contentWindow.close();
> +    } else if (signupMatch.equalsExceptRef(captureSignup)) {
> +      // capture successful sign up flow and manually press the redirectTo button
> +      tab.linkedBrowser.contentWindow.addEventListener("FirefoxAccountsCommand", function(e) {

I don't quite understand this part of the patch, so glossing over it for now :)

@@ +179,5 @@
> +    // check if the close is the result of proper redirect match
> +    if (!tab._oauthCompleted) {
> +      // event.detail determines if the tab was closed by moving to a different window
> +      // event.detail is undefined if the tab was truly closed
> +      if (!event.detail) {

I'm not sure making this distinction is important
Attachment #8440233 - Flags: feedback?(mhammond) → feedback+
(In reply to Mark Hammond [:markh] from comment #25)
> > +      tab.linkedBrowser.contentWindow.addEventListener("FirefoxAccountsCommand", function(e) {

"linkedBrowser.contentWindow" raises some e10s alarm bells - this will probably need to be restructured to be e10s-compatible. Tim/Mark can both offer advice on that!
Attachment #8440233 - Attachment is obsolete: true
Attachment #8440234 - Attachment is obsolete: true
Attachment #8441155 - Flags: feedback?(mhammond)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Updated to address e10s issues with `contentWindow`. 
The current approach to a smooth sign up flow blocked by https://github.com/mozilla/fxa-content-server/issues/1251
Comment on attachment 8441157 [details] [diff] [review]
0002-Add-FxA-OAuth-to-Loop.patch

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

I see no real red flags here...

::: browser/components/loop/MozLoopAPI.jsm
@@ +154,5 @@
> +    },
> +    /**
> +     * Use the Firefox Accounts OAuth flow to get profile data
> +     *
> +     * @callback

this confused me a little until I realized the @param notes were for the callback itself - might be worth clarifying that.

@@ +180,5 @@
> +            });
> +            let oauthTokenData = yield MozLoopService.getFxaOauthToken(tokenData, oauthResponse.session);
> +            let token = oauthTokenData.access_token;
> +            let profileData = yield MozLoopService.getFxaOauthProfile(token, profileServerUrl);
> +            var browser = Services.wm.getMostRecentWindow("navigator:browser");

I think we are going to want to use targetWindow to find the correct top-level window here.  See getChromeWindow in Chat.jsm for the excruciating dance necessary to do that.

@@ +185,5 @@
> +            browser.LoopUI.openCallPanel({target: browser.document.getElementById("loop-call-button")});
> +
> +            callback(null, Cu.cloneInto(profileData, targetWindow));
> +          } catch (ex) {
> +            callback(ex, null);

I wonder if a Cu.reportError might also make sense here.  I'm also not sure that an exception making the "success" callback should trigger a callback with the error.

::: browser/components/loop/MozLoopService.jsm
@@ +546,5 @@
> +   * @param token
> +   *
> +   * @return {Object}
> +   */
> +  getFxaOauthProfile: function(token, profileServerUrl) {

is it possible for the content to fetch this itself?  ie, if we can give the token and session info back to the content and it perform the xhr it would be better.
Adds a basic test for starters. Mark, any suggestions for testing all this appreciated!
Attachment #8441155 - Attachment is obsolete: true
Attachment #8441157 - Attachment is obsolete: true
Attachment #8441155 - Flags: feedback?(mhammond)
Attachment #8441190 - Flags: feedback?(mhammond)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
(In reply to Vlad Filippov from comment #31)
> Created attachment 8441190 [details] [diff] [review]
> 0001-bug-1022064-Adds-FxA-OAuth.patch
> 
> Adds a basic test for starters. Mark, any suggestions for testing all this
> appreciated!

I think you are going to need full end-to-end tests for the browser parts - ie, have your tests call the exposed functions against a test XHR server and test various response combinations (and even to a "bad" XHR url).  Look for .sjs files in the tree that might be somewhat close (and https://developer.mozilla.org/en-US/docs/Mochitest has some info about sjs)
Comment on attachment 8441190 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

::: browser/base/content/test/general/browser_fxa_oauth.js
@@ +10,5 @@
> +  "resource://gre/modules/FxAccountsOAuth.jsm");
> +
> +let gTests = [
> +  {
> +    desc: "Test the sign in flow",

TBH I find some of these "mini-test-frameworks" distracting and generally offering little value at the cost of additional complexity.  I understand lots of tests do this, and YMMV, so I don't object, but rather am pointing out I also wouldn't object to not having this kind of framework if you don't feel it adds value either.  That might become clearer once you have more actual tests.

::: services/fxaccounts/FxAccountsOAuth.jsm
@@ +19,5 @@
> +   * @returns {*}
> +   */
> +  launchWebFlow: function (options) {
> +    try {
> +      let deferred = Promise.defer();

can't we just create this deferred in OAuthFlow?

@@ +20,5 @@
> +   */
> +  launchWebFlow: function (options) {
> +    try {
> +      let deferred = Promise.defer();
> +      this._validateOptions(options);

this still doesn't have validateOptions cause failure.  I think _validateOptions can just throw (but then you might want to move it out of the try/catch block so the caller sees an exception rather than getting a rejected promise, but I guess this could be argued either way.)

@@ +37,5 @@
> +        fxaUrl: fxaUrl,
> +        redirectUri: config.redirect_uri
> +      };
> +
> +      log.debug('The OAuth flow has started with', oauthFlowOptions);

This will not log the param - you need to include ${} in the string to log the param in its entirety.

Also, if oauthFlowOption contains personally identifiable info, you would check for logPII (exported from FxAccountsCommon) (I don't think it does in this case though)

@@ +104,5 @@
> + * @param deferred Promise differed
> + * @constructor
> + */
> +function OAuthFlow(options, deferred) {
> +  if (!options || !options.fxaUrl || !options.redirectUri || !options.signupUri) {

this seems redundant with _validateOptions?  (I understand different things are being validated, but it seems strange to have 2 such places where different validations are done so close to each other)

@@ +108,5 @@
> +  if (!options || !options.fxaUrl || !options.redirectUri || !options.signupUri) {
> +    return Promise.reject(new Error("The OAuth flow is missing parameters"));
> +  }
> +
> +  let io = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);

Use Services.io instead of manually grabbing the service.

@@ +109,5 @@
> +    return Promise.reject(new Error("The OAuth flow is missing parameters"));
> +  }
> +
> +  let io = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
> +  let browser = Services.wm.getMostRecentWindow("navigator:browser").gBrowser;

similar to a comment I made before, I wonder if this should actually be passed in - IIUC, the calls to these functions are coming from loop which will have a content window available, so we can determine what top-level window is associated with that.  (I think in this case it is very unlikely that the most recent browser window would be different, but we might as well be sure)

@@ +136,5 @@
> +      browser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);
> +      // resolve the captured url
> +      deferred.resolve(aLocation.spec);
> +      browser.removeTab(tab);
> +      // set OAuth completed for the TabClose event

as mentioned in my previous comment, I think you should just remove the TabClose handler.

@@ +140,5 @@
> +      // set OAuth completed for the TabClose event
> +      tab._oauthCompleted = true;
> +    } else if (signupMatch.equalsExceptRef(captureSignup)) {
> +      if (aLocation.spec.indexOf('auto_continue=true') === -1) {
> +        browser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);

you should add a comment here explaining what is going on and how the promise is going to end up resolved.

@@ +141,5 @@
> +      tab._oauthCompleted = true;
> +    } else if (signupMatch.equalsExceptRef(captureSignup)) {
> +      if (aLocation.spec.indexOf('auto_continue=true') === -1) {
> +        browser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);
> +        browser.webNavigation.loadURI(aLocation.spec + '&auto_continue=true',

Are we absolutely sure that aLocation.spec will already include the '?' (ie, that it will already have at least 1 param so that appending '&' is safe)?

@@ +143,5 @@
> +      if (aLocation.spec.indexOf('auto_continue=true') === -1) {
> +        browser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);
> +        browser.webNavigation.loadURI(aLocation.spec + '&auto_continue=true',
> +          browser.webNavigation.LOAD_FLAGS_IS_LINK, null);
> +      }

I know I pointed you at LOAD_FLAGS_IS_LINK, but I think it is worth making sure you do actually need it - if not, remove it, if so, add a comment saying why.
Attachment #8441190 - Flags: feedback?(mhammond) → feedback+
Attachment #8441190 - Attachment is obsolete: true
Attachment #8441192 - Attachment is obsolete: true
Attachment #8441896 - Flags: feedback?(mhammond)
Attachment #8441896 - Flags: feedback?(ckarlof)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Vlad, I have had meeting insanity today, but I can hopefully look at this tomorrow.
Comment on attachment 8441896 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

::: browser/base/content/test/general/browser_fxa_oauth.js
@@ +66,5 @@
> +      FxAccountsOAuth.launchWebFlow({ oauthParameters: {
> +        state: "cc11e488aff7bb0c341ce64ea47b97ea3bdb79f6999a3a9b4637cdfa3383e9e7",
> +        client_id: "dcdb5ae7add825d3",
> +        oauth_uri: SERVER + "#",
> +        content_uri: "https://vlad.dev.lcip.org",

we shouldn't be touching external network resources (if that is actually what this will do!)

@@ +68,5 @@
> +        client_id: "dcdb5ae7add825d3",
> +        oauth_uri: SERVER + "#",
> +        content_uri: "https://vlad.dev.lcip.org",
> +        redirect_uri: "http://localhost:5000/fxa-oauth/redirect"
> +      }, window: gBrowser });

As discussed in IRC, I see there are many more tests being added, which is good and necessary.  For this test in particular, we will also want to check the promise returned by the flow is rejected which what you expect.

I also think we need to ensure there are some "full stack" tests that don't require much test infrastructure - eg, the .sjs that causes a redirect - ie, not having the test use load etc events to *simulate* what a redirect *should* look like, but a real, vanilla http redirect that drives the entire flow to completion.

But all in all this is looking good.

::: services/fxaccounts/FxAccountsOAuth.jsm
@@ +152,5 @@
> +  this.QueryInterface = XPCOMUtils.generateQI(["nsIWebProgressListener", "nsISupportsWeakReference"]);
> +
> +  this.onLocationChange = function (aProgress, aRequest, aLocation) {
> +    try {
> +      let captureRedirect = Services.io.newURI(options.redirectUri, null, null);

I meant to mention this before, but you might as well move the first 2 of these out into the parent scope to save re-doing that work each location change.
Attachment #8441896 - Flags: feedback?(mhammond) → feedback+
Attachment #8441896 - Attachment is obsolete: true
Attachment #8441897 - Attachment is obsolete: true
Attachment #8441896 - Flags: feedback?(ckarlof)
Attachment #8442639 - Flags: feedback?(ttaubert)
Attachment #8442639 - Flags: feedback?(ckarlof)
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Attached patch 0002-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
updated back to "http-on-modify-request" because "onLocationChange" cannot stop the request in time.
Attachment #8442639 - Attachment is obsolete: true
Attachment #8442640 - Attachment is obsolete: true
Attachment #8442639 - Flags: feedback?(ttaubert)
Attachment #8442639 - Flags: feedback?(ckarlof)
Attachment #8443266 - Flags: feedback?(gavin.sharp)
Attachment #8443266 - Flags: feedback?(ckarlof)
Attachment #8443266 - Flags: feedback?(gavin.sharp) → feedback?(ttaubert)
Comment on attachment 8443266 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

This seems a little fragile currently. What if the user leaves the fxa flow and goes on reading news? Should we remove the listeners or just keep them in case they hit "back" and complete the flow? What if I'm restarting the browser in between due to an applied update and then try to continue with the flow? It would still sign me up but what's the desired behavior here?

::: browser/base/content/test/general/browser.ini
@@ +427,5 @@
>  skip-if = e10s # Bug 940206 - nsIWebContentHandlerRegistrar::registerProtocolHandler doesn't work in e10s
>  [browser_no_mcb_on_http_site.js]
>  skip-if = e10s # Bug 516755 - SessionStore disabled for e10s
>  [browser_bug1003461-switchtab-override.js]
> +[browser_fxa_oauth.js]

Please insert before [browser_gestureSupport.js].

::: services/fxaccounts/FxAccountsOAuth.jsm
@@ +33,5 @@
> +   *     @param {String} [options.oauthParameters.scope]
> +   *     Optional. A colon-separated list of scopes that the user has authorized
> +   *     @param {String} [options.oauthParameters.action]
> +   *     Optional. If provided, should be either signup or signin.
> +   *   @param {(browser|tabbrowser)} [options.window]

This is very confusing, it should probably be |options.tabbrowser| and always take a <tabbrowser>.

@@ +65,5 @@
> +      };
> +
> +      log.debug("The OAuth flow has started");
> +
> +      return new OAuthFlow(oauthFlowOptions).then(

So you're saying |new OAuthFlow| which implies that |OAuthFlow| implements the Promise prototype. It's a normal function that returns a Promise so how about renaming it to "createOAuthFlow" and call it like:

return createOAuthFlow(oauthFlowOptions).then( ...

Due to all the state that the OAuthFlow has to track and interfaces to implement I think it would be better to define a proper prototype for OAuthFlow and call it like:

let flow = new OAuthFlow(oauthFlowOptions);
return flow.execute().then( ...

@@ +72,5 @@
> +
> +          if (params.has("code") && params.has("state")) {
> +            return { code: params.get("code"), state: params.get("state"), action: result.action };
> +          } else {
> +            throw new Error("Missing code or state parameters in the redirect response");

Else after return is bad style, you can just write |throw Error()| because we wouldn't get there when returning above.

@@ +79,5 @@
> +      );
> +    } catch (e) {
> +      log.error(e);
> +      return Promise.reject(e);
> +    }

Wrapping everything in try/catch is a bad pattern, please just return the promise created by OAuthFlow. There is nothing that should throw exceptions.

@@ +91,5 @@
> +   */
> +  _validateOptions: function (options) {
> +    if (!options || !options.oauthParameters) {
> +      throw new Error("Missing Firefox Accounts OAuth parameters");
> +    } else {

Else after throw is equally bad style because we'd bail out if we throw anyway :)

@@ +112,5 @@
> +      }
> +
> +      if (!config.state) {
> +        throw new Error("Missing 'oauthParameters.state' parameter");
> +      }

How about:

for (let prop of ["oauth_uri", "client_id", ...]) {
  if (!(prop in config)) {
    throw new Error("Missing 'oauthParameters." + prop + "' parameter");
  }
}

@@ +142,5 @@
> + *   }
> + */
> +function OAuthFlow(options) {
> +  let deferred = Promise.defer();
> +  this.QueryInterface = XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference]);

Please define a proper prototype and attach the property there. And please don't make it a weak reference.

@@ +157,5 @@
> +  }.bind(this);
> +
> +  // select the tab
> +  opener.selectedTab = tab;
> +  tab.addEventListener("TabClose", tabClose, true);

I think you don't only want to listen for TabClose. In the situation that Mark described we're actually concerned about docShell swapping, so "SwapDocShells" would be the event to listen for. In case that happens we would need to attach to a different browser that is the new home of the docShell.

@@ +160,5 @@
> +  opener.selectedTab = tab;
> +  tab.addEventListener("TabClose", tabClose, true);
> +
> +  // only an "http-on-modify-request" can stop a request properly
> +  this.observe = function (aSubject, aTopic, aData) {

The prototype should also implement this observe function.

@@ +161,5 @@
> +  tab.addEventListener("TabClose", tabClose, true);
> +
> +  // only an "http-on-modify-request" can stop a request properly
> +  this.observe = function (aSubject, aTopic, aData) {
> +    try {

Please don't wrap everything in try/catch, only the fallible parts, like QueryInterface().

@@ +162,5 @@
> +
> +  // only an "http-on-modify-request" can stop a request properly
> +  this.observe = function (aSubject, aTopic, aData) {
> +    try {
> +      let oHttp = aSubject.QueryInterface(Ci.nsIHttpChannel);

Nit: oHttp is a weird variable name, maybe "httpChan"?

@@ +165,5 @@
> +    try {
> +      let oHttp = aSubject.QueryInterface(Ci.nsIHttpChannel);
> +      let url = oHttp.URI.spec;
> +
> +      if (typeof oHttp.notificationCallbacks !== "undefined") {

Nit: |if (oHttp.notificationCallbacks) {| should suffice.

@@ +198,5 @@
> +    }
> +  };
> +
> +  // only a onLocationChange can detect a pushState event
> +  this.onLocationChange = function (aProgress, aRequest, aLocation) {

Should also be implemented by the prototype.

@@ +205,5 @@
> +      if (signupMatch.equals(captureSignup)) {
> +        action = "signup";
> +        // if this captures an FxA sign up flow, then we tell the fxa-content-server
> +        // to automatically continue the flow after the email has been verified
> +        if (aLocation.spec.indexOf("auto_continue=true") === -1) {

if (!aLocation.spec.contains("auto_continue=true")) {

@@ +206,5 @@
> +        action = "signup";
> +        // if this captures an FxA sign up flow, then we tell the fxa-content-server
> +        // to automatically continue the flow after the email has been verified
> +        if (aLocation.spec.indexOf("auto_continue=true") === -1) {
> +          browser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);

browser.stop();

@@ +207,5 @@
> +        // if this captures an FxA sign up flow, then we tell the fxa-content-server
> +        // to automatically continue the flow after the email has been verified
> +        if (aLocation.spec.indexOf("auto_continue=true") === -1) {
> +          browser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);
> +          browser.webNavigation.loadURI(aLocation.spec + "&auto_continue=true", null, null, null, null);

browser.loadURI(...);

@@ +216,5 @@
> +      deferred.reject(e);
> +    }
> +  };
> +
> +  browser._oauthListener = this;

Please don't attach custom properties to xul browsers. You can use a WeakMap but I think you might not need this.

@@ +218,5 @@
> +  };
> +
> +  browser._oauthListener = this;
> +  browser.addProgressListener(this);
> +  Services.obs.addObserver(this, "http-on-modify-request", true);

Please don't make this a weak observer, a non-weak one is just fine.
Attachment #8443266 - Flags: feedback?(ttaubert)
(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #43)

Thanks for the response and feedback Tim.

> 
> This seems a little fragile currently. What if the user leaves the fxa flow
> and goes on reading news? Should we remove the listeners or just keep them
> in case they hit "back" and complete the flow?

I'm experimenting with a global message manager, i.e 'Cc["@mozilla.org/globalmessagemanager;1"]
  .getService(Ci.nsIMessageListenerManager);', right now, that lets us attach a content script listener
to any tab. I hope this way it will be less fragile and allow us to capture a login attempt from any tab / window.

> What if I'm restarting the
> browser in between due to an applied update and then try to continue with
> the flow? It would still sign me up but what's the desired behavior here?
> 

If you restart the browser and have a tab left then it seems like we cannot do anything about that.
Even if we manage to attach the behaviour to tabs, the applications that wanted to login probably won't be expecting a resolved promise. One idea was to somehow clean up the tabs opened with this flow and not save them in history or something (but we still want to record the history properly, so the user can go back on FxA pages). Open to ideas if you know a way to deal with that part.
Attached patch bug-1022064-Adds-FxA-OAuth.patch (obsolete) — Splinter Review
Attachment #8443264 - Attachment is obsolete: true
Attachment #8443266 - Attachment is obsolete: true
Attachment #8443266 - Flags: feedback?(ckarlof)
Attachment #8451419 - Flags: feedback?(ckarlof)
Attached patch Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Blocks: 979845
Flags: firefox-backlog+
Attachment #8451419 - Attachment is obsolete: true
Attachment #8451420 - Attachment is obsolete: true
Attachment #8451419 - Flags: feedback?(ckarlof)
Attachment #8452364 - Flags: feedback?(ckarlof)
Attached patch 0777-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Attachment #8452364 - Attachment is obsolete: true
Attachment #8452365 - Attachment is obsolete: true
Attachment #8452364 - Flags: feedback?(ckarlof)
Attachment #8452499 - Flags: feedback?(ckarlof)
Comment on attachment 8452364 [details] [diff] [review]
0776-bug-1022064-Adds-FxA-OAuth.patch

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

Overall, looks good.

::: browser/base/content/test/general/browser_fxa_channel.js
@@ +10,5 @@
> +  "resource://gre/modules/FxAccountsChannel.jsm");
> +
> +let gTests = [
> +  {
> +    desc: "FxA Channel - should register a channel",

References here to FxAccountsChannel should be changed to WebChannel?

::: services/common/WebChannel.jsm
@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +

General question: What do we consider a channel? Is it a (channelId, domain) pair? Or just a (channelId)? If we can define that for ourselves, it might help us organize the code better.

@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +this.WebChannel = Object.create({

After we answer the above question, we can figure out a better name for this service. Calling this object a WebChannel may not be specific enough. It's already suspicious this has a property called _channels. One could consider this object an example of the Broker pattern (http://en.wikipedia.org/wiki/Broker_Pattern). Maybe WebChannelBroker?

@@ +17,5 @@
> +   * @param channelId {String}
> +   *        Id of the channel
> +   * @param origin {String}
> +   *        Valid origin for message on this channelId
> +   * @param callback {Function}

What is the required structure of this callback (i.e., parameters and type)?

@@ +20,5 @@
> +   *        Valid origin for message on this channelId
> +   * @param callback {Function}
> +   *        Callback function once a valid message arrives
> +   */
> +  registerChannel: function (channelId, origin, callback) {

Do we need an unregisterChannel(channelId, origin, callback) method?

@@ +43,5 @@
> +   *        Message Manager event
> +   * @private
> +   */
> +  _listener: function (event) {
> +    let message = event.data;

Please add some documentation about the format of messages (i.e., the stuff in message = event.data).

@@ +52,5 @@
> +
> +      // if channel exists and origin matches, then callback
> +      if (this._channels[message.channelId]) {
> +        // get the current pairs of origins and callbacks, copy that array to avoid issues.
> +        let channel = this._channels[message.channelId].slice();

If we rename _channels => _channelMap, then we can more easily rename _channel => _channels.

@@ +54,5 @@
> +      if (this._channels[message.channelId]) {
> +        // get the current pairs of origins and callbacks, copy that array to avoid issues.
> +        let channel = this._channels[message.channelId].slice();
> +
> +        channel.forEach(function(channelClients) {

Do we need to be concerned about what happens if one of the callbacks below adds or deletes elements from the array? I think the semantics of forEach are reasonable ( https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach) as far as additions or deletions, but it could cause weird interactions during a delete.

Consider a degenerate case of a client registering two callbacks for the same client_id and domain, and in the processing of the first callback, it unregisters and deletes both. My understanding of the semantics of forEach is that the second callback would not be invoked. If wanted to ensure that all callbacks registered for the channel were invoked, then you could first filter the channelList for an channelId by domain into another array and then iterate on that.

@@ +57,5 @@
> +
> +        channel.forEach(function(channelClients) {
> +          if (channelClients.origin === messageOrigin && channelClients.callback) {
> +            try {
> +              channelClients.callback(message.data.type, data);

Not sure that the format of the stuff in message.data is the concern of this component. Why not callback(data)?

@@ +68,5 @@
> +
> +      }
> +    }
> +  },
> +  _manager: Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager),

Let's be more specific with this. _messageManager?

@@ +69,5 @@
> +      }
> +    }
> +  },
> +  _manager: Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager),
> +  _channels: {},

This is also too vague. _channelMap? We also add some documentation here about what the structure of this map is.

@@ +70,5 @@
> +    }
> +  },
> +  _manager: Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager),
> +  _channels: {},
> +  _messageListenerAttached: false

Relocate this after the _messageManager and document what it's used for.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +14,5 @@
> +
> +/**
> + * Create a new FxAccountsOAuthClient for browser some service.
> + *
> + * @param {Object} options Options

Lots of confusing between using snake and camel case here, but I see the tension from client_id being snake.

@@ +33,5 @@
> + *     @param {String} [options.oauthParameters.action]
> + *     Optional. If provided, should be either signup or signin.
> + * @constructor
> + */
> +this.FxAccountsOAuthClient = function(options) {

Hmm. What's the intended lifetime of this client? A single request? E.g., putting the state in the constructor params suggests it's only good for a single request.

@@ +41,5 @@
> +
> +  this.config = conf;
> +  this.channelId = "oauth_" + conf.client_id;
> +  this.channelOrigin = Services.io.newURI(conf.content_uri, null, null).prePath;
> +  this.fxaUrl = conf.oauth_uri + "/authorization?" +

What is an fxAUrl? This is actually the oauthUri, and what you currently have called oauth_uri is what I consider to be the oauth_endpoint.

@@ +54,5 @@
> +
> +  /**
> +   * Opens a tab at 'this.fxaUrl', registers a channel to listen for events.
> +   */
> +  launchWebFlow: function () {

If we intend the client to be used for more than one request, we may need to move some of the config values as params of this method (e.g., state).

@@ +72,5 @@
> +   * @param data {Object}
> +   *        Additional command data
> +   * @private
> +   */
> +  _channelCallback: function (type, data) {

As I mentioned in the other file, this should probably take one parameter. I can see we're heading towards a data.data.data.... problem here, but that's what you get from a network stack. It we were parsing the bytes you wouldn't notice, but it's pretty amusing with named properties.

Maybe call the parameter here channelMessage and it has two properties, channelMessage.type and channelMessage.data.

@@ +74,5 @@
> +   * @private
> +   */
> +  _channelCallback: function (type, data) {
> +    switch (type) {
> +      case "oauth_close":

Do you want to use snake case or camel case for the types?
Attachment #8452364 - Attachment is obsolete: false
Comment on attachment 8452499 [details] [diff] [review]
0776-bug-1022064-Adds-FxA-OAuth.patch

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

Commented on the obsolete version, but vlad assures me they are similar.
Attachment #8452499 - Flags: feedback?(ckarlof) → feedback+
Points: --- → 5
(In reply to Vlad Filippov from comment #49)
> Created attachment 8452499 [details] [diff] [review]
> 0776-bug-1022064-Adds-FxA-OAuth.patch
...
> Attachment #8452365 [details] [diff] - Attachment is obsolete: true

Did you mean to mark this patch as obsolete? It was the most recent version of Add-FxA-OAuth-to-Loop.patch
Flags: needinfo?(vlad)
(In reply to Adam Roach [:abr] from comment #52)
> Did you mean to mark this patch as obsolete? It was the most recent version
> of Add-FxA-OAuth-to-Loop.patch

Hey Adam, we are working on the new version, that also has a new 'Add-FxA-OAuth-to-Loop.patch' that addresses recent changes made to the Loop Panel itself.
Should be ready in the next few hours if all goes well.
Flags: needinfo?(vlad)
Attachment #8452364 - Attachment is obsolete: true
Attachment #8452499 - Attachment is obsolete: true
Attachment #8454079 - Flags: feedback?(ckarlof)
Attached patch 1179-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Attachment #8454079 - Attachment is obsolete: true
Attachment #8454079 - Flags: feedback?(ckarlof)
Attachment #8454085 - Flags: feedback?(ckarlof)
Comment on attachment 8454085 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

Vlad, here's the gist outlining the refactor we discussed using a WebChannel object and private WebChannelBroker implementation

https://gist.github.com/ckarlof/170a89ce92febd29a6cf

::: services/common/WebChannelBroker.jsm
@@ +60,5 @@
> +   * @param event {Event}
> +   *        Message Manager event
> +   * @private
> +   */
> +    _listener: function (event) {

Weird indentation here.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +84,5 @@
> +     * @param target {EventTarget}
> +     *        Channel message event target
> +     * @private
> +     */
> +    let listener = function (webChannelId, data, target) {

I thought we had switched data -> message here?

@@ +88,5 @@
> +    let listener = function (webChannelId, data, target) {
> +      switch (data.command) {
> +        case "oauth_complete":
> +          // validate the state parameter and call onComplete
> +          if (data && this.onComplete && this.config.state === data.state) {

Why are you guarding data here but not below?
Attachment #8454085 - Flags: feedback?(ckarlof) → feedback+
Attachment #8454085 - Flags: feedback+ → feedback?(MattN+bmo)
Comment on attachment 8454085 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

f- for __exposedProps__.

::: browser/base/content/content.js
@@ +200,5 @@
> +    content.dispatchEvent(new content.CustomEvent('WebChannelMessageToContent', {
> +      detail: {
> +        webChannelId: message.webChannelId,
> +        message: JSON.stringify(message.data),
> +        __exposedProps__ : { webChannelId: "r", message: "r" }

"Do not add new instances of __exposedProps__ to mozilla-central": https://groups.google.com/d/topic/mozilla.dev.platform/yNKS1Z6UYQo/discussion

::: browser/base/content/test/general/browser_fxa_channel.js
@@ +6,5 @@
> +  "resource://gre/modules/Promise.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +  "resource://gre/modules/Task.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsChannel",
> +  "resource://gre/modules/FxAccountsChannel.jsm");

It's interesting that you made these lazy in the test but not in FxAccountsOAuthClient.jsm. Nit: no need for these to be lazy in tests.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +Cu.import("resource://gre/modules/WebChannelBroker.jsm");

Can any of these be made lazy to avoid being the first import? If they are all guaranteed to be used after this module is imported, then it's fine as-is.

::: services/fxaccounts/moz.build
@@ +12,5 @@
>    'Credentials.jsm',
>    'FxAccounts.jsm',
>    'FxAccountsClient.jsm',
> +  'FxAccountsCommon.js',
> +  'FxAccountsOAuthClient.jsm'

Nit: I believe you can add this trailing comma so you can avoid changing blame the next time you add an entry.
Attachment #8454085 - Flags: feedback?(MattN+bmo) → feedback-
Tests incomplete
Attachment #8454080 - Attachment is obsolete: true
Attachment #8454085 - Attachment is obsolete: true
Attachment #8455827 - Flags: feedback?(ckarlof)
Attachment #8455827 - Flags: feedback?(MattN+bmo)
Attached patch 1179-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Comment on attachment 8455827 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

Getting close!

::: services/common/WebChannel.jsm
@@ +19,5 @@
> +  this.origin = origin;
> +  this._channelCallback = null;
> +};
> +
> +this.WebChannel.prototype = {

Each of these methods, including _deliver needs some documentation.

@@ +29,5 @@
> +    this._channelCallback = null;
> +    WebChannelBroker.unregisterChannel(this);
> +  },
> +  send: function (message, target) {
> +    if (target.messageManager) {

What if target is undefined or target.messageManager is undefined? If the latter case, we shouldn't silently fail.

@@ +37,5 @@
> +      });
> +    }
> +  },
> +  _deliver: function(data, sender) {
> +    if (this._channelCallback) {

if this._channelCallback is undefined, this should probably log a warning.

@@ +42,5 @@
> +      try {
> +        this._channelCallback(data.webChannelId, data.message, sender);
> +      } catch (ex) {
> +        this.send({
> +          errno: 999,

What is this magic number here? We should create a constant for that.

::: services/common/WebChannelBroker.jsm
@@ +16,5 @@
> +   * based on proper origin and channel name
> +   *
> +   * @param channel {WebChannel}
> +   */
> +  registerChannel: function (channel) {

Should we allow double registering of the same channel?

@@ +37,5 @@
> +   */
> +  unregisterChannel: function (channelToRemove) {
> +    let channels = this._channelMap.slice();
> +
> +    channels.forEach((channel, index) => {

If we don't find the channel in the list, this will silently fail.

@@ +101,5 @@
> +  _sendErrorEventToContent: function (sender, webChannelId, errorMsg) {
> +    if (sender.messageManager) {
> +      sender.messageManager.sendAsyncMessage("WebChannelMessageToContent", {
> +        webChannelId: webChannelId,
> +        message: {

This is a channel error, not an application error, I don't think this should be embedded in the message. It should probably present the error at the top level.

@@ +102,5 @@
> +    if (sender.messageManager) {
> +      sender.messageManager.sendAsyncMessage("WebChannelMessageToContent", {
> +        webChannelId: webChannelId,
> +        message: {
> +          errno: 999,

Same issue with magic numbers used here, and based on the logic of the above comment, this 999 is different from the 999 in WebChannel.jsm. (different layers) :)
Attachment #8455827 - Flags: feedback?(ckarlof) → feedback+
Comment on attachment 8455827 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

Getting close!

::: services/common/WebChannel.jsm
@@ +19,5 @@
> +  this.origin = origin;
> +  this._channelCallback = null;
> +};
> +
> +this.WebChannel.prototype = {

Each of these methods, including _deliver needs some documentation.

@@ +29,5 @@
> +    this._channelCallback = null;
> +    WebChannelBroker.unregisterChannel(this);
> +  },
> +  send: function (message, target) {
> +    if (target.messageManager) {

What if target is undefined or target.messageManager is undefined? If the latter case, we shouldn't silently fail.

@@ +37,5 @@
> +      });
> +    }
> +  },
> +  _deliver: function(data, sender) {
> +    if (this._channelCallback) {

if this._channelCallback is undefined, this should probably log a warning.

@@ +42,5 @@
> +      try {
> +        this._channelCallback(data.webChannelId, data.message, sender);
> +      } catch (ex) {
> +        this.send({
> +          errno: 999,

What is this magic number here? We should create a constant for that.

::: services/common/WebChannelBroker.jsm
@@ +16,5 @@
> +   * based on proper origin and channel name
> +   *
> +   * @param channel {WebChannel}
> +   */
> +  registerChannel: function (channel) {

Should we allow double registering of the same channel?

@@ +37,5 @@
> +   */
> +  unregisterChannel: function (channelToRemove) {
> +    let channels = this._channelMap.slice();
> +
> +    channels.forEach((channel, index) => {

If we don't find the channel in the list, this will silently fail.

@@ +98,5 @@
> +   *        Error message
> +   * @private
> +   */
> +  _sendErrorEventToContent: function (sender, webChannelId, errorMsg) {
> +    if (sender.messageManager) {

If we're missing the sender or messageManager, this will fail or fail to report the error silently.

@@ +101,5 @@
> +  _sendErrorEventToContent: function (sender, webChannelId, errorMsg) {
> +    if (sender.messageManager) {
> +      sender.messageManager.sendAsyncMessage("WebChannelMessageToContent", {
> +        webChannelId: webChannelId,
> +        message: {

This is a channel error, not an application error, I don't think this should be embedded in the message. It should probably present the error at the top level.

@@ +102,5 @@
> +    if (sender.messageManager) {
> +      sender.messageManager.sendAsyncMessage("WebChannelMessageToContent", {
> +        webChannelId: webChannelId,
> +        message: {
> +          errno: 999,

Same issue with magic numbers used here, and based on the logic of the above comment, this 999 is different from the 999 in WebChannel.jsm. (different layers) :)

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +52,5 @@
> +    "&action=" + (conf.action || "signin") +
> +    "&webChannelId=" + this.webChannelId;
> +};
> +
> +this.FxAccountsOAuthClient.prototype = {

Do you need a method to release resources here, e.g., cleanup() or teardown() or release() or destroy()? Otherwise, the WebChannelBroker will hold a ref to the created channel forever.
Attachment #8456418 - Flags: feedback?(ckarlof)
Attachment #8456418 - Flags: feedback?(MattN+bmo)
Attached patch 1179-Add-FxA-OAuth-to-Loop.patch (obsolete) — Splinter Review
Attachment #8455827 - Attachment is obsolete: true
Attachment #8455828 - Attachment is obsolete: true
Attachment #8455827 - Flags: feedback?(MattN+bmo)
Assignee: nobody → vlad
Status: NEW → ASSIGNED
Comment on attachment 8456418 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

Here are some of the comments that got lost earlier.

::: browser/base/content/content.js
@@ +196,5 @@
> +// Add message listener for "WebChannelMessageToContent" messages from chrome scripts
> +addMessageListener("WebChannelMessageToContent", function (e) {
> +  let data = e.data;
> +  if (data) {
> +    content.dispatchEvent(new content.CustomEvent('WebChannelMessageToContent', {

Nit: use double quotes.

@@ +199,5 @@
> +  if (data) {
> +    content.dispatchEvent(new content.CustomEvent('WebChannelMessageToContent', {
> +      detail: Cu.cloneInto({
> +        webChannelId: data.webChannelId,
> +        message: data.message

Nit: always include the comma after the last property on an object literal so blame won't change if you add a line after.

@@ +200,5 @@
> +    content.dispatchEvent(new content.CustomEvent('WebChannelMessageToContent', {
> +      detail: Cu.cloneInto({
> +        webChannelId: data.webChannelId,
> +        message: data.message
> +      }, content)

Ditto

::: browser/base/content/test/general/browser_fxa_oauth.js
@@ +7,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +  "resource://gre/modules/Task.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsOAuthClient",
> +  "resource://gre/modules/FxAccountsOAuthClient.jsm");
> +const ENDPOINT = "http://mochi.test:8888";

Nit: newline above to make it stand out more.

@@ +88,5 @@
> +}
> +
> +function loadURI(aUri, aCallback) {
> +  gBrowser.addEventListener("load", function() {
> +    gBrowser.removeEventListener("load", arguments.callee, true);

Nit: arguments.callee is deprecated[1] so assign a name to the function above and use it here.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope/arguments/callee

::: services/common/WebChannel.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please add a comment using /* */ syntax which gives an overview of WebChannels and their use.

@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "WebChannelBroker",
> +  "resource://gre/modules/WebChannelBroker.jsm");

Nit: at least in browser/ code this would normally be indented to align with the first argument.

::: services/common/WebChannelBroker.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please add a comment using /* */ syntax which gives an overview of WebChannels and their use.

@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm");

This doesn't need to be lazy since it's very commonly used.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +Cu.import("resource://gre/modules/WebChannel.jsm");

Load WebChannel.jsm lazily maybe?
Adding dev-doc-needed for WebChannel.jsm
Keywords: dev-doc-needed
Comment on attachment 8456418 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

::: browser/base/content/test/general/browser_fxa_channel.js
@@ +6,5 @@
> +  "resource://gre/modules/Promise.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +  "resource://gre/modules/Task.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsChannel",
> +  "resource://gre/modules/FxAccountsChannel.jsm");

I guess this test still needs to be updated. Remember to rename this test file too.

::: browser/base/content/test/general/browser_fxa_oauth.js
@@ +14,5 @@
> +  {
> +    desc: "FxA OAuth - parameter validation",
> +    run: function* () {
> +      yield validationHelper(null,
> +        "Error: Missing Firefox Accounts OAuth parameters", "Empty parameters throw an exception");

Empty parameters would use undefined instead of null

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +34,5 @@
> + *     Optional. If provided, should be either signup or signin.
> + * @constructor
> + */
> +this.FxAccountsOAuthClient = function(options) {
> +  options = options || {};

This pattern is normally used when an argument is optional but in this case it isn't which makes this a bit confusing. Could we move the first check in _validateConfig to this point instead?

@@ +44,5 @@
> +  this.complete = false;
> +  this.config = conf;
> +  this.webChannelId = "oauth_" + conf.client_id;
> +  this.channelOrigin = Services.io.newURI(conf.content_uri, null, null).prePath;
> +  this.fxaOAuthStartUrl = conf.oauth_uri + "/authorization?" +

All of the above properties should be defined at the top of the prototype object below and provide default values. The assignments based on info passed to the constructor can remain in the constructor body but they should also be in the prototype object. At least this is how we normally do this for Fx-team. I believe it's so you can look in one place to see all properties on an object. We avoid adding properties dynamically.

@@ +49,5 @@
> +    "client_id=" + conf.client_id +
> +    "&state=" + conf.state +
> +    "&scope=" + (conf.scope || "") +
> +    "&action=" + (conf.action || "signin") +
> +    "&webChannelId=" + this.webChannelId;

Are you sure none of these should be URL encoded? I would recommend using URLSearchParams[1] to build the query params (e.g. with .append) and it handles escaping for you.

[1] https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams

@@ +64,5 @@
> +      this._registerChannel();
> +    }
> +
> +    if (this.complete) {
> +      this._reportError(new Error("This client already completed the OAuth flow"));

Like we discussed yesterday in person, I think this is a case where we should throw.

@@ +78,5 @@
> +   * @private
> +   */
> +  _registerChannel: function() {
> +    /**
> +     * Processes messages that are called back from the FxAccountsChannel

Should this be WebChannel now?

@@ +94,5 @@
> +        switch (message.command) {
> +          case "oauth_complete":
> +            // validate the state parameter and call onComplete
> +            if (this.onComplete && this.config.state === message.state) {
> +              log.debug("OAuth flow completed.");

AFAICT: log is undefined. See https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Log.jsm

BTW. we wouldn't want this to log for regular users. Debug logging should probably be behind a pref or some other flag.

@@ +113,5 @@
> +
> +    this._channelCallback = listener.bind(this);
> +    this.channel = new WebChannel(this.webChannelId, this.channelOrigin);
> +    this.channel.listen(this._channelCallback);
> +    log.debug("Channel registered: " + this.webChannelId + " with origin " + this.channelOrigin);

ditto.

@@ +144,5 @@
> +    if (!config.redirect_uri) {
> +      throw new Error("Missing 'oauthParameters.redirect_uri' parameter");
> +    }
> +
> +    if (!config.state) {

You could do these bottom 5 checks in a for…of loop to make it more compact.
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #67)
> Comment on attachment 8456418 [details] [diff] [review]
> 1178-bug-1022064-Adds-FxA-OAuth.patch
> 
> AFAICT: log is undefined. See
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Log.
> jsm
> 
> BTW. we wouldn't want this to log for regular users. Debug logging should
> probably be behind a pref or some other flag.
> 
> @@ +113,5 @@
> > +
> > +    this._channelCallback = listener.bind(this);
> > +    this.channel = new WebChannel(this.webChannelId, this.channelOrigin);
> > +    this.channel.listen(this._channelCallback);
> > +    log.debug("Channel registered: " + this.webChannelId + " with origin " + this.channelOrigin);
> 
> ditto.

We are using "Cu.import("resource://gre/modules/FxAccountsCommon.js");", see:
http://lxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsCommon.js#25 

This is FxA Logging that is behind a pref. Let me know if there are issues with that. Thanks for adding more feedback!
OK, that wasn't obvious. Thanks for clarifying. It seems like it's fine.
Comment on attachment 8456418 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

Okay, I think this will be my last pass on this attachment. Overall, the code is really nice and I think the architecture works.

My main concerns for the WebChannel component is the chance for leaks and the origin checks using prePath strings instead of principals. I think the WebChannel modules should get a super-review once the review comments are addressed before we ship code using this a general purpose content <=> chrome event/message helper.

::: browser/base/content/content.js
@@ +189,5 @@
>  
>  
> +// An event listener for custom "WebChannelMessageToChrome" events on pages
> +addEventListener("WebChannelMessageToChrome", function (e) {
> +  sendAsyncMessage("WebChannelMessageToChrome", e.detail);

I think the principal (e.target.nodePrincipal) should be included with the message as the 4th argument so its clearer that the message is coming from the page since I think you should be using it instead of the URI for the origin check in the Broker since the currentURI doesn't necessarily match the origin of the DOM event's origin. This raises the question about how sub-frames should be handled.

@@ +196,5 @@
> +// Add message listener for "WebChannelMessageToContent" messages from chrome scripts
> +addMessageListener("WebChannelMessageToContent", function (e) {
> +  let data = e.data;
> +  if (data) {
> +    content.dispatchEvent(new content.CustomEvent('WebChannelMessageToContent', {

It wouldn't hurt to double-check that creating and dispatching the event like this is safe for untrusted content.

::: services/common/WebChannel.jsm
@@ +30,5 @@
> +    throw new Error("WebChannel id and origin are required.");
> +  }
> +
> +  this.webChannelId = webChannelId;
> +  this.origin = origin;

It could be useful to construct an nsIURI at this point and use its prePath property. The main benefit is that you will be able to check if the origin passed is a valid URI. Secondly, it would clean up the argument ensuring it doesn't include path parameters or other garbage. You could warn or throw if the passed origin differs from uri.spec.

@@ +50,5 @@
> +   *        The source of the message
> +   */
> +  listen: function (callback) {
> +    if (callback) {
> +      this._deliverCallback = callback;

Is it desired to silently overwrite this._deliverCallback if this gets called a second time?

@@ +53,5 @@
> +    if (callback) {
> +      this._deliverCallback = callback;
> +      WebChannelBroker.registerChannel(this);
> +    } else {
> +      Cu.reportError("Failed to listen. Callback missing.");

Throw an error here but I'll note that you can't create a one-way (send-only) WebChannel. It's possible that may be wanted in the future. If we want to support that then I don't think this should report an error.

::: services/common/WebChannelBroker.jsm
@@ +44,5 @@
> +    let channelIdx = this._findChannelIndex(channelToRemove, this._channelMap);
> +    if (channelIdx === -1) {
> +      Cu.reportError("Failed to unregister the channel. Channel not found.");
> +    } else {
> +      this._channelMap = this._channelMap.slice(channelIdx);

This seems wrong to me as I understand it to be replacing the array with a new array containing only the channel to remove. i.e. it unregisters everything but the one you want :P
I think you wanted splice (with a "p") and howMany = 1. Switching to a real Map will fix this too. I actually wonder if you would be better off with a WeakMap in this case to avoid leaks if a tab closes without proper cleanup. That may need more thinking to understand whether consumers would be keeping the channel alive so it doesn't get GCed prematurely. Please ensure there is a test that unregistering removes the channel from the map.

@@ +49,5 @@
> +    }
> +  },
> +
> +  /**
> +   * @param event {Event}

Nit: add a description talking about how this is the dispatcher for WebChannel messages.

@@ +58,5 @@
> +    let data = event.data;
> +    let sender = event.target;
> +
> +    if (data && data.webChannelId) {
> +      let messageOrigin = sender.currentURI.prePath;

It's generally not recommend to use URIs for origin/security checks in the product. You can use event.principal.origin instead.

@@ +67,5 @@
> +      // find valid callbacks for this event
> +      channels.forEach(c => {
> +        if (c.webChannelId === data.webChannelId && c.origin === messageOrigin) {
> +          validChannelFound = true;
> +          c._deliver(data, sender);

It feels weird to call into a private method on the channel object IMO

@@ +100,5 @@
> +   * @param sender {browser}
> +   *        <browser> with a "messageManager" that will send be used to send the mssage
> +   * @private
> +   */
> +  _sendErrorEventToContent: function (webChannelId, errorMsg, sender) {

Nit: it looks like errorMsg is optional so please make it the last argument, update the comment and assign a default value in the argument list to make it more clear that it's optional (without reading the doc comment. Please do this for all optional arguments in the patch.

@@ +107,5 @@
> +        webChannelId: webChannelId,
> +        error: errorMsg ? errorMsg : "Web Channel Broker error"
> +      }, sender);
> +    }
> +  },

Perhaps there should be an else block that still logs the message?

@@ +118,5 @@
> +   *        An array of WebChannel objects
> +   * @returns {number}
> +   * @private
> +   */
> +  _findChannelIndex: function(channelToFind, channelMap) {

Do you plan to use this on maps other than this._channelMap? If not, I wouldn't take it as an argument and instead refer to this._channelMap directly. I also think you could switch to a real Map object instead of an array and then you wouldn't need this function.
Attachment #8456418 - Flags: feedback?(MattN+bmo) → feedback+
Target Milestone: --- → mozilla34
Mark, it's my understanding that you'll be handling the Loop usage of this FxA API in Gecko, correct? Vlad has some POC patches to the Loop Service demonstrating the usage of this API attached to this bug. You can of course take them or leave them. :)
Flags: needinfo?(standard8)
MattN, FxA+Loop is targeting Fx 34. I'd prefer to land this soonish (next week or two) to give the Loop dev and QA team as much time as possible with it to easily hit that target. Any concerns about that?
Flags: needinfo?(MattN+bmo)
(In reply to Chris Karlof [:ckarlof] from comment #72)
> Any concerns about that?

No concern. My understanding, prior to discussing with Vlad earlier, was that it was supposed to land before the merge on Monday anyways.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8456418 [details] [diff] [review]
1178-bug-1022064-Adds-FxA-OAuth.patch

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

Looking good Vlad.
Attachment #8456418 - Flags: feedback?(ckarlof) → feedback+
> No concern. My understanding, prior to discussing with Vlad earlier, was that it was supposed to land before the merge on Monday anyways.

Sounds good to me!

We can continue to land additional tests after that, if necessary.

Thanks, MattN!
Iteration: --- → 34.1
QA Whiteboard: [qa?]
More tests incoming...
Attachment #8456418 - Attachment is obsolete: true
Attachment #8456420 - Attachment is obsolete: true
Attachment #8460052 - Flags: review?(ckarlof)
Attachment #8460052 - Flags: review?(MattN+bmo)
Attached patch 0002-Add-Loop-to-FxA-OAuth.patch (obsolete) — Splinter Review
More tests incoming...
Attachment #8460052 - Attachment is obsolete: true
Attachment #8460053 - Attachment is obsolete: true
Attachment #8460052 - Flags: review?(ckarlof)
Attachment #8460052 - Flags: review?(MattN+bmo)
Attachment #8460723 - Flags: review?(ckarlof)
Attachment #8460723 - Flags: review?(MattN+bmo)
Attached patch 0002-Add-Loop-to-FxA-OAuth.patch (obsolete) — Splinter Review
QA Contact: jsmith
Comment on attachment 8460723 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +155,5 @@
> +          case "oauth_complete":
> +            // validate the state parameter and call onComplete
> +            if (this.onComplete && this.config.state === message.state) {
> +              log.debug("OAuth flow completed.");
> +              this.onComplete(message);

For these Oauth messages, you have the control information (the command) flat with the associated data. I may be able to live with that, but you shouldn't leak the command up to the consumer. This would necessitate creating an new object that sends up only expected values. This is probably a good idea anyway because if the consumer deletes any properties on that object, you don't want that to visible to other recipients of the message.
Attachment #8460723 - Attachment is obsolete: true
Attachment #8460724 - Attachment is obsolete: true
Attachment #8460723 - Flags: review?(ckarlof)
Attachment #8460723 - Flags: review?(MattN+bmo)
Attachment #8461281 - Flags: review?(ckarlof)
Attachment #8461281 - Flags: review?(MattN+bmo)
Attached patch 0002-Add-Loop-to-FxA-OAuth.patch (obsolete) — Splinter Review
QA Whiteboard: [qa?] → [qa-]
Attachment #8461281 - Flags: review?(ckarlof)
(In reply to Chris Karlof [:ckarlof] from comment #71)
> Mark, it's my understanding that you'll be handling the Loop usage of this
> FxA API in Gecko, correct? Vlad has some POC patches to the Loop Service
> demonstrating the usage of this API attached to this bug. You can of course
> take them or leave them. :)

Great thanks, I've added a reference on bug 979845, so we'll pick them up there when we implement that bug.
Flags: needinfo?(standard8)
Comment on attachment 8461281 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

I should be able to get to finish this tomorrow.

Gavin, can you super-review (or delegate) for this new WebChannel API? Vlad or I can help explain it to make it easier. The high-level is that it's a wrapper around the relaying of DOM events <=> message managers with origin checking baked in. It also makes it easy for multiple interested parties listen to messages for the same origin at runtime without having to hardcode their own method call in an event listener.
Attachment #8461281 - Flags: superreview?(gavin.sharp)
Whiteboard: [fxa]
Comment on attachment 8461281 [details] [diff] [review]
0001-bug-1022064-Adds-FxA-OAuth.patch

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

Looking good.

I still need to review the browser-chrome and non-FxA unit tests.

::: browser/base/content/content.js
@@ +218,5 @@
>  
> +// An event listener for custom "WebChannelMessageToChrome" events on pages
> +addEventListener("WebChannelMessageToChrome", function (e) {
> +  // if target is window then we want the document principal, otherwise fallback to target itself.
> +  let principal = e.target.document ? e.target.document.nodePrincipal : e.target.nodePrincipal;

I think it's safer to reverse the check so we use e.target.nodePrincipal if it exists and then fallback to e.target.document.nodePrincipal. I'm worried about a page adding a custom "document" property on the target that references another element and therefore changes the nodePrincipal. I'm not sure if this can happen in practice but I'd rather err on the safe side.

Why get the principal if e.detail is falsy since we're not using it?

@@ +221,5 @@
> +  // if target is window then we want the document principal, otherwise fallback to target itself.
> +  let principal = e.target.document ? e.target.document.nodePrincipal : e.target.nodePrincipal;
> +
> +  if (e.detail) {
> +    sendAsyncMessage("WebChannelMessageToChrome", e.detail, null, principal);

Do we really want to silently ignore if e.detail is falsy?

@@ +227,5 @@
> +}, true, true);
> +
> +// Add message listener for "WebChannelMessageToContent" messages from chrome scripts
> +addMessageListener("WebChannelMessageToContent", function (e) {
> +  let data = e.data;

Nit: This temp variable doesn't really seem necessary (it only saves two characters) but adds two lines (declaration and newline).

@@ +230,5 @@
> +addMessageListener("WebChannelMessageToContent", function (e) {
> +  let data = e.data;
> +
> +  if (data) {
> +    content.dispatchEvent(new content.CustomEvent("WebChannelMessageToContent", {

Again, I'm not sure we should silently ignore when e.data is falsy

::: browser/base/content/test/general/browser_fxa_oauth.html
@@ +1,4 @@
> +<html>
> +<head>
> +  <meta content="text/html;charset=utf-8" http-equiv="Content-Type">
> +  <meta content="utf-8" http-equiv="encoding">

Missing doctype and <meta charset="utf-8"> is enough. Same for the other html test file.

@@ +14,5 @@
> +          command: "oauth_complete",
> +          data: {
> +            state: "state",
> +            code: "code1",
> +            closeWindow: "signin"

Nit: add trailing comma

::: browser/base/content/test/general/browser_web_channel.html
@@ +5,5 @@
> +  <title>web_channel_test</title>
> +</head>
> +<body>
> +<script>
> +  window.onload = function(){

Nit: space before "{"

@@ +7,5 @@
> +<body>
> +<script>
> +  window.onload = function(){
> +
> +    var testName = window.location.search.replace(/^\?/, "");

If we had bug 1037715 you could do window.location.searchParams.toString().

@@ +62,5 @@
> +    }, true);
> +
> +
> +
> +    window.dispatchEvent(firstMessage);

Nit: too much whitespace around this line

::: services/common/WebChannel.jsm
@@ +2,5 @@
> + * 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/. */
> +
> +/*
> +  WebChannel is an abstraction that uses the Message Manager and Custom Events

Use JS-doc formatting so this gets picked up on MXR:
/**
 *
 */

@@ +10,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["WebChannel"];
> +
> +this.ERRNO_UNKNOWN_ERROR              = 999;
> +this.ERROR_UNKNOWN                    = "UNKNOWN_ERROR";

FYI: You're not exporting these.

@@ +42,5 @@
> +    throw new Error("WebChannel id and origin are required.");
> +  }
> +
> +  this.webChannelId = webChannelId;
> +  this.webChannelOrigin = Services.io.newURI(webChannelOrigin.prePath, null, null);

It seems like we are getting a URI with a prePath and then getting the prePath just to turn it into a new URI?
Isn't |this.webChannelOrigin = webChannelOrigin;| sufficient?

@@ +51,5 @@
> +  /**
> +   * WebChannel id
> +   */
> +  webChannelId: null,
> +  /**

Nit: add a newline before JSDOC comments

::: services/common/WebChannelBroker.jsm
@@ +2,5 @@
> + * 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/. */
> +
> + /*
> + WebChannelBroker is a global object that helps manage WebChannel objects.

Use JS-doc formatting so this gets picked up on MXR:
/**
 *
 */

@@ +66,5 @@
> +        data.message = data.message || {};
> +
> +        for (var channel of this._channelMap.keys()) {
> +          if (channel.webChannelId === data.webChannelId &&
> +            channel.webChannelOrigin.prePath === Services.io.newURI(event.principal.origin, null, null).prePath) {

Why can't you compare prePath to event.principal.origin?

::: services/common/moz.build
@@ +18,5 @@
>  ]
> +
> +EXTRA_JS_MODULES += [
> +  'WebChannel.jsm',
> +  'WebChannelBroker.jsm',

I don't think there's anything Services-specific about these modules so can you put them in toolkit/modules/ instead? Otherwise we would have to check if services is built before we use these modules.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +2,5 @@
> + * 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/. */
> +
> +/*
> +  Firefox Accounts OAuth browser login helper

Use JS-doc formatting so this gets picked up on MXR.

::: services/fxaccounts/tests/xpcshell/test_oauth_client.js
@@ +41,5 @@
> +    new FxAccountsOAuthClient(params);
> +  } catch (e) {
> +    do_check_eq(e.toString(), expected);
> +  }
> +}

You should do a return after the do_check_eq and fail if we reach the bottom of the function (meaning no exception occurred). Right now I don't think this will fail if an exception doesn't occur.
(In reply to Matthew N. [:MattN] from comment #85)
> Comment on attachment 8461281 [details] [diff] [review]
> 0001-bug-1022064-Adds-FxA-OAuth.patch

Thanks for the review!
To satisfy the "dev-doc-needed" keyword I have created 2 articles for the JSM components in this PR:

* https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/WebChannel.jsm
* https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/FxAccountsOAuthClient.js
Attached patch bug-1022064-Adds-FxA-OAuth.patch (obsolete) — Splinter Review
Attachment #8461281 - Attachment is obsolete: true
Attachment #8461281 - Flags: superreview?(gavin.sharp)
Attachment #8461281 - Flags: review?(MattN+bmo)
Attachment #8464451 - Flags: review?(MattN+bmo)
Attachment #8464451 - Flags: superreview?(gavin.sharp)
Comment on attachment 8464451 [details] [diff] [review]
bug-1022064-Adds-FxA-OAuth.patch

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

r=me. Well done for your first mozilla-central patch and thanks again for your patience. I've pushed it to try server[1] for you to make sure the tests pass in automation. Also, please update the commit message to make it more descriptive. Include something about WebChannel in it and r=MattN for now.

Results will appear in a few hours here: https://tbpl.mozilla.org/?tree=Try&rev=9cdd3b4b9bc6

I can help you figure out the results tomorrow. Feel free to ping me as a reminder.

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer

::: browser/base/content/test/general/browser_web_channel.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "WebChannel",
> +  "resource://gre/modules/WebChannel.jsm");

Nit: this doesn't need to be lazy in a test

@@ +8,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "WebChannel",
> +  "resource://gre/modules/WebChannel.jsm");
> +
> +let HTTP_PATH = "http://example.com";
> +let HTTP_ENDPOINT = "/browser/browser/base/content/test/general/browser_web_channel.html";

The uppercase name is normally used with "const". You can switch these.
Attachment #8464451 - Flags: review?(MattN+bmo) → review+
Attachment #8464451 - Attachment is obsolete: true
Attachment #8464451 - Flags: superreview?(gavin.sharp)
Attachment #8464870 - Flags: superreview?(gavin.sharp)
Comment on attachment 8464870 [details] [diff] [review]
Bug-1022064-Adds-WebChannel-communication-API-and-Fx.patch

High level feedback from my discssion with Matt:
- it's odd that WebChannel objects have "webChannelID" and "webChannelOrigin" properties - the "webChannel" part of the name is redundant.
- proposal:
  - name the properties just "ID" and "origin"?

- making the interface for consumers "channel-centric" seems a bit unusual
- the ability to specify a custom broker isn't used, and I have a hard time seeing how it would be
- proposal:
  - remove the stopListening/listen methods from WebChannel objects
  - give the WebChannel constructor a "callback" parameter
  - remove the WebChannel constructor's broker parameter
  - make WebChannel users call registerChannel on the right broker themselves

To reduce the number of compartments used and make it easier on consumers that would now need to reference both WebChannelBroker and WebChannel, I suggest combining both objects into the same JSM.

Can you make sure to file a followup bug for using the WebChannel mechanism for about:accounts? We should follow up on that soon.

sr=me with those comments addressed.
Attachment #8464870 - Flags: superreview?(gavin.sharp) → superreview+
It would be good to extend the MDN documentation (which is awesome for a patch in progress, btw!) to include coverage for the WebChannelBroker interface definition, as well as the interface definition for the WebChannel callback (WebChannelCallback?).
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #91)
> Comment on attachment 8464870 [details] [diff] [review]
> Bug-1022064-Adds-WebChannel-communication-API-and-Fx.patch

Thanks for the feedback Gavin!

> High level feedback from my discssion with Matt:
> - it's odd that WebChannel objects have "webChannelID" and
> "webChannelOrigin" properties - the "webChannel" part of the name is
> redundant.
> - proposal:
>   - name the properties just "ID" and "origin"?

Sure.

> - making the interface for consumers "channel-centric" seems a bit unusual

I'm unclear what you mean by this.

> - the ability to specify a custom broker isn't used, and I have a hard time
> seeing how it would be

It's there so we can mock out the broker for testing the WebChannel in isolation. Do you have other ideas about this?

> - proposal:
>   - remove the stopListening/listen methods from WebChannel objects
>   - give the WebChannel constructor a "callback" parameter

Channels are intended to be two-way (send and listen) with listening as optional. Putting a callback in the constructor seems inappropriate to me, especially if it would be optional.

stopListening is intended to allow consumers to clean up themselves and release resources in the broker. Thoughts about this?

>   - remove the WebChannel constructor's broker parameter

Primarily there for testing, as mentioned above.

>   - make WebChannel users call registerChannel on the right broker themselves

Outside of a testing environment it should. We could remove the broker option from the constructor and just let a testing consumer override this._broker with a mocked broker.

> To reduce the number of compartments used and make it easier on consumers
> that would now need to reference both WebChannelBroker and WebChannel, I
> suggest combining both objects into the same JSM.

Sure.

> Can you make sure to file a followup bug for using the WebChannel mechanism
> for about:accounts? We should follow up on that soon.

Sure.

> sr=me with those comments addressed.
Created bug 1046535 to track changing the FxA/Fx Sync integration to use this mechanism.
(In reply to Chris Karlof [:ckarlof] from comment #93)
> stopListening is intended to allow consumers to clean up themselves and
> release resources in the broker. Thoughts about this?

unregisterChannel would exist on the broker and be passed a channel.
Currently, we have:
let wc = new WebChannel(id, origin);
wc.listen(callback);
.....
wc.send(msg, target);
.....
wc.stopListening();

Are you proposing:
let wc = new WebChannel(id, origin, callback);
WebChannelBroker.registerChannel(wc);
....
wc.send(msg, target);
....
WebChannelBroker.unregisterChannel(wc);

If so, I have some concerns this proposal.

1) I consider the WebChannelBroker abstraction as a private implementation detail of how WebChannels work. The broker is just an abstraction around a messageManager, but maybe we'd like to change how this works in the future. Changing it is going to be more challenging if existing consumers interface to two APIs (WebChannel and WebChannelBroker) rather than one. For example, maybe we could remove the need for a singleton broker entirely. Having a listen()/stopListening() API on the WebChannel will allow us to do this more easily because existing consumers never knew about the broker at all.

2) I prefer methods to have a single concern and to be clear what the consequences are from calling them. IMO, the proposed changes send mixed signals to the developer. In the proposed change, you pass the callback in the WebChannel constructor, but you're not done, because you still need to call registerChannel on the broker. One might consider moving registerChannel() inside the constructor, but I also prefer that constructors don't do anything too crazy --  just set the stage. Also, moving registerChannel() into the constructor makes it hard for us to mock out the broker during testing.

Are there significant downsides to the way the API is currently structured that the proposed changes are meant to address? If there are, I'm missing them.
(In reply to Chris Karlof [:ckarlof] from comment #93)
> > - proposal:
> >   - remove the stopListening/listen methods from WebChannel objects
> >   - give the WebChannel constructor a "callback" parameter

Just spoke to Chris about this, his arguments are convincing. Let's drop this part of the proposal.
Blocks: 1047130
Matt, I have updated the patch. could we run a Try Build on this one?
Attachment #8461282 - Attachment is obsolete: true
Attachment #8464870 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
(In reply to Vlad Filippov from comment #98)
> Created attachment 8466056 [details] [diff] [review]
> Bug-1022064-Adds-WebChannel-communication-API-and-Fx.patch
> 
> Matt, I have updated the patch. could we run a Try Build on this one?

Sure: https://tbpl.mozilla.org/?tree=Try&rev=2e2a05a02ba4

BTW. if you think you'll be doing more mozilla-central work, feel free to request Level 1 commit access for yourself and I will vouch. https://www.mozilla.org/hacking/commit-access-policy/
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #100)
> (In reply to Vlad Filippov from comment #98)
> > Created attachment 8466056 [details] [diff] [review]
> > Bug-1022064-Adds-WebChannel-communication-API-and-Fx.patch
> > 
> > Matt, I have updated the patch. could we run a Try Build on this one?
> 
> Sure: https://tbpl.mozilla.org/?tree=Try&rev=2e2a05a02ba4
> 

 Hm seems like one debug build got stuck. Does that happen often?
That's a known intermittent failure as there is a matching bug in the list when I click on it. You should be good for checkin.
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/bf5985c067af
Keywords: checkin-needed
Whiteboard: [fxa] → [fxa][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bf5985c067af
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fxa][fixed-in-fx-team] → [fxa]
You messed up the test annotations in browser/base/content/test/general/browser.ini.

https://hg.mozilla.org/integration/fx-team/rev/07bf0e09b1b0
Depends on: 1052247
Depends on: 1058070
Product: Core → Firefox
Target Milestone: mozilla34 → Firefox 34
You need to log in before you can comment on or make changes to this bug.