We should generate checks for permissions if an interface is only available/usable if a permissions check succeeds. This could also replace the code in Navigator::DoNewResolve.

Oh, and we should try to make test_interfaces.html test this.

Peter, are you still working on this? It would be really awesome if we managed to get this into the tree, this will help us code-gen some of the stuff for the hasFeature API we're discussing on dev-webapi. Thanks!

Can I steal this bug? I started to hack a patch together but I have no idea if the changes I'm making would be accepted. I'm adding a new ExtendedAttributeStringList syntax for extended attributes, it looks like this: [PermissionsCheck("contacts-read","contacts-write","contacts-create")] interface Foo { }; And it codegens this call in CGConstructorEnabled: CheckPermission(aObj, "contacts-read", "contacts-write", "contacts-create"); Thoughts?

(In reply to Reuben Morais [:reuben] from comment #3) > Can I steal this bug? > > I started to hack a patch together but I have no idea if the changes I'm > making would be accepted. I'm adding a new ExtendedAttributeStringList > syntax for extended attributes, it looks like this: > > [PermissionsCheck("contacts-read","contacts-write","contacts-create")] > interface Foo { }; > > And it codegens this call in CGConstructorEnabled: > > CheckPermission(aObj, "contacts-read", "contacts-write", > "contacts-create"); That looks good to me. This means ANDing various permissions right? (The right people to answer your question are Peter and Boris though.)

General idea seems fine to me. Just coordinate with Peter, please.

Here's the patch that I had been working on, I'll try to update it to a current tree. (In reply to Reuben Morais [:reuben] from comment #3) > [PermissionsCheck("contacts-read","contacts-write","contacts-create")] I do |PermissionsCheck="permission1 permission2"|. A whitespace-separated list should be fine, no?

Peter's patch, rebased and fixed. Permissions are OR'ed instead of AND'ed, codegen appends a nullptr to the permission list so we don't read past the end of the buffer. I also removed the cached permission manager pointer from nsContentUtils because that causes a recursive initialization of the layout module - NS_InitXPCOM2 initializes nsContentUtils which tries to GetService PermissionManager which tries GetService stuff that isn't initialized yet.

Not sure what's the best way to split this stuff. A single patch here? A single patch in a different bug? Multiple patches in this bug? A bug per interface?

Broader Try run than what I did during development: https://tbpl.mozilla.org/?tree=Try&rev=5c917ae0ebc7

Comment on attachment 8418770 [details] [diff] [review] Convert several things to use CheckPermissions (and Pref) instead of Func, v2 Again, feel free to suggest better ways to split this up if this is not appropriate.

I'm going to try to look at this on Monday, but in the meantime I'm trying to decide how OK I am with adding yet more branching to isEnabled and whether there's a better way to do that bit.

Review ping?

Comment on attachment 8418769 [details] [diff] [review] Implement CheckPermissions extended attribute, v2 >+++ b/dom/bindings/BindingUtils.cpp Yeah, sorry for the lag. I filed bug 1011826 to avoid holding this up even more. >+ if (global.Failed()) { Then it threw a security exception. And we have no way to deal with that here, so really shouldn't use GlobalObject. Just use xpc::WindowGlobalOrNull(rootedObj) to get a window, please. >+++ b/dom/bindings/BindingUtils.h Please document the behavior of the new method. >+++ b/dom/bindings/Codegen.py >+ descriptor.checkPermissionsIndexesForMembers.get(interfaceMember.identifier.name)) s/Indexes/Indices/? >+ lastCondition = getCondition(array[0], self.descriptor) # So we won't put a specTerminator > # at the very front of the list. Please fix the indent. >+ for (k, v) in descriptor.permissions.iteritems(): This is not going to produce stable output each time codegen is run, unless I'm missing something. That needs to be fixed; we need deterministic output. >+ perms = CGList((CGGeneric("\"%s\"," % p) for p in k), joiner="\n") Use single quotes in python as needed to avoid needing to escape double quotes? >+++ b/dom/bindings/Configuration.py >+ # It's a list of strings Whitespace-separated strings? >+ permissionsList = tuple(set(sorted(permissionsList))) This once again looks nondeterministic. Shouldn't it be tuple(sorted(Set(permissionsList)))? r=me with those fixed.

Comment on attachment 8418770 [details] [diff] [review] Convert several things to use CheckPermissions (and Pref) instead of Func, v2 >+++ b/dom/base/Navigator.cpp >-Navigator::HasNFCSupport(JSContext* /* unused */, JSObject* aGlobal) >- // Do not support NFC if NFC content helper does not exist. >- nsCOMPtr<nsISupports> contentHelper = do_GetService("@mozilla.org/nfc/content-helper;1"); >- if (!contentHelper) { >- return false; This part has gotten lost. Why is that OK? >+++ b/dom/webidl/Downloads.webidl >-// XXXTODO: When we have a generic way to do feature detection in marketplace Do we have that now? Is it being used to replace this stuff? >+++ b/dom/webidl/MozNFC.webidl > [NoInterfaceObject, >- Func="Navigator::HasNFCManagerSupport"] >+ CheckPermissions="nfc-manager"] > interface MozNFCManager { This doesn't make sense to me, before or after your changes: if it's NoInterfaceObject, what are we controlling with the permission? Please file a followup bug on this. >+++ b/dom/webidl/Navigator.webidl >-[NoInterfaceObject] >+[NoInterfaceObject, >+ Pref="dom.battery.enabled"] > interface NavigatorBattery { ... >- [Throws, Func="Navigator::HasBatterySupport"] >+ [Throws] > readonly attribute BatteryManager? battery; This seems wrong. You want the pref on the member, not on the interface here, no. I assume we have no tests for this? r=me with those fixed, but I want to understand the NFC situation.

(In reply to Boris Zbarsky [:bz] from comment #16) > >+++ b/dom/base/Navigator.cpp > >-Navigator::HasNFCSupport(JSContext* /* unused */, JSObject* aGlobal) > >- // Do not support NFC if NFC content helper does not exist. > >- nsCOMPtr<nsISupports> contentHelper = do_GetService("@mozilla.org/nfc/content-helper;1"); > >- if (!contentHelper) { > >- return false; > > This part has gotten lost. Why is that OK? I'm hoping "because it passes Try" is enough. Dimi Lee, is removing this piece of code OK? It doesn't seem to affect any tests. > >+++ b/dom/webidl/Downloads.webidl > >-// XXXTODO: When we have a generic way to do feature detection in marketplace > > Do we have that now? Is it being used to replace this stuff? Kinda, bug 983502 is supposed to be something like that. I shouldn't have converted DOMDownloadManager, only DOMDownload (which was already using [Func]). I'll remove this bit. > >+++ b/dom/webidl/MozNFC.webidl > > [NoInterfaceObject, > >- Func="Navigator::HasNFCManagerSupport"] > >+ CheckPermissions="nfc-manager"] > > interface MozNFCManager { > > This doesn't make sense to me, before or after your changes: if it's > NoInterfaceObject, what are we controlling with the permission? > > Please file a followup bug on this. Filed bug 1015457. > >+++ b/dom/webidl/Navigator.webidl > >-[NoInterfaceObject] > >+[NoInterfaceObject, > >+ Pref="dom.battery.enabled"] > > interface NavigatorBattery { > ... > >- [Throws, Func="Navigator::HasBatterySupport"] > >+ [Throws] > > readonly attribute BatteryManager? battery; > > This seems wrong. You want the pref on the member, not on the interface > here, no. I assume we have no tests for this? Looks like we don't have any tests for the case where the pref is unset. I'll fix this.

> I'm hoping "because it passes Try" is enough. Probably just means there is no coverage? As in, no tests try to poke this API on non-gonk (conditional on it existing there, of course), where it would throw exceptions all over afaict.

It looks like we do run the NFC marionette tests on emulator, conditional on if ('mozNfc' in window.navigator) { If we tried to do window.navigator.mozNfc on a non-gonk configuration, it shouldn't cause any problems, since all the code is only built if MOZ_NFC is defined. Or maybe the problem is gonk without hardware support? So when the "ro.moz.nfc.enabled" property is unset… Hm, I see. So it looks like that code needs to stay. I'll undo that part of the patch.

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/1dacf5af46ed remote: https://hg.mozilla.org/integration/b2g-inbound/rev/ce4bb3a18d03

\o/ Reuben, do you mind updating https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings please?

I had to backout since it's breaking all devices and emulator builds (probably all non unified builds). https://hg.mozilla.org/integration/b2g-inbound/rev/cbc896965ae0 Note that I had to keep a dom reviewer to be able to land that. We should probably relax the rule for backouts!

(In reply to comment #26) > Note that I had to keep a dom reviewer to be able to land that. We should > probably relax the rule for backouts! Sorry, I've already fixed that, but we're currently waiting for someone to deploy the updated hook (bug 1013983.)

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/8a66b707dd5a remote: https://hg.mozilla.org/integration/b2g-inbound/rev/4136b65c1c4b

https://hg.mozilla.org/mozilla-central/rev/8a66b707dd5a https://hg.mozilla.org/mozilla-central/rev/4136b65c1c4b