793204 - PermissionSettings interface needs 'remove' API method

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[David Dahl :ddahl]

nsIDOMPermissionSettings needs a remove API method so it is more clear that we are removing permissions instead of "remove" being "set to 'unknown'"

Comment 1

[David Dahl :ddahl]

Note: this is a follow-up for bug 770731

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think this is actually a blocker. The workaround of setting "unknown" used in bug 758269 is good enough for now until we start doing more complex stuff with settings where we need settings that affect subdomains etc.

For now we can just leave the bug 758269 workaround.

Comment 3

[Reuben Morais [:reuben]]

Is this bug still valid? The code suggests that setting to "unknown" no longer removes the permission.

Comment 4

[David Dahl :ddahl]

(In reply to Reuben Morais [:reuben] from comment #3)
> Is this bug still valid? The code suggests that setting to "unknown" no
> longer removes the permission.

Its really an ergonomics thing as the API would be more explicit if there was a remove method.

Comment 5

[Reuben Morais [:reuben]]

(In reply to David Dahl :ddahl from comment #4)
> Its really an ergonomics thing as the API would be more explicit if there
> was a remove method.

Right, what I really wanted to ask was: what is the current working mechanism for removing permissions? nsIPermissionManager does expose a remove() method, but PermissionSettings doesn't use it, and throws if set() is called with a value of "unknown".

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, we should either add a "remove" function, or change the .set function such that it allows "unknown" to be used to remove permissions.

However note that we should probably only allow removing permissions for principals where principal.appStatus==NOT_INSTALLED

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Either way, we should probably internally simply send a "PermissionSettings:AddPermission" message with "unknown" as the value. And change the code at [1] to permit the relevant change.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionSettings.jsm#48

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Either way, we should probably internally simply send a
> "PermissionSettings:AddPermission" message with "unknown" as the value. And
> change the code at [1] to permit the relevant change.
> 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/permission/
> PermissionSettings.jsm#48

Removing a permission with this API only makes sense if the permission is an explicit one and in that case the workaround (correct way if inBrowser==false) would be setting the permission to "prompt" as we're doing now. Removing an implicit permission would only make sense if implicit permissions were inheritable by hosted/inBrowser==true pages, which I hope they aren't.

So actually the correct change would be either to allow sending unknown *only* for hosted (inBrowser==true) pages, or just to set the value to prompt also for that pages (and no change is needed then).

The second option would have the advantage (or disadvantage, depending on how you look at it) that you keep the history of what sites you've ever given permissions to.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note the second paragraph of comment 6. I think we should only allow this for pages where principal.appStatus==NOT_INSTALLED. This means pages that are have the browserFlag==true and pages which are from a different origin than the app.

I agree we could allow setting these to prompt. But that does have privacy implications since it leaves history around forever, and it means more complexity in the frontend since you likely don't want to forever accumulate websites for which you show geolocation status.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #9)
> 
> I agree we could allow setting these to prompt. But that does have privacy
> implications since it leaves history around forever, and it means more
> complexity in the frontend since you likely don't want to forever accumulate
> websites for which you show geolocation status.

That's why I said it could be an advantage or disadvantage :) On one hand I get to keep the story and can remove or give permissions without revisiting the page first. On the other hand... well, I'm keeping the story and maybe I don't want it to remember where I've been.

But in the end as I said on bug 829397 I think we're going to need both solutions: A way to clear the app data, which would clear the permission story also, and a way to adjust the permissions on a page by page way. If we have both things then the privacy issue is solved without fixing this bug.

Comment 11

[Reuben Morais [:reuben]]

Attachment: nsIDOMPermissionSettings.remove(), WIP

This implements remove() as suggested in comment 6. Regarding comment 10, I assume if we can solve the privacy problem by implementing the solutions to clear permissions per app/per page, then this solution is OK.

Right now, removePermission() fails silently if principal.appStatus is not APP_INSTALLED. Is that OK?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 703966 [details] [diff] [review]
nsIDOMPermissionSettings.remove(), WIP

Review of attachment 703966 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/permission/PermissionSettings.jsm
@@ +136,5 @@
>          return "unknown";
>      }
>    },
>  
> +  removePermission: function removePermission(aPermName, aManifestURL, aOrigin, aBrowserFlag) {

This function doesn't seem used or needed. Just remove it.

Comment 13

[Gregor Wagner [:gwagner]]

Comment on attachment 703966 [details] [diff] [review]
nsIDOMPermissionSettings.remove(), WIP

Review of attachment 703966 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/permission/PermissionSettings.jsm
@@ +68,1 @@
>      

nit: use {} and remove whitespace

Comment 14

[Reuben Morais [:reuben]]

Attachment: nsIDOMPermissionSettings.remove()

This version fixes The Missing Comma that broke every test ever, and makes PermissionsInstaller and one of the tests that were using set-to-unknown use the remove API.

Carrying r=sicking.

Comment 15

[Reuben Morais [:reuben]]

Attachment: nsIDOMPermissionSettings.remove(), without debugging messages

Comment 16

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2027e3e07448

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

There are no automated tests included for the success case of this new function. The only test included is the fail use case (which BTW should have been added and not just replaced the original test with addPermission, because they're different if related tests).

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Reuben Morais [:reuben] from comment #14)
> Created attachment 705183 [details] [diff] [review]
> nsIDOMPermissionSettings.remove()
> 
> This version fixes The Missing Comma that broke every test ever, and makes
> PermissionsInstaller and one of the tests that were using set-to-unknown use
> the remove API.

Also, as PermissionSettingsModule.remove is implemented in the patch, this change will break the installer. 
+    this._internalAddPermission(data, false);

should be
+    this._internalAddPermission(data, true);

Otherwise the removes for installed, privileged and certified apps will fail, and they should not.

And I'm not sure the aBrowserFlag parameter is needed (or even desirable) on PermissionSettings.remove. Shouldn't that flag be always true for the valid use cases?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2027e3e07448

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

Er this isn't fixed. The change as has landed breaks the reinstall of applications. In fact I don't think it should have landed at all since the changes were enough to need another review, but that's just IMO. 

In any case, this change should be backed out until it's fixed.

Comment 21

[Gregor Wagner [:gwagner]]

(In reply to Antonio Manuel Amaya Calvo from comment #20)
> Er this isn't fixed. The change as has landed breaks the reinstall of
> applications. In fact I don't think it should have landed at all since the
> changes were enough to need another review, but that's just IMO. 
> 
> In any case, this change should be backed out until it's fixed.

I told Reuben to land without a new review.
I will back it out again.

Comment 22

[Gregor Wagner [:gwagner]]

Backout again: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef03bfa48a45

Comment 23

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/mozilla-central/rev/99dabf8d5e98

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Gregor Wagner [:gwagner] from comment #22)
> Backout again:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ef03bfa48a45

https://hg.mozilla.org/mozilla-central/rev/ef03bfa48a45

Comment 25

[Reuben Morais [:reuben]]

Attachment: nsIDOMPermissionSettings.remove() v2

(In reply to Antonio Manuel Amaya Calvo from comment #18)
> Also, as PermissionSettingsModule.remove is implemented in the patch, this
> change will break the installer. 

Thanks, fixed.

(In reply to Antonio Manuel Amaya Calvo from comment #17)
> There are no automated tests included for the success case of this new
> function. The only test included is the fail use case (which BTW should have
> been added and not just replaced the original test with addPermission,
> because they're different if related tests).

We should either re-phrase the comments on that test to something like "setting to unknown should fail", or replace it with the equivalent test using remove(). This patch does the latter.

I sent to this Try because it depends on a change that may break on Windows. Will ask for review once the results appear.

Comment 26

[Reuben Morais [:reuben]]

Attachment: nsIDOMPermissionSettings.remove() v2, with tests

A bunch of whitespace fixes creeped in, but those shouldn't be there in the first place so I hope that's OK.

sicking: note that I included a change to PermissionsInstaller, switching it to use the method you asked me to remove.

Antonio: Sorry for the miscommunication before, the parent/child relation and the details of the security model weren't very clear to me when writing the first iteration of this patch. I think this patch meets all the requirements listed here.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 706199 [details] [diff] [review]
nsIDOMPermissionSettings.remove() v2, with tests

Review of attachment 706199 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/permission/PermissionSettings.js
@@ +111,5 @@
> +    let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> +    let principal = secMan.getAppCodebasePrincipal(uri, appID, true);
> +
> +    if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED ||
> +        !this.isExplicit(aPermName, aManifestURL, aOrigin, true)) {

You shouldn't be allowed to remove explicit permissions. This check should just be

if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) {
  ...

::: dom/permission/PermissionSettings.jsm
@@ +65,5 @@
> +    let deleteAllowed = true;
> +    if (aAction === "unknown")
> +      deleteAllowed = (aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED);
> +
> +    return deleteAllowed &&

This would be more clearly written as:

return (aAction !== "unknown" ||
        aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) &&
        (perm !== Ci.nsIPermissionManager.UNKNOWN_ACTION) &&
        isExplicit;


Hmm.. though shouldn't the logic really be:

return (aAction === "unknown" &&
        aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) ||
       (aAction !== "unknown" &&
        (perm !== Ci.nsIPermissionManager.UNKNOWN_ACTION) &&
        isExplicit);

Comment 28

[Reuben Morais [:reuben]]

(In reply to Jonas Sicking (:sicking) from comment #27)
> Comment on attachment 706199 [details] [diff] [review]
> nsIDOMPermissionSettings.remove() v2, with tests
> 
> Review of attachment 706199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/permission/PermissionSettings.js
> @@ +111,5 @@
> > +    let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> > +    let principal = secMan.getAppCodebasePrincipal(uri, appID, true);
> > +
> > +    if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED ||
> > +        !this.isExplicit(aPermName, aManifestURL, aOrigin, true)) {
> 
> You shouldn't be allowed to remove explicit permissions. This check should
> just be
> 
> if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) {
>   ...

If we remove the check then trying to remove an explicit permission will cause the child process to be killed. Is that intended?

> Hmm.. though shouldn't the logic really be:
> 
> return (aAction === "unknown" &&
>         aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) ||
>        (aAction !== "unknown" &&
>         (perm !== Ci.nsIPermissionManager.UNKNOWN_ACTION) &&
>         isExplicit);

Yes, that's what my code does, except I extracted the "action is or isn't delete" part to avoid having the two similar checks in in the return statement, because that made it more easily readable IMO. I'll inline it.

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 706199 [details] [diff] [review]
nsIDOMPermissionSettings.remove() v2, with tests

Review of attachment 706199 [details] [diff] [review]:
-----------------------------------------------------------------

r+, addressing the comments.

::: dom/permission/PermissionSettings.js
@@ +111,5 @@
> +    let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> +    let principal = secMan.getAppCodebasePrincipal(uri, appID, true);
> +
> +    if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED ||
> +        !this.isExplicit(aPermName, aManifestURL, aOrigin, true)) {

I don't think that allows to remove explicit permissions... it just fails when the permission is implicit even if the app is not installed. Then again, I don't think not installed apps get granted any implicit permissions so I agree the condition should be changed.

::: dom/permission/PermissionSettings.jsm
@@ +65,5 @@
> +    let deleteAllowed = true;
> +    if (aAction === "unknown")
> +      deleteAllowed = (aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED);
> +
> +    return deleteAllowed &&

Yeah, the logic should most definitely be the last one. Allow the operation if it's an allowed delete OR if it was allowed with the previous version logic.

::: dom/permission/tests/test_permission_basics.html
@@ +73,2 @@
>      try {
> +      mozPermissions.remove(testPerm, certAppManifest, originCert);

Leave it as it was and just change the error to 'Setting a permission to unknown'. This test is needed also as it was and I saw you added specific tests for remove. By the way, this test used privAppManifest and originPriv because that permission is explicit and thus more lenient. Don't change it to certApp, please.

@@ +90,5 @@
> +
> +    testRemove(explicitPerm, privAppManifest, originOther,
> +               "Remove explicit permission of privileged app");
> +    testRemove(explicitPerm, certAppManifest, originOther,
> +               "Remove explicit permission of certified app");

If you need a test for failure of same origin, add it here

Comment 30

[Antonio Manuel Amaya Calvo (:amac)]

Ups, sorry, I used the splinter review and thought it would add my comments on the context of Jonas' ones.

I added them inline here to clarify what I wanted to say.

(In reply to Antonio Manuel Amaya Calvo from comment #29)
> Comment on attachment 706199 [details] [diff] [review]
> nsIDOMPermissionSettings.remove() v2, with tests
> 
> Review of attachment 706199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, addressing the comments.
> 
> ::: dom/permission/PermissionSettings.js
> @@ +111,5 @@
> > +    let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> > +    let principal = secMan.getAppCodebasePrincipal(uri, appID, true);
> > +
> > +    if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED ||
> > +        !this.isExplicit(aPermName, aManifestURL, aOrigin, true)) {
> 

(Jonas comment)
You shouldn't be allowed to remove explicit permissions. This check should just be

if (principal.appStatus !== Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) {
  ...

(my comment)
I don't think that allows to remove explicit permissions... it just fails
when the permission is implicit even if the app is not installed. Then
again, I don't think not installed apps get granted any implicit permissions
so I agree the condition should be changed.



> 
> ::: dom/permission/PermissionSettings.jsm
> @@ +65,5 @@
> > +    let deleteAllowed = true;
> > +    if (aAction === "unknown")
> > +      deleteAllowed = (aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED);
> > +
> > +    return deleteAllowed &&

(Jonas comment)
Hmm.. though shouldn't the logic really be:

return (aAction === "unknown" &&
        aPrincipal.appStatus === Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED) ||
       (aAction !== "unknown" &&
        (perm !== Ci.nsIPermissionManager.UNKNOWN_ACTION) &&
        isExplicit);

(my comment)
Yeah, the logic should most definitely be the last one. Allow the operation
if it's an allowed delete OR if it was allowed with the previous version
logic.

Comment 31

[Mounir Lamouri (:mounir)]

Comment on attachment 706199 [details] [diff] [review]
nsIDOMPermissionSettings.remove() v2, with tests

Review of attachment 706199 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/permission/nsIDOMPermissionSettings.idl
@@ +14,5 @@
>    void set(in DOMString permission, in DOMString value, in DOMString manifestURI, in DOMString origin, in bool browserFlag);
>  
>    bool isExplicit(in DOMString permission, in DOMString manifestURI, in DOMString origin, in bool browserFlag);
> +
> +  void remove(in DOMString permission, in DOMString manifestURI, in DOMString origin);

Why remove() doesn't take a |browserFlag| while all other methods do? It's very unclear. Even more because the implementation uses browserFlag=true which is odd at a first glance.

Please, document that.

::: dom/permission/PermissionSettings.js
@@ +121,5 @@
> +    }
> +
> +    // PermissionSettings.jsm handles delete when value is "unknown"
> +    cpm.sendSyncMessage("PermissionSettings:AddPermission", {
> +      type: aPermName,

Given that there is a removePermissions method in the jsm, can't you send a RemovePermission message and handle it to call RemovePermission?

Comment 32

[Reuben Morais [:reuben]]

Attachment: nsIDOMPermissionSettings.remove() v3, comments addressed

Thanks for the reviews everyone!

> Yeah, the logic should most definitely be the last one. Allow the operation
> if it's an allowed delete OR if it was allowed with the previous version
> logic.

FWIW, that's what my code is doing, except the appStatus!=NOT_INSTALLED check was extracted into a variable. I changed it to use the suggested style.

(In reply to Mounir Lamouri (:mounir) from comment #31)
> Comment on attachment 706199 [details] [diff] [review]
> nsIDOMPermissionSettings.remove() v2, with tests
> 
> Review of attachment 706199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/permission/nsIDOMPermissionSettings.idl
> @@ +14,5 @@
> >    void set(in DOMString permission, in DOMString value, in DOMString manifestURI, in DOMString origin, in bool browserFlag);
> >  
> >    bool isExplicit(in DOMString permission, in DOMString manifestURI, in DOMString origin, in bool browserFlag);
> > +
> > +  void remove(in DOMString permission, in DOMString manifestURI, in DOMString origin);
> 
> Why remove() doesn't take a |browserFlag| while all other methods do? It's
> very unclear. Even more because the implementation uses browserFlag=true
> which is odd at a first glance.
> 
> Please, document that.

See comment 18. I added a comment explaining.

> ::: dom/permission/PermissionSettings.js
> @@ +121,5 @@
> > +    }
> > +
> > +    // PermissionSettings.jsm handles delete when value is "unknown"
> > +    cpm.sendSyncMessage("PermissionSettings:AddPermission", {
> > +      type: aPermName,
> 
> Given that there is a removePermissions method in the jsm, can't you send a
> RemovePermission message and handle it to call RemovePermission?

removePermission is meant to be called from parent (e.g. PermissionsInstaller). It passes allowAllChanges=true to _internalAddPermission, which would bypass the check in isChangeAllowed, so that's why I don't use it.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/add0b94c2c0b

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/add0b94c2c0b