(no longer active) Reporter
	Description • 11 years ago

Building up on the same idea as in bug 958667, we need to be able to declare a permission as a prerequisite for a method/attribute/interface to be exposed.  Strawman proposal: [Permission="foobar"].

Currently we do this by using a [Func] attribute to point to some custom C++ code implementing the permission check, in a manner similar to what Navigator::CheckPermission does.  We sometimes duplicate this kind of logic in other places too, such as <http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraManager.cpp#75>.

Again, the codegen'ed check here should do a logical and with other similar attributes which affect the name visibility, such as [Pref], [Func] and [AvailableIn].

One complication is how this would interact with [NavigatorProperty].  We can either codegen something that Navigator::DoNewResolve would call into, or fail the codegen in that case and solve this in a follow-up.  As far as I can see, we only use permission checks for navigator.mozSettings and navigator.mozDownloadManager through the resolve hook.

Once we have this available, we should rewrite the existing permission checks on top of this.  I would be happy to help out with that.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 11 years ago

> One complication is how this would interact with [NavigatorProperty].

Just fine. The resolve hook already checks the enabled func, so it would just work with this new setup.

Doing this on top of bug 958667 for NavigatorProperty and interface objects is easy.  For methods/properties it's a bigger problem, unless we want to disallow some combinations of Func/AvailableIn/Permission or unless we add more stuff to Prefable.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 11 years ago

Oh, a second complication, actually: we need to think a bit about how to find the right window to use for the permissions check...

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

But generally, I expect we can crib what navigator bits do.

	(no longer active) Reporter
	Comment 4 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #1)
> > One complication is how this would interact with [NavigatorProperty].
> 
> Just fine. The resolve hook already checks the enabled func, so it would
> just work with this new setup.

Great!

> Doing this on top of bug 958667 for NavigatorProperty and interface objects
> is easy.  For methods/properties it's a bigger problem, unless we want to
> disallow some combinations of Func/AvailableIn/Permission or unless we add
> more stuff to Prefable.  :(

I don't know enough about the codegen code to tell which option is better, so please pick the easiest/best option! :-)

	(no longer active) Reporter
	Comment 5 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #2)
> Oh, a second complication, actually: we need to think a bit about how to
> find the right window to use for the permissions check...

I think we always get it from the global passed to the codegen'ed [Func].  (Except of course for workers.)

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 11 years ago

> so please pick the easiest/best option

Easiest is only allowing two of them.  Best is to allow all three.  ;)

	(no longer active) Reporter
	Comment 7 • 11 years ago

(In reply to comment #6)
> > so please pick the easiest/best option
> 
> Easiest is only allowing two of them.  Best is to allow all three.  ;)

If I had to pick, I would say leave out [Func] for now.  I would expect that for most cases, [AvailableIn] and [Permission] should make it unnecessary to write a custom function.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

Oh, I can support any two, on a per-property basis, with the stuff in bug 958667.  So you'd be able to do Func and Permission or Func and AvailableIn or Permission and AvailableIn.  Just not all three at once.  ;)

	(no longer active) Reporter
	Comment 9 • 11 years ago

(In reply to comment #8)
> Oh, I can support any two, on a per-property basis, with the stuff in bug
> 958667.  So you'd be able to do Func and Permission or Func and AvailableIn or
> Permission and AvailableIn.  Just not all three at once.  ;)

OK I see, thanks for the explanation.  That sounds good to me.

	Peter Van der Beken [:peterv]
	Comment 10 • 11 years ago

Is this not a dupe of bug 952486?

	(no longer active) Reporter
	Comment 11 • 11 years ago

That bug seems to be filed for interfaces and not other things, but please feel free to dupe in either direction if needed.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

So have we decided what we want here, exactly?  Permission="single string here" for a start, listing just one permission name?

	Peter Van der Beken [:peterv]
	Comment 13 • 10 years ago

The patch I have been working on (for interfaces, but not too hard to support properties) supports a list of permissions and the checks for them are or'ed, which seems to support most (all?) cases that we have.

	(no longer active) Reporter
	Comment 14 • 10 years ago

(In reply to comment #13)
> The patch I have been working on (for interfaces, but not too hard to support
> properties) supports a list of permissions and the checks for them are or'ed,
> which seems to support most (all?) cases that we have.

That sounds good to me.

	(no longer active) Reporter
	Comment 15 • 10 years ago

I think bug 952486 did everything we need here.