952486 - Add a PermissionsCheck extended attribute to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

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.

Comment 1

[Peter Van der Beken [:peterv]]

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

Comment 2

[(no longer active)]

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!

Comment 3

[Reuben Morais [:reuben]]

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?

Comment 4

[(no longer active)]

(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.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: WIP

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?

Comment 7

[Reuben Morais [:reuben]]

Attachment: Implement CheckPermissions extended attribute

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.

Comment 8

[Reuben Morais [:reuben]]

Attachment: Convert several things to use CheckPermissions (and Pref) instead of Func

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?

Comment 9

[Reuben Morais [:reuben]]

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

Comment 10

[Reuben Morais [:reuben]]

Attachment: Implement CheckPermissions extended attribute, v2

Comment 11

[Reuben Morais [:reuben]]

Attachment: Convert several things to use CheckPermissions (and Pref) instead of Func, v2

Comment 12

[Reuben Morais [:reuben]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Reuben Morais [:reuben]]

Review ping?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Reuben Morais [:reuben]]

(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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 19

[Reuben Morais [:reuben]]

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.

Comment 20

[Reuben Morais [:reuben]]

Attachment: Implement CheckPermissions extended attribute, v3

Comment 21

[Reuben Morais [:reuben]]

Attachment: Convert several things to use CheckPermissions (and Pref) instead of Func, v3

Comment 22

[Reuben Morais [:reuben]]

https://tbpl.mozilla.org/?tree=Try&rev=48313b8136d3

Comment 23

[Reuben Morais [:reuben]]

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

Comment 24

[(no longer active)]

\o/

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

Comment 26

[[:fabrice] Fabrice Desré]

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!

Comment 27

[(no longer active)]

(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.)

Comment 28

[Reuben Morais [:reuben]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 30

[Reuben Morais [:reuben]]

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#CheckPermissions