804623 - Permission Prompt Helper needs more access context to enable/disable permissions

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[David Dahl :ddahl]

This is a followup bug. See bug 773114 comment 38

Jonas: It's unfortunate that we are preventing "read" operations even if just "write" has been set to DENY_ACTION. This is fine for now since we don't get enough context in this function to know whether a "read" or "write" operation is about to be performed.

on the code snippet: 

    for (let idx in installedPerms) {
      // if any of the installedPerms are deny, run aCallbacks.cancel
      if (installedPerms[idx] == Ci.nsIPermissionManager.DENY_ACTION ||
          installedPerms[idx] == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
        aCallbacks.cancel();
        return;
      }
    }

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

This currently makes useless requesting only read permission on explicit permissions, since they will be rejected always. So in a way it just make differentiating access mode useless. It doesn't matter than my app only needs to read contact, and that I can request just read access, if by just requesting read access I get always denied, while requesting read-write I'll not get denied.

So we're basically training developers to request more permissions than necessary and throwing away the principle of least privilege!

This is something that must be fixed ASAP. The readonly permission request is useless right now.

Comment 2

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

That's not correct. If you just request "read" for an explicit permission then the "read" permission will be set to PROMPT and the "write" permission won't be written to the database at all.

So when the prompt happens we'll simply display the prompt.

The bug here only happens if the permission database is modified explicitly using the permission API. But I don't think there is any way with the current UIs anywhere in the product to actually trigger this bug.

The API suggested in bug 828278 comment 16 would enable fixing this bug.

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Jonas, that's not what we've seen, according to what Albert told me. Needinfo-ing him for the steps to reproduce.

Comment 4

[Albert [:albert]]

The problem I have seen can be reproduced with mozilla-central gecko build.

Steps to reproduce:
1: Open sms app.
2: Click on send new message icon.
3: Type destination: a
4: Type message: a

Expected:
The header of the screen shows 'a' and sending the message fails.

Actual result:
The header of the screen shows 'Messages' and sending the message fails.

logcat output:
E/GeckoConsole(  649): Content JS LOG at app://sms.gaiamobile.org/js/contacts.js:30 in onerror: Contact finding error. Error: undefined

The sms app's manifest define readonly permission for contacts, when the app tries to access contacts permission is denied by aCallbacks.cancel(); of the comment 1.

If the manifest is changed to request readwrite permission for contacts, the app works as expected.

Comment 5

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

I don't this is due to the problem that this bug is filed on and that is described in comment 0. I suspect there is something else going on since all entries that are in installedPerms should have ALLOW here.

Please file a separate bug and cc me and Gregor.

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #5)
> I don't this is due to the problem that this bug is filed on and that is
> described in comment 0. I suspect there is something else going on since all
> entries that are in installedPerms should have ALLOW here.
> 
> Please file a separate bug and cc me and Gregor.

Er... Unless I'm very much mistaken, it *is* this bug:

// Expand permission names.
let expandedPermNames = expandPermissions(msg.type, access);

---- access defaults to readwrite

for (let idx in expandedPermNames) {
   let access = msg.access ? expandedPermNames[idx] : msg.type;
   let permValue = 
     permissionManager.testExactPermissionFromPrincipal(principal, access);
   installedPermValues.push(permValue);
}
---- This is checking for all the possible access values, which again default to readwrite

And finally, as stated in c0: 

if (installedPerms[idx] == Ci.nsIPermissionManager.DENY_ACTION ||
     installedPerms[idx] == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
    aCallbacks.cancel();
    return;
}


If the permission isn't on the database then installedPerms[idx] will be UNKNOWN_ACTION (mostly because it can only be ALLOW, DENY, PROMPT, or UNKNOWN). And thus it'll enter on the if and cancel.

In any case, I don't get why the || with the unknown action is necessary. Even if with just one deny we want to cancel, UNKNOWN just means the permission wasn't requested, not that it was denied.

Comment 7

[Jon Hylands [:jhylands]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> I don't this is due to the problem that this bug is filed on and that is
> described in comment 0. I suspect there is something else going on since all
> entries that are in installedPerms should have ALLOW here.
> 
> Please file a separate bug and cc me and Gregor.

Jonas,

I've done some testing and logging, and I can verify that, in the case discussed here (clicking on a SMS message to open the thread, with the permissions set to readonly), installedPermValues is ALLOW at index 0, and UNKNOWN at index 1.

Does it make sense to fix this with UNKNOWN being treated as PROMPT, or something else?

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> 
> Does it make sense to fix this with UNKNOWN being treated as PROMPT, or
> something else?

I dont think we can treat UNKNOWN as PROMPT, other non-privileged web apps can then prompt for any permission i think ? (e.g. bug 814294)

Comment 9

[Jon Hylands [:jhylands]]

Okay, I've thought about this a little more.

If we take out the || clause with the UNKNOWN check at the beginning (in the DENY check), and add a fallthrough at the end of the function to essentially do a DENY if all the perms are UNKNOWN, then it should work properly, I would think.

Comment 10

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

What *should* happen here is the following:

At install time, if a a privileged app requests "readonly" access to contacts, then we add a single entry to the nsIPermissionsManager for "contacts-read" with the value PROMPT. No other entries should be added to the nsIPermissionManager related to contacts. (I think this is already happening correctly).

When the app goes to read some information from contacts, we end up in the
permission-prompt code and tell it to bring up a prompt for "contacts".

At this point, what we *should* do, is to look at "contacts-read", "contacts-write" *or* "contacts-create" depending on if it's a read, write or create action. If it's a read action and "contacts-read" has the value ALLOW we simply return ALLOW. If it has the value DENY or UNKNOWN we return DENY. If it has the value PROMPT we keep going.

There are simpler things we could do, but we might as well fix things so that the permission prompt code is told if it's a read, write or create action.

So if we get here it means that we are trying to perform an action which contains the value PROMPT in the nsIPermissionManager.

So we put up a prompt, and when the user makes a choice we only write "contacts-read", "contacts-write" and "contacts-create" those permissions that had the value PROMPT. I.e. if the database contained

"contacts-read" PROMPT
"contacts-write" UNKNOWN
"contacts-create" PROMPT

and the page tries to do a read action, then we display a prompt and if the user chooses ALLOW we write "contacts-read" and "contacts-create" and thus get

"contacts-read" ALLOW
"contacts-write" UNKNOWN
"contacts-create" ALLOW

Comment 11

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

(In reply to Jon Hylands [:jhylands] from comment #9)
> Okay, I've thought about this a little more.
> 
> If we take out the || clause with the UNKNOWN check at the beginning (in the
> DENY check), and add a fallthrough at the end of the function to essentially
> do a DENY if all the perms are UNKNOWN, then it should work properly, I
> would think.

I think this might implement the simpler solution I alluded to in the previous comment.

As long as you make sure to only write ALLOW/DENY to the values that were PROMPT once the user chooses something in the UI.

We definitely need to test that these things behave as they should though.


Also, FWIW, I think comment 9 describes how to fix the problem mentioned in comment 1, whereas comment 10 solves the problem this bug was originally filed on.

Comment 12

[Gregor Wagner [:gwagner]]

Jason, do you have an app where we can test this?
We should also add a test for this.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

We have been checking the code a little bit more, and it seems that the problem isn't as much that concrete check as the whole askPermission method. It seems that the access attribute from aMessage is not used, even when it's correctly passed (and that attribute provides the context this bug request). Before fixing bug 816900 the code worked because it concatenated several incorrect behaviors to provide a correct result in the end. Fixing that though made the other errors show up.

The code, if all the calling methods are passing a correct value for the access attribute should be much more simple:

* Check the permission for just msg.type + '-' + msg.access (or to make it more correct, expandPermission with msg.type and msg.access as parameters). 
* If the permission is allow return success, if it's deny or unknown return cancel, otherwise (it's prompt) ask for the permission.

There's no need to check for all the possible subpermissions that could be or could not be requested if the msg.access has the permission that IS being actually requested. We've checked the contacts API and at least that one is filling correctly the requested access when asking for the permission. If the rest aren't doing the same, the APIs should be fixed instead of trying to fix this function to work with APIs that don't fill that attribute correctly.

Oh, to make things better, the function as is implemented right now could end requesting a different permission than the one that should actually be requested, see the loop at [1].



[1] https://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#91

Comment 14

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 15

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 16

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

Comment on attachment 729587 [details] [diff] [review]
patch

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

We definitely need tests for this

Comment 17

[Gregor Wagner [:gwagner]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> Comment on attachment 729587 [details] [diff] [review]
> patch
> 
> Review of attachment 729587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We definitely need tests for this

I tried to extend our current apps tests but I couldn't install a certified app in a mochitest-browser-chrome. Fabrice do you know if we have a way to do this?
I need a certified app because the readonly access field in combination with allow by default is only used in certified apps.

Comment 18

[Gregor Wagner [:gwagner]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> Comment on attachment 729587 [details] [diff] [review]
> patch
> 
> Review of attachment 729587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We definitely need tests for this

Well bug 813797 made it impossible to test this :(

Comment 19

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2418deea4b19

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2418deea4b19

Comment 21

[Gregor Wagner [:gwagner]]

Marketplace apps might run into this.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #21)
> Marketplace apps might run into this.

In what sense? Can you clarify the use cases impacted here?

Comment 23

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

Gregor, Antonio, is this affecting the 1.0.1 branch as well?

As I understand it, this bug is causing us to not be able to grant read-only access to permissions. Is that correct? If so, and if it affects the 1.0.1 branch, we should nominate for tef+

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

As far as I know, this doesn't even affect 1.1. The code on both 1.0.1 and 1.1 is incorrect... but it's incorrect on a way that ends up working correctly. Bug 816900 fixed part of the code and thus it was more correct but ironically worked worse. And that bug hasn't landed on 1.0.1 or 1.1. If you want to land this one it's ok with me, though, since FWIW I think this code is better than we currently have on 1.0.1/1.1.

Comment 25

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #24)
> As far as I know, this doesn't even affect 1.1. The code on both 1.0.1 and
> 1.1 is incorrect... but it's incorrect on a way that ends up working
> correctly. Bug 816900 fixed part of the code and thus it was more correct
> but ironically worked worse. And that bug hasn't landed on 1.0.1 or 1.1. If
> you want to land this one it's ok with me, though, since FWIW I think this
> code is better than we currently have on 1.0.1/1.1.

Okay. So that might explain why I was confused that I thought read only privileges with the access property was working, but this bug in some comments above questions that. 

So what sounds like a correction workflow of some sorts? And I'm back to not fully understanding the user impact.

Comment 26

[Jason Smith [:jsmith]]

I'll talk with Gregor about this tomorrow.

Comment 27

[Gregor Wagner [:gwagner]]

As said in comment 1, the readonly permission is useless because "write" is set to deny and without this patch we deny a request even if a single access field is set to deny.

Comment 28

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #27)
> As said in comment 1, the readonly permission is useless because "write" is
> set to deny and without this patch we deny a request even if a single access
> field is set to deny.

Thanks for the clarity. Bumping to tef then.

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

To repeat myself, it doesn't fail as Gregor described on either 1.0.1 or 1.1. You can test that easily enough. If on the SMS app you can find a contact, then it's working. If you cannot, then you're affected by this bug. That's because although the permission code IS incorrect on 1.0.1 and 1.1 it's... Incorrect on a way that ends up nullifying the incorrectness. Then a patch that landed only on m-c broke the incorrectness balance and made it actually fail.

Comment 30

[Antonio Manuel Amaya Calvo (:amac)]

Oh, and my previous comment doesn't mean, at all, that I'm opposed to landing this on 1.0.1 or 1.1. It's just that, on all fairness, it cannot be called blocking :)

Comment 31

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #30)
> Oh, and my previous comment doesn't mean, at all, that I'm opposed to
> landing this on 1.0.1 or 1.1. It's just that, on all fairness, it cannot be
> called blocking :)

Okay. Now I finally understand what's going on here. And that explains why I never saw this bug while testing on b2g18. We changed something on m-c which revealed the bug, but it's not exposed on the other two branches. But we should uplift this to fix the underlying problem anyways.

Let's get approval here then.

Comment 32

[Dale Chadwick]

Attachment: Amazing Gif Art_v1.30_apkpure.com.apk

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.