Jed is going to take a look at this. :-)

Thanks to :bholley for going over the fundamentals with me today. I think I have what I need to get this done.

There is a property in the options dictionary passed to one of the BrowserID functions whose name is "want-issuer". This is not a good name, and in particular, the webidl parser hates it. I have opened Bug 1013383 to change it. I think there's time to do this before Firefox Accounts ships on B2G 2.0.

Just checking in - Jed, do you think it's realistic to have this landed within the next week? I've got some other important stuff blocked on it.

I would like to land bug 987111 sometime tomorrow. Jed, can you post that patch? Please let me know if you're having trouble with it.

Yes, sorry about the delay. I've been underwater. I ought to be able to have this done today, but I will almost certainly need your advice on one or two points. Thanks j

Great! Feel free to ping me, or send me mail if I'm not on IRC. I should respond quickly.

Currently, we instantiate an "internal" object, which creates an instance of the nsDomIdentity object and returns it from its init() method: https://mxr.mozilla.org/mozilla-central/source/dom/identity/nsDOMIdentity.js#749 What does the webidl interface definition have to look like to allow that?

As far as I can tell, the distinction between nsDOMIdentity and nsDOMIdentityInternal is that the functions of the latter are not supposed to be exposed to content. Given the access restriction of __exposedProps__, it's not clear to me why this was ever necessary. But either way, it definitely isn't necessary when you use WebIDL, because you'll only expose the functions you define in the .webidl file (and access to those will be mediated by the WebIDL boundary).

I had thought it was for mocking in tests, but on closer inspection, I don't see the 'internal' bit being used anywhere. So maybe we can change all that around.

First stab at a patch. This has not been fully tested yet. - Created Identity.webidl - Removed 'internal' object from nsDOMIdentity.js - Removed __exposedProps__ Some manual testing done in browser, but nothing more yet. :bholley, is this on the right track? Thanks for your help! j

Comment on attachment 8431985 [details] [diff] [review] Implement mozId with WebIDL (wip) Review of attachment 8431985 [details] [diff] [review]: ----------------------------------------------------------------- Looks good on a first pass! ::: dom/webidl/Identity.webidl @@ +10,5 @@ > +callback IdentityOnErrorCallback = void (DOMString error); > + > +dictionary IdentityWatchOptions { > + // Required callback > + IdentityOnLoginCallback onlogin; You should test, but I'm pretty sure that this can still be undefined. The ? just makes things nullable (so for strings, you can have undefined, null, empty string, or non-empty string), but I believe that all dictionary members are optional. So if you want to require certain parameters, you may need to check for them explicitly in the implementation and throw if need be. Again, test it. @@ +29,5 @@ > + DOMString privacyPolicy; > + > + // Optional parameters > + DOMString? backgroundColor; > + long? refreshAuthentication; Per the above, I don't think you really want everything here to be nullable.

(In reply to Bobby Holley (:bholley) from comment #11) > Comment on attachment 8431985 [details] [diff] [review] > Implement mozId with WebIDL (wip) > > Review of attachment 8431985 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good on a first pass! Thanks for looking! > You should test, but I'm pretty sure that this can still be undefined. The ? > just makes things nullable (so for strings, you can have undefined, null, > empty string, or non-empty string), but I believe that all dictionary > members are optional. So if you want to require certain parameters, you may > need to check for them explicitly in the implementation and throw if need > be. Again, test it. Oh excellent. Yes, this turns out to be the case. That makes things so much better. Patch updated. More to come.

Updated per Comment 11

try push in progress looks pretty good, but dom/identity/tests/mochitest/test_declareAudience.html and test_syntheticEvents.html are timing out. I'll look into this tomorrow.

Updated to fix breakage of dom/identity mochitests

Oops - removed debugging cruft

Comment on attachment 8432062 [details] [diff] [review] Implement navigator.mozId using WebIDL (wip) This patch implements the navigator.mozId API with WebIDL. :bholley, thanks for your review! :spenrose, note that this is not going to be uplifted to FxOS 2.0 - this is only to land in m-c for now. Can you help me ensure that I've got all of the Firefox Accounts parameters properly exposed and described? We'll have to decide what to do for native Persona support, which currently is only offered on FirefoxOS (not desktop). For that implementation, a bunch of awful changes to the API were made to enable Marketplace to use Persona. Now that Marketplace is moving to Firefox Accounts, we thankfully won't need to expose those non-standard parameters. I'll open a separate bug to work out the native Persona needs with :callahad, since the "goldilocks" API is moving a bit away from the stateful API we have here. Overall, since :bholley is blocked by this bug, I'd rather do a follow-up for the Persona implementation. Thank you both, j

Comment on attachment 8432062 [details] [diff] [review] Implement navigator.mozId using WebIDL (wip) Review of attachment 8432062 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks for pushing this through. ::: dom/webidl/Identity.webidl @@ +1,1 @@ > + Please add a license header at the top. @@ +6,5 @@ > +callback IdentityOnLogoutCallback = void (); > + > +callback IdentityOnCancelCallback = void (DOMString? error); > + > +callback IdentityOnErrorCallback = void (DOMString error); Please remove the newlines between these callback definitions. @@ +61,5 @@ > + void watch (optional IdentityWatchOptions options); > + void request (optional IdentityRequestOptions options); > + void logout (); > + > + // Legacy api Can we get a followup bug filed and assigned to stick the legacy API methods behind a separate pref dom.identity.exposeLegacyGetAPI or somesuch? This entire API is behind a pref right now. So if we decide to ship this in new places, we'll want to make sure that we don't accidentally expose the legacy API to new places. Let me know if you need help with any of the mechanics of this. @@ +63,5 @@ > + void logout (); > + > + // Legacy api > + void get (IdentityOnLoginCallback callback, optional IdentityGetOptions options); > + void getVerifiedEmail (IdentityOnLoginCallback callback); Remove the spaces between the method name and open-paren for all of these methods.

Comment on attachment 8432062 [details] [diff] [review] Implement navigator.mozId using WebIDL (wip) Review of attachment 8432062 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/Identity.webidl @@ +56,5 @@ > + > +[JSImplementation="@mozilla.org/identity/manager;1", > + NavigatorProperty="mozId", > + Pref="dom.identity.enabled"] > +interface IdentityManager { Oh! And you need [NoInterfaceObject] here (see the failing test on B2G ICS in [1]) https://tbpl.mozilla.org/?tree=Try&rev=2389a9b5113f

Comment on attachment 8432062 [details] [diff] [review] Implement navigator.mozId using WebIDL (wip) Review of attachment 8432062 [details] [diff] [review]: ----------------------------------------------------------------- Flagging bz for a 2-second super-review on Identity.webidl. I think I've covered everything, but I just want to make sure.

(In reply to Bobby Holley (:bholley) from comment #19) > Comment on attachment 8432062 [details] [diff] [review] > Implement navigator.mozId using WebIDL (wip) > > Review of attachment 8432062 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good! Thanks for pushing this through. Thanks again for your help! > ::: dom/webidl/Identity.webidl > @@ +1,1 @@ > > + > > Please add a license header at the top. Done > @@ +6,5 @@ > > +callback IdentityOnLogoutCallback = void (); > > + > > +callback IdentityOnCancelCallback = void (DOMString? error); > > + > > +callback IdentityOnErrorCallback = void (DOMString error); > > Please remove the newlines between these callback definitions. Done > @@ +61,5 @@ > > + void watch (optional IdentityWatchOptions options); > > + void request (optional IdentityRequestOptions options); > > + void logout (); > > + > > + // Legacy api > > Can we get a followup bug filed and assigned to stick the legacy API methods > behind a separate pref dom.identity.exposeLegacyGetAPI or somesuch? This > entire API is behind a pref right now. So if we decide to ship this in new > places, we'll want to make sure that we don't accidentally expose the legacy > API to new places. Sounds good. Done. > @@ +63,5 @@ > > + void logout (); > > + > > + // Legacy api > > + void get (IdentityOnLoginCallback callback, optional IdentityGetOptions options); > > + void getVerifiedEmail (IdentityOnLoginCallback callback); > > Remove the spaces between the method name and open-paren for all of these > methods. Fixed

(In reply to Bobby Holley (:bholley) from comment #20) > Comment on attachment 8432062 [details] [diff] [review] > Implement navigator.mozId using WebIDL (wip) > > Review of attachment 8432062 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/Identity.webidl > @@ +56,5 @@ > > + > > +[JSImplementation="@mozilla.org/identity/manager;1", > > + NavigatorProperty="mozId", > > + Pref="dom.identity.enabled"] > > +interface IdentityManager { > > Oh! And you need [NoInterfaceObject] here (see the failing test on B2G ICS Aha! Thank you - I didn't know about that attribute, and that test failure was confusing me. Fixed.

Addresses issues in Comments 19 and 20 r=bholley

Hmm... There's a lot of red in the B2G ICS test. I can't tell off-hand if this patch is responsible for that. I'll look into it. https://tbpl.mozilla.org/?tree=Try&rev=d5200d2c8758

... although, before it was Test 11 that was orange, and now it's passing :)

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #25) > Hmm... There's a lot of red in the B2G ICS test. I can't tell off-hand if > this patch is responsible for that. I'll look into it. > > https://tbpl.mozilla.org/?tree=Try&rev=d5200d2c8758 It's not related to your patch - it's also there on the TBPL from your base revision: https://tbpl.mozilla.org/?rev=cbe4f69c2e9c

Ah yes. Thanks for the confirmation, :bholley.

Comment on attachment 8432655 [details] [diff] [review] Implement mozId with WebIDL (wip) Review of attachment 8432655 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, Jed, thanks for doing this. The only lines in the IDL that puzzled me were these: +dictionary IdentityRequestOptions { + // Required parameters + DOMString termsOfService; + DOMString privacyPolicy; Here's what I see in request(): let optionalStringProps = ["privacyPolicy", "termsOfService"]; for (let propName of optionalStringProps) { if (!aOptions[propName] || aOptions[propName] === "undefined") continue; ... <snip> ... message[propName] = aOptions[propName]; which matches how I have used the code. So r=me if those parameters are moved to the block commented optional or you can tell me what I'm missing.

Comment on attachment 8432655 [details] [diff] [review] Implement mozId with WebIDL (wip) sr=jst (stealing request from bz)

Thanks spenrose, thanks jst!

r=spenrose sr=jst

sr=me, fwiw.

Feelin the love. bz, jst, bholley, spenrose, thank you all!

clearing checkin needed since bholley did the checkin :)

I think this is breaking gaia/b2g trunk tests. The provisioning API needs to be exposed. Reopening.

Backed out for causing bug 1019827. https://hg.mozilla.org/integration/b2g-inbound/rev/2b6b0ced08ef

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/2b6b0ced08ef

This is looking more difficult than I initially thought it would be. What's happening here is that there are two parts to the BrowserID API: The relying-party (RP) API, and the identity provider (IDP) API. The RP API consists of the methods that an RP would use: watch(), request(), logout(). The IDP API consists of all the identity provisioning and authentication flows, which are never called directly by the RP. The problem, I think, is that we provide a native implementation of the RP API in FirefoxOS, but *not* of the IDP API. Rather, the IDP logic is all executed inside a sandboxed iframe. This now does not work. I'm setting up a Persona instance that doesn't bind to navigator.mozId when performing IDP API functions in order to test whether this can be simply fixed on the server-side.

Well wait, I'm getting the same provisioning failure with a current m-c/gaia build with none of this Identity.webidl stuff in it. And I'm also getting it with a two-week-old image on my Flame. Checking in with ateam and gaia on irc to see what people are experiencing.

Jed, what's the status here? We need to get this landed.

Sam, who is the best person to help :bholley with this since Jed left? I think it might be you or maybe Jared, but I'm not sure.

Me. Bobby this turned out to be really hard. We can't change the API due to WebAPI veto, and Jed was not able to get the IDL working with the not-designed-that-way mozId code despite working on it for several days. (In reply to Chris Karlof [:ckarlof] from comment #44) > Sam, who is the best person to help :bholley with this since Jed left? I > think it might be you or maybe Jared, but I'm not sure.

(In reply to Sam Penrose from comment #45) > Me. Bobby this turned out to be really hard. We can't change the API due to > WebAPI veto Can you elaborate on the API change that would be required, and who is vetoing it? > and Jed was not able to get the IDL working with the > not-designed-that-way mozId code despite working on it for several days. Can you elaborate on the problem? What happened after comment 42? This needs to happen. The current API that the mozId stuff is using is going to be gone before the end of year.

(In reply to Bobby Holley (:bholley) from comment #46) > (In reply to Sam Penrose from comment #45) > > Me. Bobby this turned out to be really hard. We can't change the API due to > > WebAPI veto > > Can you elaborate on the API change that would be required, and who is > vetoing it? See https://bugzilla.mozilla.org/show_bug.cgi?id=1014077#c3 and comment 4. > > > and Jed was not able to get the IDL working with the > > not-designed-that-way mozId code despite working on it for several days. > > Can you elaborate on the problem? What happened after comment 42? I don't remember exactly. The gist of the problem was that writing an IDL to match the existing behavior of mozId was a task Jed was not able to complete after days of focused work. > This needs to happen. The current API that the mozId stuff is using is going > to be gone before the end of year. Given that a mozId expert has tried and failed to complete this task, can we ask an IDL expert to take it on? The API is not the hard part: expressing it in IDL is the hard part.

(In reply to Sam Penrose from comment #47) > (In reply to Bobby Holley (:bholley) from comment #46) > > (In reply to Sam Penrose from comment #45) > > > Me. Bobby this turned out to be really hard. We can't change the API due to > > > WebAPI veto > > > > Can you elaborate on the API change that would be required, and who is > > vetoing it? > > See https://bugzilla.mozilla.org/show_bug.cgi?id=1014077#c3 and comment 4. That bug was filed (and ehsan raised his objection) more than a week before jed landed his patch in comment 37, so I'm skeptical that this is actually related to the reason this patch was backed out. Can you articulate why that spec change is necessary to convert this API to WebIDL? > > > and Jed was not able to get the IDL working with the > > > not-designed-that-way mozId code despite working on it for several days. > > > > Can you elaborate on the problem? What happened after comment 42? > > I don't remember exactly. The gist of the problem was that writing an IDL to > match the existing behavior of mozId was a task Jed was not able to complete > after days of focused work. Jed did complete it (with a fair amount of my help) such that it was green (and landed) on mozilla-central. The patch was later backed out because of gaia test failures. Jed discusses these somewhat in comment 41 (suggesting the possibility of a server-side solution), and we need somebody familiar with the problem space to pick up his breadcrumbs. > Given that a mozId expert has tried and failed to complete this task, can we > ask an IDL expert to take it on? The API is not the hard part: expressing it > in IDL is the hard part. The API is already expressed in WebIDL. The issue is the gaia test failures. We need someone familiar with mozId to look at those failures, and either fix them (on the client or the server) or articulate (with specifics) the exact problem that needs to be solved with bindings machinery. In general, teams are responsible for the security of the code they own. The mozId API explicitly disables certain important security features, which I approved as a temporary measure in late May after Jed assured me that this API was days away from moving off of the deprecated machinery. That didn't happen, and needs to happen now.

Your arguments make sense. I did have the sense from Jed that the test failures were related to the IDL change in some important way, but I may be misremembering or he may be mistaken. I had not grokked that we had previously accepted responsibility for missing a deadline; my apologies. It's on my plate.

(In reply to Sam Penrose from comment #49) > Your arguments make sense. I did have the sense from Jed that the test > failures were related to the IDL change in some important way They're almost certainly related to the patch, which is what we need to investigate. But I'm quite confident that the issue doesn't stem from a fundamental incompatibility between mozId and WebIDL, given that WebIDL is the machinery used by pretty much every other web-exposed API at this point (modulo a few stragglers that I'm tracking down now). > misremembering or he may be mistaken. I had not grokked that we had > previously accepted responsibility for missing a deadline; my apologies. > It's on my plate. That's awesome, thank you! My IRC time will be a bit sporadic this week because I'm taking care of someone in the hospital, but I should be very responsive to email. If you have any questions about anything (how stuff works, why something behaves in a given why, how to fix an issue you've identified), don't hesitate to ping me - I should respond in a couple of hours. I can also Vidyo if you like, so long as you don't mind uncertain timing and the possibility that the call will be interrupted by an overzealous nurse. ;-)

Rebased; Gaia UITest for Persona still broken.

(In reply to Sam Penrose from comment #52) > Created attachment 8469456 [details] > Link to Gaia PR tweaking UITest Gaia try build is here: https://tbpl.mozilla.org/?rev=6dd7299ab04a8b967787428083347432945f73dc&tree=Gaia-Try

Jared the breakage on the Gaia try appears unrelated to this change -- can we merge it?

(In reply to Sam Penrose from comment #54) > Jared the breakage on the Gaia try appears unrelated to this change -- can > we merge it? Hey Sam - I'm not totally sure whether those emulator builds should be red at this point. Maybe RyanVM has thoughts on whether this is good to merge. I feel funny about not having an r+ from someone, so I'll go ahead and r+ it. I'll also set checkin-needed optimistically ^_^

Comment on attachment 8469456 [details] Link to Gaia PR tweaking UITest Not sure who owns the uitest dev_app, but this change is very FxA-specific and looks good to me.

Those look infrastructure-related. I retriggered the failed builds. Note that if you have the ability to push to Try, you should be able to do that as well. The link below describes that along with some other info you might find useful: https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Jared, can you merge the Gaia PR for me? It needs to land first.

Jared, can you merge the Gaia PR for me? It needs to land first.

Hey Sam, Ah, sorry about that - didn't see your comment last week. Hmm, it looks like the gaia-try results are gone, so you'll need to retrigger the build by force-pushing to the branch; it probably wouldn't hurt to rebase against current master, since this branch is rather old. Once that's done, and gaia-try is looking green, please ni? me again, and I'll be happy to merge the branch. Also feel free to ping me if you are fuzzy on any part of this process ^_^ Cheers, Jared

Getting back to this; thanks Jared.

Gaia-try looks good (except for two known bugs in Gij), merging. Thanks Sam! Master: https://github.com/mozilla-b2g/gaia/commit/fcdb64708e89010f53d972ae3a9cfe767dc02614

We still need to land the actual patch, right?

(In reply to Bobby Holley (:bholley) from comment #64) > We still need to land the actual patch, right? Yes. I'm in Tribe today but will see if it needs rebasing and set checkin-needed tomorrow.

Doh! I guess I usually merge and close all at once, sorry about that :-P

[Blocking Requested - why for this release]: remove security exceptions per previous comments. Note that this patch is for 2.1 only. I will upload a rebased patch for mozilla-central.

Hey Sam - Has the final webidl patch been reviewed? I don't see an r+.

Yes, it's r=me.

The patch needs rebasing. A fresh Try push would be nice too.

For Aurora, not m-c https://tbpl.mozilla.org/?tree=Try&rev=2e48bcf770f0

Triage group decided blocking+ due to comment 48, for security concerns.

Comment on attachment 8497645 [details] [diff] [review] (Aurora) 1011084-mozId-webidl.patch Approval Request Comment [Feature/regressing bug #]: webidl security [User impact if declined]: reduced security [Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=2e48bcf770f0 [Risks and why]: reworks DOM API, general corner case risk [String/UUID change made/needed]: no

Needs rebasing.

v2.1: https://github.com/mozilla-b2g/gaia/commit/d4d54850334c1222b505c99ee27e53aef157c5d5

This commit did not have a bug number, and when trying to track down some relevant changes it made it hard to view the history. Please make sure all commits to gaia have the proper bug number listed in the commit message in the future. I've gone ahead and backed this out and re-landed with an appropriate commit message. Backout: https://github.com/mozilla-b2g/gaia/commit/24c2a0530728e5461572371d7ada49f165b76b0c Re-land: https://github.com/mozilla-b2g/gaia/commit/075fa520b61246643186e5604e02e75e239d7e6b

As discussed on IRC, let's get this landed on trunk first before pushing to Aurora.

Rebased for mozilla-central. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3e14cdec257b

Requesting checkin for the "-mc" patch, which is clean on try.

Sorry about that, Kevin. Thanks for taking care of it. (In reply to Kevin Grandon :kgrandon from comment #76) > This commit did not have a bug number, and when trying to track down some > relevant changes it made it hard to view the history. Please make sure all > commits to gaia have the proper bug number listed in the commit message in > the future. I've gone ahead and backed this out and re-landed with an > appropriate commit message. > > Backout: > https://github.com/mozilla-b2g/gaia/commit/ > 24c2a0530728e5461572371d7ada49f165b76b0c > Re-land: > https://github.com/mozilla-b2g/gaia/commit/ > 075fa520b61246643186e5604e02e75e239d7e6b