The AitC implementation needs several utility functions to interact with BrowserID to obtain assertions etc. This seems like a good fit for a module in services/common for now (which could be lifted out to an even more global place when features like "login to the browser" are implemented).

First take at a common BrowserID module, should address all comments from bug 744257!

An additional note: as mentioned in the comment for _createFrame, it would be ideal to try and reuse both the iframe and the sandbox; however doing so will require us to be much more careful. Sometimes the functions that use _getSandbox want the latest "state" of browserid.org (i.e. cookie jar) so it's safest to recreate the iframe. I suggest we leave this optimization to a future iteration.

I caught a few nits that have been fixed in v1.1 of the patch.

Comment on attachment 614955 [details] [diff] [review] Common BrowserID Module - v1.1 Review of attachment 614955 [details] [diff] [review]: ----------------------------------------------------------------- This code is much cleaner than before! We're almost there. I need to solicit feedback from someone who knows the DOM/content interactions. I'll track someone down... ::: services/common/Makefile.in @@ +19,5 @@ > rest.js \ > stringbundle.js \ > tokenserverclient.js \ > utils.js \ > + browserid.js \ The list is sorted. ::: services/common/browserid.js @@ +5,5 @@ > +"use strict"; > + > +const EXPORTED_SYMBOLS = ["BrowserID"]; > +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; > +Cu.import("resource://gre/modules/Services.jsm"); Nit: add empty line (2 total) between above for readability. @@ +10,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://services-common/log4moz.js"); > +Cu.import("resource://services-common/preferences.js"); > + > +// Globals. No need for comment, IMO. @@ +13,5 @@ > + > +// Globals. > +var ID_URI = Preferences.get( > + "services.common.browserid.url", "https://browserid.org" > +); I don't believe we need the default value since the pref is defined in services-common.js. Also, if the preference changes, it won't get picked up until the module is loaded again. Either inline this logic or turn it into a function. You also may want to create a reference to the pref branch so you don't have to type "services.common.browserid." everywhere. const Prefs = new Preferences("services.common.browserid."); ... Prefs.get("url"); Nit: let @@ +19,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "gIOSvc", > + "@mozilla.org/network/io-service;1", "nsIIOService"); > +XPCOMUtils.defineLazyServiceGetter(this, "gURILoader", > + "@mozilla.org/uriloader;1", "nsIURILoader"); > + Document the purpose of this type, please! @@ +29,5 @@ > + this._log.level = Log4Moz.Level[Preferences.get( > + "services.common.log.logger.browserid", "Debug" > + )]; > +} > +BrowserIDSvc.prototype = { Functions have docs now \o/ @@ +37,5 @@ > + * > + * @return frame > + * (iframe) An empty, hidden iframe > + */ > + _createFrame: function _createFrame() { nit: define public functions before internal ones. @@ +39,5 @@ > + * (iframe) An empty, hidden iframe > + */ > + _createFrame: function _createFrame() { > + if (this._frame) { > + // TODO: Figure out how we can reuse the same iframe FYI, we have a policy that all TODOs have an associated bug number. Consider filing that follow-up... @@ +53,5 @@ > + let doc = hiddenDOMWindow.document; > + > + The following way of obtaining a window is more fickle than the above > + but appShellService doesn't work sometimes. Investigate. > + */ Commented code blocks make Hulk angry. @@ +55,5 @@ > + The following way of obtaining a window is more fickle than the above > + but appShellService doesn't work sometimes. Investigate. > + */ > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + let doc = win.document; nit: "win" is not used. Consolidate above lines into 1. @@ +69,5 @@ > + this._container = doc.documentElement; > + > + // Stop about:blank from being loaded. > + let webNav = frame.docShell.QueryInterface(Ci.nsIWebNavigation); > + webNav.stop(Ci.nsIWebNavigation.STOP_NETWORK); I'm going to defer review of the semantics to someone who knows things better. Maybe I'll get lucky and philikon will do another drive-by... @@ +88,5 @@ > + let self = this; > + let frame = _createFrame(); > + > + let parseHandler = { > + handleEvent: function(event) { Name all functions. @@ +105,5 @@ > + > + // Load the iframe. > + self._log.info("Creating BrowserID sandbox"); > + this._frame.addEventListener("DOMContentLoaded", parseHandler, true); > + gURILoader.openURI(channel, true, this._frame.docShell); I'm not sure this is the optimal way to do this. MXR revealed only two places where we use nsIURILoader from JS. 1 is https://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#1083 and it's usage is much more complex than what we have here. I suspect that this implementation is either overly naive and missing important functionality or that there is a better way of doing it. I want additional feedback before I sign off on this. @@ +118,5 @@ > + * (string) Email for which the assertion is to be created. > + * If no email is provided, the first available email for the > + * currently logged in user will be used instead. > + * @param audience > + * (string) Domain for which the assertion is to be created. Open Issue: What exactly will the domain be for services/automated-consumers? Security Question: How will we restrict which callers can obtain assertions for which domains? Is this enforced elsewhere in the BrowserID stack? Do all chrome-privileged scripts have the same permissions? @@ +145,5 @@ > + if (!audience) { > + this._log.error("getAssertion called but no audience provided"); > + cb(new Error("Audience not provided"), null); > + return; > + } Nit: Move this block before cb validation so arguments are all validated first and in order. @@ +148,5 @@ > + return; > + } > + > + let self = this; > + this._getSandbox(function(sandbox) { Name all functions. @@ +194,5 @@ > + if (!cb) { > + throw new Error("getAssertionWithLogin called without a callback"); > + } > + if (!win) { > + cb(new Error("getAssertionWithLogin called without a window")); Throw instead. Validate arguments in order. @@ +206,5 @@ > + let self = this; > + function callback(val) { > + if (!val) { > + self._log.error("Could not obtain assertion in getAssertionWithLogin"); > + cb(new Error("Could not obtain assertion"), null); Reuse the same string? @@ +240,5 @@ > + try { > + list = JSON.parse(res); > + } catch (e) { > + self._log.error("Exception in JSON.parse for isLoggedIn: " + e); > + cb(new Error(e), null); return; I /think/ JSON.parse() will throw an Error, so you don't need to wrap. @@ +277,5 @@ > + > + if (!emails.length) { > + this._isLoggedIn(function(err, yes) { > + if (err) { > + cb(err, null); return; Add line break between statements. @@ +280,5 @@ > + if (err) { > + cb(err, null); return; > + } > + if (yes) { > + cb(null, emails); return; Add line break. @@ +286,5 @@ > + cb(new Error("Logged in but no emails found"), null); > + }); > + } else { > + cb(null, emails); > + } Use early return to reduce nesting. @@ +306,5 @@ > + this._getSandbox(function(sandbox) { > + function callback(email) { > + if (email) { > + cb(null, email); > + } else { Use early return. @@ +313,5 @@ > + } > + sandbox.importFunction(callback, "callback"); > + > + let scriptText = > + "callback(window.BrowserID.Storage.site.get(" + Where can I find the definition of this API so I can verify semantics? ::: services/common/services-common.js @@ +4,5 @@ > pref("services.common.log.logger.rest.request", "Debug"); > pref("services.common.log.logger.rest.response", "Debug"); > > pref("services.common.tokenserverclient.logger.level", "Info"); > +pref("services.common.browserid.url", "https://browserid.org"); Please preserve alphabetical ordering. I'm also tempted to ask you to throw a trailing "/" on there. Also, maybe adjust the name to better describe the type of URL. loginIframeURL?

Comment on attachment 614955 [details] [diff] [review] Common BrowserID Module - v1.1 Review of attachment 614955 [details] [diff] [review]: ----------------------------------------------------------------- Much like gps, I don't know enough about the JS sandboxing stuff to do a drive-by, let alone review. This needs a review from a platform peer. Please try to arrange this. ::: services/common/browserid.js @@ +16,5 @@ > + "services.common.browserid.url", "https://browserid.org" > +); > + > +XPCOMUtils.defineLazyServiceGetter(this, "gIOSvc", > + "@mozilla.org/network/io-service;1", "nsIIOService"); Services.io gps found all the other nits I had. @@ +39,5 @@ > + * (iframe) An empty, hidden iframe > + */ > + _createFrame: function _createFrame() { > + if (this._frame) { > + // TODO: Figure out how we can reuse the same iframe You might be able to reuse the iframe, but you need to be very careful about it to avoid leaks. You need to listen for the chrome window that you're attaching the iframe to to be destroyed, then clear your iframe reference, and also unregister all the event handlers you have attached to that iframe (it seems you're at least doing the latter). @@ +105,5 @@ > + > + // Load the iframe. > + self._log.info("Creating BrowserID sandbox"); > + this._frame.addEventListener("DOMContentLoaded", parseHandler, true); > + gURILoader.openURI(channel, true, this._frame.docShell); What's wrong with nsIWebNavigation::loadURI()?

(In reply to Gregory Szorc [:gps] from comment #4) > I need to solicit feedback from someone who knows the DOM/content > interactions. I'll track someone down... We've used Sandboxing extensively in many labs add-ons (and still do). I'll describe what we're trying to achieve with this part of the code, in case it helps whoever will end up reviewing it. We want execute some JavaScript as if we were content script in a window that has loaded BrowserID. The only pieces of code that does this sort of thing in mozilla-central are Scratchpad & Web Console. In both their cases, however, the content is already loaded and visible; whereas in our case we must construct a hidden iframe and load browserid.org in it before using the sandbox. (For reference, this is where Scratchpad executes some JS in a content window: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#338) The creation of the iframe itself is fairly straightforward, but a docshell for it won't be created until it is actually part of the XUL tree. We currently put it in the XUL tree for the most recent browser window, but this will fail if there was none. There is the potential for using a hiddenDOMWindow that is always guaranteed to be provided by nsIAppShellService. However, that simply didn't work, I created bug 745415 so we can figure out why later (the current code will work in most cases). We stop about:blank from loading (it is loaded by default as soon as a docshell for the iframe is created), no harm is done if we let it load, but it seems cleaner to stop it since we don't need about:blank (as an aside, not stopping this load caused some weird crashes in the past, like bug 344305). As for using a channel to load browserid.org into the iframe, I don't have a good answer, except that in a previous instance where I was doing something similar (but not quite the same), I had to use a channel: https://github.com/anantn/slurp/blob/master/lib/parse.js#L49. There is no reason to use one in this particular instance, however, and philikon's suggestion of using nsIWebNavigator::loadURI() works just as well. > > +var ID_URI = Preferences.get( > > + "services.common.browserid.url", "https://browserid.org" > > +); > Also, if the preference changes, it won't get picked up until the module is > loaded again. Either inline this logic or turn it into a function. Good catch, I made ID_URI a getter on BrowserIDSvc instead. > > + _createFrame: function _createFrame() { > > + if (this._frame) { > > + // TODO: Figure out how we can reuse the same iframe > > FYI, we have a policy that all TODOs have an associated bug number. Consider > filing that follow-up... Bug 745414. > @@ +118,5 @@ > > + * (string) Email for which the assertion is to be created. > > + * If no email is provided, the first available email for the > > + * currently logged in user will be used instead. > > + * @param audience > > + * (string) Domain for which the assertion is to be created. > > Open Issue: What exactly will the domain be for services/automated-consumers? It's hard to say for services in general, but for AITC in particular, the audience will the domain where the apps dashboard is hosted. Currently, that is https://myapps.mozillalabs.com, but that may change to https://apps.persona.org (it will certainly be one of those two). > Security Question: How will we restrict which callers can obtain assertions > for which domains? Is this enforced elsewhere in the BrowserID stack? Do all > chrome-privileged scripts have the same permissions? We don't need to restrict it, since only privileged code can load this module. If you have chrome privileges, you can do all sorts of evil stuff (including phishing the browserid.org site itself) - there is no point in trying to prevent callers from obtaining an assertion for an arbitrary audience. On the contrary, this feature will be very useful when we implement the "Log in to the Browser" feature later. The only security risk here is inadvertently giving content code access to this function. Now, that would be a disaster! That is precisely why we use sandboxes, their sole purpose is to be careful about the chrome<->content boundaries. > @@ +240,5 @@ > > + try { > > + list = JSON.parse(res); > > + } catch (e) { > > + self._log.error("Exception in JSON.parse for isLoggedIn: " + e); > > + cb(new Error(e), null); return; > > I /think/ JSON.parse() will throw an Error, so you don't need to wrap. JSON.parse throws an exception, right? It seems like we'd want to convert that exception into an Error for the callback to process instead of sending the exception upward to the caller. > @@ +313,5 @@ > > + } > > + sandbox.importFunction(callback, "callback"); > > + > > + let scriptText = > > + "callback(window.BrowserID.Storage.site.get(" + > > Where can I find the definition of this API so I can verify semantics? Ah, tricky question! This internal BrowserID API is a temporary contract between us and the identity team. It is documented mostly in the form of tests in the browserid repo (https://github.com/mozilla/browserid/blob/dev/resources/static/test/cases/shared/user.js tests the BrowserID.User.* functions, while https://github.com/mozilla/browserid/blob/dev/resources/static/test/cases/shared/storage.js covers the BrowserID.Storage.* functions), this mostly ensures that there won't an update to BrowserID that breaks this module. We are working on a better API that covers all our use cases, and they will be better documented (and tested). See https://id.etherpad.mozilla.org/35 for more details. When we get that new API, we'll switch over to it. > > pref("services.common.tokenserverclient.logger.level", "Info"); > > +pref("services.common.browserid.url", "https://browserid.org"); > > Please preserve alphabetical ordering. I'm also tempted to ask you to throw > a trailing "/" on there. It is common to omit the "/" when referring to either an "origin" or "audience" (in the apps world at-least). This is mostly because all URLs that refer to a particular origin start with "/", and we can easily concatenate the two to get a URL without "//" in it. Thanks for the very comprehensive review(s)! It's fun to go into the nitty-gritty of producing clean code once in a while (something we don't do often enough in labs) :-)

Updated patch that covers most concerns in comment #4.

(In reply to Gregory Szorc [:gps] from comment #4) > I need to solicit feedback from someone who knows the DOM/content > interactions. I'll track someone down... One important note: in the current code the iframe and sandbox are being created synchronously, which is a *HUGE* drawback, and has potential performance implications. AFAIK, we can't do this work in a ChromeWorker since workers don't have access to XPCOM; and DOM manipulations aren't thread safe in JS. It would be super helpful to get input from someone on the platform team on the "right" way to do this.

Autoland Patchset: Patches: 615010 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=a87bb6fc1399 Try run started, revision a87bb6fc1399. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=a87bb6fc1399

Try run for a87bb6fc1399 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=a87bb6fc1399 Results (out of 15 total builds): success: 15 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-a87bb6fc1399

Comment on attachment 615010 [details] [diff] [review] Common BrowserID Module - v2 Canceling review as this module is going to be refactored to expose a simpler API; and will be moved to /identity from /services/common.

Refactored BrowserID module (moved to top-level identity directory). Implements a new API with only one function: BrowserID.getAssertion(...) that provides all the features that were previously provided by five separate functions.

Now that you are creating a new module and are touching the build system, you'll need a reviewer to sign off on the build system changes. khuey?

I'm not sure I understand the motivation for putting this into its own top-level directory. A new top level directory for a single JS module seems like overkill. What was the issue with putting it in services/common? More fundamentally, I don't understand why we're doing a complicated "sandboxed request"/iframe dance to get what boils down to a JSON response from a remote server. Why can't you just use an XHR to fetch the JSON directly? If there's a design doc that makes this painfully obvious somewhere, by all means just point me to it :)

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14) > I'm not sure I understand the motivation for putting this into its own > top-level directory. A new top level directory for a single JS module seems > like overkill. What was the issue with putting it in services/common? > I think there is still some question about where to put this code. It seemed like conversations about where to land it tended to end with "not my module", perhapsfor good reason. As Identity is a new Mozilla module, we thought identity/ was a good spot for it. I originally had another patch placing a similar set of jsms and a component in services/identity. I don't think it matters where it lands. > More fundamentally, I don't understand why we're doing a complicated > "sandboxed request"/iframe dance to get what boils down to a JSON response > from a remote server. Why can't you just use an XHR to fetch the JSON > directly? If there's a design doc that makes this painfully obvious > somewhere, by all means just point me to it :) It is because this is not a JSON response, there needs to be a full fledged page that has all of the crypto primitives and navigator.id loaded so the keys can be generated and signatures created. This is really a stop-gap measure until we get the new platform crypto API landed - which is in review now - as well as other Firefox bits: JSON assertions and publickey code, etc. This parti

(In reply to David Dahl :ddahl from comment #15) > This parti ^^ please ignore

Fix some nits and add more documentation.

(In reply to David Dahl :ddahl from comment #15) > It is because this is not a JSON response, there needs to be a full fledged > page that has all of the crypto primitives and navigator.id loaded so the > keys can be generated and signatures created. So this is just a shortcut to avoid having to duplicate in-product the code that currently lives on https://browserid.org? And putting that code into the product is out of the question?

(In reply to David Dahl :ddahl from comment #15) > I don't think it matters where it lands. I mostly agree (at least in the near term). Given that, we should go with the easiest solution, which seems to be putting it in services/common next to the rest of the utility code it seems to depend on, as in the previous patches.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18) > So this is just a shortcut to avoid having to duplicate in-product the code > that currently lives on https://browserid.org? And putting that code into > the product is out of the question? It's very much on the roadmap. This is a temporary measure. The reason we can't just do it in platform right away is that the patch would then be gigantic (our JS library, minified and gzipped, is 100k). We need to have the functionality now and then progressively bake more of it into platform.

Comment on attachment 615954 [details] [diff] [review] Common BrowserID Module - v3.1 So some questions about the sandbox setup. 1) If two calls to getAssertion race such that the second _getSandbox call happens before the callback for the first one has been called, what will happen? Will the first callback ever happen? 2) Why do you need the parseHandler object? Just pass the function you want to be the listener to addEventListener. 3) This code is relying on the this.ID_URI never leading to a page with subframes. Is that a good assumption. or should the DOMContentLoaded callback check the event target? 4) The current setup leaves the frame around, using up memory, after you're done with it. Is there any way to mitigate that? I realize you're handing out a sandbox with that frame's window on the proto chain to some callback function that might keep using it for a while. 5) Speaking of which, say _createFrame gets called again after you handed out the sandbox; this will kill the window in this._frame. Is the assumption that no one will want to do anything async with the sandbox, modulo #1 above? 6) When you null out this._frame you should also null out this._container. 7) In fact, since you're sticking these things on a _service_, right now using browserid in a Firefox window and then closing that window leaks the window until the next time browserid is used in another window. That seems bad. 8) "+ // Sometimes the provided win hasn't fully loaded yet": why not? 9) I'm assuming that importFunction does things safely and all that... In general this looks ok; r- specifically because of the "keep objects alive too long" issue.

(In reply to Ben Adida [:benadida] from comment #20) > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #18) > > So this is just a shortcut to avoid having to duplicate in-product the code > > that currently lives on https://browserid.org? And putting that code into > > the product is out of the question? > > It's very much on the roadmap. This is a temporary measure. > > The reason we can't just do it in platform right away is that the patch > would then be gigantic (our JS library, minified and gzipped, is 100k). We > need to have the functionality now and then progressively bake more of it > into platform. Adding to what Ben said, porting the code living on browserid.org is likely to be a complex effort as that code was written from the perspective of a web service to be used from any browser. However, the apps roadmap requires AITC support in Firefox 15 (actually, we wanted to make it in for 14, but it's too late for that now, especially with the tree closure), and we'd like to avoid waiting for the port. This is the fastest way to get us what we want, and the APIs that the module exposes have been designed in such a manner that when we do begin the transition to a native navigator.id implementation, little or no changes will be needed to AITC code.

Who is working on the integration of the required code to make this hack unnecessary? What are the blocking bugs? I'm really not very happy with the idea of farming out Firefox application code to a web service because implementing things natively is "too hard", so I'd like to understand more about why you think it's "too hard".

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #23) > Who is working on the integration of the required code to make this hack > unnecessary? What are the blocking bugs? We will be implementing this, and by we, I'm including folks from the identity, platform, and firefox teams because a full navigator.id implementation requires help from all of them :) There are two relevant bugs that are currently open to start this work. Bug 665057 (Design and implement crypto needed by BrowserID), and Bug 664614 (Browser API for Identity Services). A bug for the implementation of the actual DOM API (navigator.id) has not been created yet, but it is the last step in this process, and depends on these two bugs. > I'm really not very happy with the idea of farming out Firefox application > code to a web service because implementing things natively is "too hard", so > I'd like to understand more about why you think it's "too hard". Ideally, we would love to have a native BrowserID implementation so we can avoid doing hacks like these. I wouldn't necessarily use the word "hard" when describing the native work, but it is pretty clear that it will take a non-trivial amount of time and effort. This is because the current efforts towards a native implementation are (for the most part) a new implementation of BrowserID based on the spec, as code reuse from browserid.org isn't feasible simply because of the nature of the two codebases. We can delve into more details if you like, but we're making a judgement call here in determining how long a native implementation of BrowserID will take to complete. This is an instance of Mozilla client code relying on Mozilla server code. Sure, it is more efficient to have everything on the client, but in the case that the most efficient way also takes the most time, I think is it acceptable to offload some of the work to servers (that we authored and are responsible for running!) to get things going. In my humble opinion, major features like these shouldn't be built in one fell swoop. We want something working quickly so we can meet the goals the Apps team has set for themselves, and then work iteratively to improve the implementation by moving things down to the client, piece by piece.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #23) > Who is working on the integration of the required code to make this hack > unnecessary? What are the blocking bugs? khuey is working on the DOM API component, and as Anant mentioned we are in the process of coordinating the resources to make this happen natively. But everyone's been incredibly busy, as you know. > I'm really not very happy with the idea of farming out Firefox application > code to a web service because implementing things natively is "too hard", so > I'd like to understand more about why you think it's "too hard". It's not too hard. It's just a solid chunk of work. It took us a long time to get the shim working, and a significant portion of that work will have to be replicated in FF native. So, as Anant said, this is an issue of time/prioritization.

My "too hard" meant to imply "within the desired time frame". :) I understand the desire to bypass the native integration work and get the feature sooner, but I'm not sure that this is an acceptable shortcut.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #26) > I understand the desire to bypass the native integration work and get the > feature sooner, but I'm not sure that this is an acceptable shortcut. I'm having trouble understanding the issue: we all agree that this module will need to be updated. What's the problem with a temporary solution that works right now? Is there a general design principle that this violates and that I need to better understand? If this general approach is unacceptable, even to introduce new features that are eventually refactored, then that will have a significant impact on a number of Innovation projects, and it would be useful to document it so we can then adjust the k9o plan, which will almost certainly be impacted.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #26) > I understand the desire to bypass the native integration work and get the > feature sooner, but I'm not sure that this is an acceptable shortcut. Even though I will be the first to admit that there is a better way to this (and we have committed to do it, just not in the desired timeframe), I don't think this patch ugly beyond repair. What in particular don't you like about it? What we're doing here is not very different than Scratchpad for that matter; the only major differences being we provide a script to execute and the iframe is hidden. (Okay, those are pretty big differences, but still...)!

I think my question (and plausibly Gavin's, too) is whether the version we all gleefully agree needs replacing is good enough to be Firefox in the meantime. There are plenty of pieces of Firefox code that have an impending refactor on the horizon, but we shouldn't use the promise of future refactoring to let us ship low quality software today. I don't mean that to be insulting - different development environments demand different things. It would be stupid and slow to develop research projects to established-product quality bars. But likewise we shouldn't land code in Firefox that doesn't meet that bar. So, if that's an accurate summary, can we get crystal clear on the areas of concern and what (if anything) we can do to mitigate the risks there, short of the eventual refactoring?

(In reply to Johnathan Nightingale [:johnath] from comment #29) > So, if that's an accurate summary, can we get crystal clear on the areas of > concern and what (if anything) we can do to mitigate the risks there, short > of the eventual refactoring? There are two risks associated with shipping this code: - It depends on specific behavior of browserid.org, and if that site makes an incompatible change, this feature will break. The mitigation we have in place is to never push a new version of browserid.org that will break the API that we've agreed to with the identity team, and said API is unit tested by them. - Sandboxing is tricky in general and has several security concerns, but is by no means new to Firefox. Scratchpad and Web Console both do it, and is a powerful tool when used safely. All of bz's concerns in comment #21 are valid, but none of them are intractable, and will indeed be addressed in the next iteration of the patch. These are the only two objective reasons I can think of that affect the quality of this patch. There are several subjective reasons of course, such as this being the ugliest possible way of achieving what we want, and that a native navigator.id implementation is imminent. It's always better to ship code in the client than relying on server behavior when possible, we acknowledge that, but we estimate it will take longer than the product deadline for Apps in the Cloud (FF15).

Significantly revamped version of the patch: - Moved to /services/identity (from top-level /identity) - Simplified API, the module now exports two methods: -- getAssertion(cb, options); (attempts to obtain an assertion silently) -- getAssertionWithLogin(cb, options, window); (attempts to make the user login) I have tests! See bug 749907. The module was also refactored to address bz's concerns. Answers to his questions follow (out of order for clarity's sake): (In reply to Boris Zbarsky (:bz) from comment #21) > So some questions about the sandbox setup. > > 1) If two calls to getAssertion race such that the second _getSandbox call > happens before the callback for the first one has been called, what will > happen? Will the first callback ever happen? > > 4) The current setup leaves the frame around, using up memory, after you're > done with it. Is there any way to mitigate that? I realize you're handing > out a sandbox with that frame's window on the proto chain to some callback > function that might keep using it for a while. > > 5) Speaking of which, say _createFrame gets called again after you handed > out the sandbox; this will kill the window in this._frame. Is the > assumption that no one will want to do anything async with the sandbox, > modulo #1 above? > > 6) When you null out this._frame you should also null out this._container. > > 7) In fact, since you're sticking these things on a _service_, right now > using browserid in a Firefox window and then closing that window leaks the > window until the next time browserid is used in another window. That seems > bad. I addressed all of these 4 by making a new object "BrowserIDSandbox". A new instance of this object is constructed every time a sandbox is needed (so if there are two successive getAssertion calls, two sandboxes will be made). Additionally, this object has a "free" method in it's prototype, which callers who need a sandbox are expected to call when they are done with it. In our case, _getAssertionWithEmail is the primary user of the sandbox, and it calls free() after it has finished attempting to get an assertion. > 2) Why do you need the parseHandler object? Just pass the function you > want to be the listener to addEventListener. Fixed! > 3) This code is relying on the this.ID_URI never leading to a page with > subframes. Is that a good assumption. or should the DOMContentLoaded > callback check the event target? Good point, there is no such guarantee, so it's better to check. I added a guard at the beginning of the handler. > 8) "+ // Sometimes the provided win hasn't fully loaded yet": why not? The window is provided by the caller (who we don't know), and sometimes immediately after a URI has been loaded in the window. For an assertion grab to be successful we have to wait for all scripts in the window to finish loading, it felt safer to do it this way. > 9) I'm assuming that importFunction does things safely and all that... I believe so, all the methods imported are local closures and have no way to reach the parent object.

Comment on attachment 619256 [details] [diff] [review] Common BrowserID Module - v4 Review of attachment 619256 [details] [diff] [review]: ----------------------------------------------------------------- This patch is getting much better with every version! My biggest complaint is lack of tests. ::: services/identity/browserid.js @@ +13,5 @@ > +Cu.import("resource://services-common/log4moz.js"); > +Cu.import("resource://services-common/preferences.js"); > + > +const PREFS = new Preferences("identity.browserid."); > +const ID_URI = PREFS.get("url"); What if the pref is updated during application execution? Perhaps make this a function which dynamically fetches the value? @@ +77,5 @@ > + ); > + } > + > + let self = this; > + new BrowserIDSandbox(function _gotSandbox(obj) { Please rename "obj" to "sandbox" or similar everywhere in this file. "obj" is too generic. @@ +78,5 @@ > + } > + > + let self = this; > + new BrowserIDSandbox(function _gotSandbox(obj) { > + self._getEmails(obj, cb, options); Shouldn't _getEmails() call obj.free()? Maybe put that in a try..finally here? @@ +79,5 @@ > + > + let self = this; > + new BrowserIDSandbox(function _gotSandbox(obj) { > + self._getEmails(obj, cb, options); > + }); You can bind the inner function: new BrowserIDSandbox(function _gotSandbox(obj) { this._getEmails(obj, cb, options); }.bind(this)); If _getEmails was implemented as _getEmails(cb, options, obj), you could write this as: new BrowserIDSandbox(this._getEmails.bind(this, cb, options)); @@ +113,5 @@ > + * Be aware! The provided contentWindow must also have loaded the > + * BrowserID include.js shim for this to work! This behavior is > + * temporary until we implement native support for navigator.id. > + */ > + getAssertionWithLogin: function getAssertionWithLogin(cb, options, win) { Since options is currently optional, I'd change to (cb, win, options). It doesn't feel right to explicitly pass null as arguments. I still feel like getAssertion(cb, options) with "win" as an option is a better API. I know others disagree with me. Maybe someday we'll have named arguments in JS... @@ +135,5 @@ > + self._log.error("Exception in JSON.parse for _getAssertion: " + e); > + } > + self._gotEmails(emails, obj, cb, options); > + } > + obj.sandbox.importFunction(callback, "callback"); AFAIK you don't need the 2nd argument if the desired name already matches the function name. @@ +193,5 @@ > + ); > + let oldPerm = pm.testExactPermission(origin, "popup"); > + try { > + pm.add(origin, "popup", pm.ALLOW_ACTION); > + } catch(e) {} Is this worth logging? @@ +212,5 @@ > + self._log.error(msg); > + cb(new Error(msg), null); > + } > + > + // Set popup blocker permission to original value. If cb() above throws, you never get here. ::: testing/xpcshell/xpcshell.ini @@ +67,5 @@ > [include:widget/tests/unit/xpcshell.ini] > [include:content/base/test/unit/xpcshell.ini] > [include:content/test/unit/xpcshell.ini] > [include:toolkit/components/url-classifier/tests/unit/xpcshell.ini] > +[include:services/aitc/tests/unit/xpcshell.ini] Oops.

Note: Bug 753238 is the promised evolution of this patch. It is currently uncertain if that code will land in time for Fx15, and as the AitC code depends on this functionality, we will currently work on both patches in parallel. The module in this bug will serve as the fallback. Reviews still very much appreciated so we can be ready to land this!

Comment on attachment 619256 [details] [diff] [review] Common BrowserID Module - v4 Review of attachment 619256 [details] [diff] [review]: ----------------------------------------------------------------- Well this was fun. ::: services/identity/Makefile.in @@ +24,5 @@ > + > +libs:: > + $(NSINSTALL) -D $(module_dir) > + $(NSINSTALL) -R $(source_modules) $(module_dir) > + Custom rules go below the rules.mk include. ::: services/identity/browserid.js @@ +215,5 @@ > + > + // Set popup blocker permission to original value. > + try { > + pm.add(origin, "popup", oldPerm); > + } catch(e) {} Let's revert the permissions changes at the beginning of this function. There are exceptions that cannot be caught (e.g. out of memory, too much recursion?). @@ +228,5 @@ > + Cu.evalInSandbox(scriptText, sandbox, "1.8", ID_URI, 1); > + } > + > + // Sometimes the provided win hasn't fully loaded yet > + let cWin = win.wrappedJSObject; .wrappedJSObject here is unnecessary (and might lead to unexpected behavior, if the page replaced .document, .document.readyState, .addEventListener, or .removeEventListener). @@ +253,5 @@ > + if (!res) { > + let msg = "BrowserID.User.getAssertion empty assertion for " + email; > + self._log.error(msg); > + cb(new Error(msg), null); > + return; Should we be calling obj.free() here? Needs investigation. @@ +332,5 @@ > + delete this._sandbox; > + this._container.removeChild(this._frame); > + this._frame = null; > + this._container = null; > + }, We need to make sure that we call this before the content callback whereever applicable. We have no guarantee that execution will return from the content callback (even with try {} catch()) due to uncatchable exceptions. @@ +365,5 @@ > + let self = this; > + this._frame.addEventListener( > + "DOMContentLoaded", > + function _makeSandboxContentLoaded(event) { > + if (event.target.location.toString() != ID_URI) { Consider changing this to event.target == self._frame. ::: services/identity/identity-common.js @@ +1,1 @@ > +pref("identity.browserid.log", "Debug"); Are we going to ship with this set?

Tweaked version that should address all the comments above. Also has tests that we are able to check-in (in addition to the ones we can't in bug 749907). They all pass! (In reply to Gregory Szorc [:gps] from comment #32) > Please rename "obj" to "sandbox" or similar everywhere in this file. "obj" > is too generic. I wanted to avoid sandbox.sandbox everywhere, so I made the inner property "box" and renamed obj to sandbox. > > + let self = this; > > + new BrowserIDSandbox(function _gotSandbox(obj) { > > + self._getEmails(obj, cb, options); > > Shouldn't _getEmails() call obj.free()? Maybe put that in a try..finally > here? No, the sandbox is passed along to _gotEmails which will free it. (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #34) > @@ +253,5 @@ > > + if (!res) { > > + let msg = "BrowserID.User.getAssertion empty assertion for " + email; > > + self._log.error(msg); > > + cb(new Error(msg), null); > > + return; > > Should we be calling obj.free() here? Needs investigation. We should, moved to the top before we start any callbacks at all. > @@ +365,5 @@ > > + let self = this; > > + this._frame.addEventListener( > > + "DOMContentLoaded", > > + function _makeSandboxContentLoaded(event) { > > + if (event.target.location.toString() != ID_URI) { > > Consider changing this to event.target == self._frame. Doesn't work. JS object comparison and I don't get along :/ > ::: services/identity/identity-common.js > @@ +1,1 @@ > > +pref("identity.browserid.log", "Debug"); > > Are we going to ship with this set? Yes, I think so. I don't see any logs generated at all unless I manually add a DumpAppender, Greg?

In light of the native ID work, I'd like to refactor this patch to move browserid.js into the module directory for services-aitc instead of making services/identity. I think this will be much cleaner, since when we do start using the native module, we only have to delete a single file instead of a whole directory; while still keeping the AITC client in working condition, all in one place. I'll put up an updated patch ASAP.

Well, turns out I didn't actually upload the patch and it's been sitting in my local queue all week :/ Here it is.

Fixes bug 745345.

(In reply to Anant Narayanan [:anant] from comment #38) > Fixes bug 745345. Err, I meant bug 745415.

Comment on attachment 628588 [details] [diff] [review] Common BrowserID Module - v4.3 Final review pass! Getting close to the finish line...

Comment on attachment 628588 [details] [diff] [review] Common BrowserID Module - v4.3 Review of attachment 628588 [details] [diff] [review]: ----------------------------------------------------------------- Can you move the mochitest tests into a 'mochitest' subdirectory? This code looks pretty familiar. I /think/ we're good enough. I still defer to {gavin,khuey,bz,etc} for the DOM/docshell foo. ::: services/aitc/modules/browserid.js @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +const EXPORTED_SYMBOLS = ["BrowserID"]; Since there will be a new BrowserID interface in toolkit, should we change this name to be more specific? AitcBrowserID? @@ +165,5 @@ > + this._log.error(err); > + try { > + cb(new Error(err), null); > + } catch(e) { > + this._log.warn("Callback threw in _gotEmails " + e); CommonUtils.exceptionStr? @@ +370,5 @@ > + * (function) Callback to be invoked with a Sandbox, when ready. > + * @param uri > + * (String) URI to be loaded in the Sandbox. > + */ > +function Sandbox(cb, uri) { Some people might confuse this with Components.utils.Sandbox. It isn't exported from this module, so you should be fine. You may want to consider a rename, just in case. @@ +440,5 @@ > + uri, > + this._frame.docShell.LOAD_FLAGS_NONE, > + null, // referrer > + null, // postData > + null // headers Love that you documented the nulls. ::: services/aitc/tests/Makefile.in @@ +14,5 @@ > XPCSHELL_TESTS = unit > > include $(topsrcdir)/config/rules.mk > + > +_BROWSER_FILES = \ Despite what's in other Makefiles, the convention is for local variables to be lower-case. Upper-case variables may have special behavior provided by rules.mk, or similar.

That should be everything. Except: (In reply to Gregory Szorc [:gps] from comment #41) > > +const EXPORTED_SYMBOLS = ["BrowserID"]; > > Since there will be a new BrowserID interface in toolkit, should we change > this name to be more specific? AitcBrowserID? The toolkit one is going to be named Identity.jsm. The name BrowserID will be phased out - just like this patch! > > +function Sandbox(cb, uri) { > > Some people might confuse this with Components.utils.Sandbox. It isn't > exported from this module, so you should be fine. You may want to consider a > rename, just in case. Yes, I had that feeling when I wrote "sandbox.sandbox". But, again, this file is the only consumer of this API and this file's lifetime is 6 weeks, at most.

Trimmed some trailing whitespace and format patch correctly. https://tbpl.mozilla.org/?tree=Try&rev=bb09dd69d0a5

Comment on attachment 629253 [details] [diff] [review] Common BrowserID Module - v4.4.1 Bug landed, clearing flag.