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

RESOLVED DUPLICATE of bug 821961

Status

P1
normal
RESOLVED DUPLICATE of bug 821961
6 years ago
6 years ago

People

(Reporter: jsmith, Assigned: etienne)

Tracking

unspecified
B2G C3 (12dec-1jan)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
blocking-basecamp: --- → ?
(Reporter)

Comment 1

6 years ago
As I move throw building out these bugs, if either of you think I screwed up, let me know.
(Reporter)

Updated

6 years ago
Blocks: 815565
Summary: Do not list any app permissions for installed apps that do not make sense for the app type → Do not list any app permissions for installed apps that have not been requested by that app
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
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.
(Reporter)

Comment 4

6 years ago
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.
Summary: Do not list any app permissions for installed apps that have not been requested by that app → Do not list any requested app permissions for installed apps that are always denied access (DENY_ACTION)
(Reporter)

Comment 5

6 years ago
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.
Assignee: nobody → etienne
blocking-basecamp: ? → +
Priority: -- → P1
(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 :)
(Reporter)

Comment 7

6 years ago
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.
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.
Depends on: 812289
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.
Target Milestone: --- → B2G C3 (12dec-1jan)
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.
Also "prompt/explicit" and "allow/explicit" should be shown, right?
(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.
Whiteboard: [target:12/21]
(Reporter)

Comment 13

6 years ago
Duping to a more clean definition per Jonas's feedback about the confusion about 3 bugs open for one bug's purpose.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 821961
Whiteboard: [target:12/21]
(Reporter)

Comment 14

6 years ago
Created attachment 692626 [details]
Pointer to GitHub PR 6950
(Reporter)

Updated

6 years ago
Attachment #692626 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
No longer blocks: 815565
(Reporter)

Updated

6 years ago
No longer depends on: 812289
You need to log in before you can comment on or make changes to this bug.