1179718 - Add a CheckAllPermissions extended attribute to WebIDL

Status: RESOLVED FIXED

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

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

The CheckPermissions extended attribute implemented in bug 952486 have a "or" relationship of the listed permissions; that is, any one of the listed permission is enough for the interface or interface member to be exposed.

I'd like to add a new CheckAllPermissions attribute that enforces "and" relationship of the listed permissions. So the content has to have all the listed permissions for the interface or interface member to be exposed. We already have some hierarchical permissions implemented but couldn't use WebIDL to express them instead have to check the sub-permission in code.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sounds reasonable to me.  kanru, do you want to do this yourself?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Sounds reasonable to me.  kanru, do you want to do this yourself?

Ah, I want to give it a try, it should be interesting.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r?bz

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

https://reviewboard.mozilla.org/r/12545/#review11153

::: dom/bindings/BindingUtils.h:3135
(Diff revision 1)
>  CheckPermissions(JSContext* aCx, JSObject* aObj, const char* const aPermissions[]);

I think we should rename the existing CheckPermissions to CheckAnyPermissions to make it clearer what's going on.  Followup patch is ok for this if you're prefer that.

::: dom/bindings/Codegen.py:11454
(Diff revision 1)
> +        if len(descriptor.allpermissions):

Seems like this could be factored out into a function that takes the "anypermissions" or "allpermissions" string and does the right thing...

::: dom/bindings/Configuration.py:569
(Diff revision 1)
> +            # Adds a allpermission list to this descriptor and returns the index to use.

This should _definitely_ be factored out some.

There's some code duplication in the parser too that might make sense to factor out; I can't tell for sure from these diffs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

https://reviewboard.mozilla.org/r/12547/#review11159

Ship It!

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r?bz

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

https://reviewboard.mozilla.org/r/12545/#review11867

::: dom/bindings/Codegen.py:1903
(Diff revision 2)
> +        assert checkAllPermissions is None  or isinstance(checkAllPermissions, int)

Extra space after "None" here.  Please remove it.

::: dom/bindings/Codegen.py:11451
(Diff revision 2)
> -            for (k, v) in sorted(descriptor.permissions.items()):
> +            permissions = getattr(descriptor, name, None)

I don't think you need to pass the None there.  The descriptor will always have the attr, no?

::: dom/bindings/Configuration.py:553
(Diff revision 2)
> -            def addPermissions(ifaceOrMember):
> +            def addPermissions(ifaceOrMember, checkAll=False):

You only have two callers, one of which passes true and the other passes false.  Just make checkAll a required argument.

Also, it's probably better to make it the "attribute" string and then just set "permissions" based on that.  Avoids the weird boolean argument thing where you can't tell what it means at the callsite.

::: dom/bindings/parser/WebIDL.py:1177
(Diff revision 2)
> +        if (self.getExtendedAttribute("CheckAllPermissions") and

This should really be changed to have a list of primary-global-only attributes (just two of them for now?) and walk it, raising exceptions as needed.

::: dom/bindings/parser/WebIDL.py:3381
(Diff revision 2)
> +        if (self.getExtendedAttribute("CheckAllPermissions") and

And here too, should just have a list of primary-global-only things.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8633277 [details]
MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

https://reviewboard.mozilla.org/r/13195/#review11881

Ship It!

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8633277 [details]
MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

https://reviewboard.mozilla.org/r/12545/#review12093

Ship It!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c07736ee7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/009f8711fd6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd71c32a6cb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7c07736ee7a
https://hg.mozilla.org/mozilla-central/rev/009f8711fd6c
https://hg.mozilla.org/mozilla-central/rev/4dd71c32a6cb