PermissionSettings interface needs 'remove' API method

RESOLVED FIXED in mozilla21

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: ddahl, Assigned: reuben)

Tracking

Trunk
mozilla21
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Note: this is a follow-up for bug 770731
Blocks: 758269
Assignee: nobody → anygregor
blocking-basecamp: --- → ?
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.
blocking-basecamp: ? → -
(Assignee)

Comment 3

5 years ago
Is this bug still valid? The code suggests that setting to "unknown" no longer removes the permission.
(Reporter)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(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".
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
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
(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.
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.
(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.
Blocks: 832371
(Assignee)

Comment 11

5 years ago
Created attachment 703966 [details] [diff] [review]
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?
Assignee: anygregor → reuben.bmo
Attachment #703966 - Flags: feedback?(jonas)
Attachment #703966 - Flags: feedback?(anygregor)
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.
Attachment #703966 - Flags: feedback?(jonas) → review+
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
Attachment #703966 - Flags: feedback?(anygregor) → feedback+
(Assignee)

Comment 14

5 years ago
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.

Carrying r=sicking.
Attachment #703966 - Attachment is obsolete: true
Attachment #705183 - Flags: review+
(Assignee)

Comment 15

5 years ago
Created attachment 705207 [details] [diff] [review]
nsIDOMPermissionSettings.remove(), without debugging messages
Attachment #705183 - Attachment is obsolete: true
Attachment #705207 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2027e3e07448
Keywords: checkin-needed
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).
(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?
https://hg.mozilla.org/mozilla-central/rev/2027e3e07448
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Backout again: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef03bfa48a45
https://hg.mozilla.org/mozilla-central/rev/99dabf8d5e98
(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
(Assignee)

Comment 25

5 years ago
Created attachment 706100 [details] [diff] [review]
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.
Attachment #705207 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 795334
(Assignee)

Comment 26

5 years ago
Created attachment 706199 [details] [diff] [review]
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.
Attachment #706100 - Attachment is obsolete: true
Attachment #706199 - Flags: review?(jonas)
Attachment #706199 - Flags: review?(amac)
(Assignee)

Updated

5 years ago
No longer depends on: 795334
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);
Attachment #706199 - Flags: review?(jonas) → review+
(Assignee)

Comment 28

5 years ago
(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 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
Attachment #706199 - Flags: review?(amac) → review+
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 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?
(Assignee)

Comment 32

5 years ago
Created attachment 706678 [details] [diff] [review]
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.
Attachment #706199 - Attachment is obsolete: true
Attachment #706678 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/add0b94c2c0b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/add0b94c2c0b
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.