817031 - Do not list any requested app permissions for installed apps that are always denied access (DENY_ACTION)

Status: RESOLVED DUPLICATE of bug 821961

(Firefox OS Graveyard :: Gaia::Settings, defect, P1)

Description

[Jason Smith [:jsmith]]

Breakout based on Jonas's input in https://bugzilla.mozilla.org/show_bug.cgi?id=811281#c25. We need to fix the settings apps permissions UI to do the following in relation to permissions:

If the app permission is always DENY for the app permission for the app type (e.g. hosted app calls out support for the contacts permission, which would always be DENY), then do not list the app permission in the app permission UI. The permission should always be DENY as well.

Comment 1

[Jason Smith [:jsmith]]

As I move throw building out these bugs, if either of you think I screwed up, let me know.

Comment 2

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

Fixing up the summary. We should only list permissions for an app that the app actually has requested. I.e. if an app is installed which doesn't, and never has, enumerated "contacts" in its manifest, we shouldn't show UI for controlling contacts API for that app.

The way that I think we should determine which APIs to list for an app is to check which permissions are stored in the permissions database. I.e. we should keep a list of all permissions that can be granted to "normal" and "privileged" apps and for each such permission check if there's an entry in the permissions database for the app.

If there is no entry in the database, don't show any UI for that permission. Otherwise show either a readonly UI (if the permission is implicit) or UI which allows the user to choose between prompt/accept/deny (if the permission is explicit)

See also bug 817034

Comment 3

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

The set of permissions are:

Explicit:
geolocation
contacts (can be -read, -write and/or -create)
device-storage:pictures (can be -read, -write and/or -create)
device-storage:videos (can be -read, -write and/or -create)
device-storage:music (can be -read, -write and/or -create)
device-storage:sdcard (can be -read, -write and/or -create)
wifi-manage (I'm not actually sure this one is hooked up correctly everywhere.
             It might end up being a certified API)

Implicit:
alarms
tcp-socket
browser
fmradio
desktop-notification
systemXHR


Some of these names might change a bit. But if they do we'll deal with that then.

Comment 4

[Jason Smith [:jsmith]]

Oh. What I was originally going for in this bug actually is a bit different, although I'm wondering if we're saying the same thing in different wording.

The current behavior lists any permission called out in the manifest against the hardcoded list of permissions held in the permissions settings code (there is a problem about the list not being exactly right, but that's bug 817087. There's additionally bug 817034 about the implicitly granted permissions). 

The problem I'm talking about in this bug is that this situation can happen right now:

1. I install a hosted web app that calls out a certified app permission
** Example test page: http://people.mozilla.com/~fdesre/openwebapps/test.html
** Example manifest: http://people.mozilla.com/~fdesre/openwebapps/permissions.manifest
2. Installation ends up being successful
3. Go to settings --> app permissions --> app name

Result - You'll end up seeing the certified app permission listed with a select box defaulted to DENY that can be modified to ASK and ALLOW. This doesn't make sense for two reasons:

1. If a permission is always DENY_ACTION, then I don't see why we would even show it in the UI
2. Any permission with DENY_ACTION should not be able to have it's permission level to be modified to ASK or ALLOW

So the problem I'm talking about in this bug is that DENY_ACTION permissions are getting listed and can be modified.

Does that clarify what I'm getting at? I think we're saying the same thing, but I'm not sure.

Comment 5

[Jason Smith [:jsmith]]

To back up - if we combine all three bugs together (bug 817087, this one, and bug 817034), here's what I mean at a high-level:

For every app permission listed in the app manifest that has app type X:

  [1] If permission for this app type == DENY_ACTION:
       Don't list the permission in the UI and don't let it be modifiable

  [2] If permission for this app type == PROMPT_ACTION:
       List the permission in the UI and let it be modifiable to ASK, ALLOW, DENY

  [3] If permission for this app type == ALLOW_ACTION and is not a certified app:
       List the permission in the UI as read only

This bug targets [1]. bug 817034 targets [3]. Getting the permission list right in that for loop is bug 817087.

Comment 6

[Josh Carpenter [:jcarpenter]]

(In reply to Jason Smith [:jsmith] from comment #5)
> To back up - if we combine all three bugs together (bug 817087, this one,
> and bug 817034), here's what I mean at a high-level:
> 
> For every app permission listed in the app manifest that has app type X:
> 
>   [1] If permission for this app type == DENY_ACTION:
>        Don't list the permission in the UI and don't let it be modifiable

Yep, UX agrees :)

Comment 7

[Jason Smith [:jsmith]]

Per Josh's addition in bug 817034 - the conclusion is that the only permissions that should be listed on this UI therefore is any app permissions for that app type are PROMPT_ACTION. Anything else should not be listed.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

As part of bug 812289 I'm going to expose if a permission can be modified by an user or not. Marking this as dependant because of that.

Comment 9

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

If/when bug 812289 patch lands, PermissionSettings.get will return the permission and the type (as in "allow/explicit", "allow/implicit", "deny/implicit", "deny/explicit") and so on. As per this bug, this means that only permissions that are marked as "deny/explicit" should be modifiable (and shown). If a permission is "deny/implicit" then it should not be shown.

Comment 11

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

Also "prompt/explicit" and "allow/explicit" should be shown, right?

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #11)
> Also "prompt/explicit" and "allow/explicit" should be shown, right?

Sorry for the delay, yeah, but this bug is just for the deny cases :) As I said on bug 817034 I think those two bugs should be consolidated on one.

Anyway, the current patch-in-process has changed what I said. Now PermissionsSettings.get returns only "allow", "deny", "prompt", and "unknown" and there's  (or there will be) a new method, PermissionsSettings.isExplicit() that returns true if the permission is explicit, false otherwise. So only the permissions that have PermissionsSettings.isExplicit returning true should be shown.

Comment 13

[Jason Smith [:jsmith]]

Duping to a more clean definition per Jonas's feedback about the confusion about 3 bugs open for one bug's purpose.

Comment 14

[Jason Smith [:jsmith]]

Attachment: Pointer to GitHub PR 6950