821961 - [Settings] App permissions UI should display only explicit permissions

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Here's my attempt to collapse bug 817087, bug 817034, and bug 817031 into one bug to improve human sanity. What we actually need to do here is just do the following:

If it's a hosted app, then the only permission that could be listed is the geolocation permission if it's called out. Otherwise, nothing can be listed, given that geolocation is the only permission that is PROMPT_ACTION for a hosted app.

If it's a privileged app, then only the following permissions can be listed if they are called out in alphabetical order:

- contacts
- device-storage:music
- device-storage:pictures
- device-storage:sdcard
- device-storage:videos
- geolocation

Otherwise, nothing else is listed, given that the above permissions are the only permissions that are PROMPT_ACTION for a privileged app.

Note - we just landed support for testing privileged apps, so you should be able to test the privileged piece now as well.

Comment 4

[Jason Smith [:jsmith]]

Same blocking+ reason as the other 3 bugs - I'm just collapsing this to make it thirty times easier to understand.

Comment 5

[David Scravaglieri [:scravag]]

Blocking as it is a split of a blocker.

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Actually the description is incorrect. The settings app should only list and allow modification of explicit permissions for all kind of apps (including certified which currently don't have any explicit permission but that could change). The type of app should not be taken into account directly, only if the permission is explicit for that app. 

To that extent, bug 812289 includes a new method on mozSettings, isExplicit, that returns true on the permissions that should be shown and allowed to be modifiable for a given app. 

In fact I think one of the now closed as dupplicates of this bugs includes a patch by Fco. Jordano on that direction.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #6)
> Actually the description is incorrect. The settings app should only list and
> allow modification of explicit permissions for all kind of apps (including
> certified which currently don't have any explicit permission but that could
> change). The type of app should not be taken into account directly, only if
> the permission is explicit for that app. 

No. Product and UX have already determined that certified apps should not be listed in the apps permissions UI a few times.

The list is correct from the finalized permissions matrix Paul built for hosted and privileged apps.

> 
> To that extent, bug 812289 includes a new method on mozSettings, isExplicit,
> that returns true on the permissions that should be shown and allowed to be
> modifiable for a given app. 

That should streamline the implementation of this feature then pretty easily.

> 
> In fact I think one of the now closed as dupplicates of this bugs includes a
> patch by Fco. Jordano on that direction.

Probably does. I migrated over to this bug mainly to eliminate confusion. I'll port over the github changes to this patch.

Comment 8

[Jason Smith [:jsmith]]

Attachment: Pointer to GitHub PR 6950

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Hi,

yes, the attached patch already fix this. Stealing from :etienne to finish the work ;)

Comment 10

[Daniel Coloma:dcoloma]

Francisco, it seems the platform is ready, can we merge it?

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

Daniel, will check if the current code is still working with the final patch.

Thanks.

Comment 12

[Etienne Segonzac (:etienne)]

Apparently the latest spec is:

1. Only show the applications that have a modifiable (explicit) permission. 
2. For those apps, only show the actual explicit permissions (not all the possible ones).
3. For derived permissions (the one that have a -read, -write,..) show the permission only once and modify all the values at once

*But* the app permissions list screen in the settings app is already suffering form bad performances and implementing 1. makes it border line unusable (since we need to query the permissionsSettings api 2 times for each permission of each app in order to render the first app list screen).

Here is a proposed revised spec:
1. List all the apps except a static list of HIDDEN_APPS (this is what the homescreen does) [1]
2. For those apps, show all the permissions from the app manifest plus any granted permission that wasn't in the manifest (this is the geolocation use case) [2].
3. For those permissions, disable the select if the permission is implicit (the permission won't be editable)
3bis. Alternatively we can completely hide implicit permissions (no technical issue, just a UX decision)
4. For derived permissions (the one that have a -read, -write,..) show the permission only once and modify all the values at once if the permission is explicit
5. If the app is not removable, disable the un-install button


If we want this for the 21, I need the go on the revised spec in the next 15 hours.
Josh, I think it's under your roof but feel free to redirect the needinfo.

Thanks!

[1] This static list lives in gaia
[2] The complete list of permissions comes from a copy of gecko's permissionTable.jsm thanks to Antonio's script

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Etienne Segonzac (:etienne) from comment #12)
> 
> Here is a proposed revised spec:
> 1. List all the apps except a static list of HIDDEN_APPS (this is what the
> homescreen does) [1]

If appStatus is visible from GAIA, you can exclude all the certified apps (appStatus==3) since they currently don't have any explicit permissions.

> [2] The complete list of permissions comes from a copy of gecko's
> permissionTable.jsm thanks to Antonio's script

For those not in the know this is a build script that gets the latest PermissionTable.jsm code and generates a resource file with the permissions name at build time so it doesn't have to be hardcoded in Gaia. If/when PermissionTable.jsm gets into xulrunner it can directly use it from there.

Comment 14

[Etienne Segonzac (:etienne)]

(In reply to Antonio Manuel Amaya Calvo from comment #13)
> (In reply to Etienne Segonzac (:etienne) from comment #12)
> > 
> > Here is a proposed revised spec:
> > 1. List all the apps except a static list of HIDDEN_APPS (this is what the
> > homescreen does) [1]
> 
> If appStatus is visible from GAIA, you can exclude all the certified apps
> (appStatus==3) since they currently don't have any explicit permissions.

We don't have access to the app status.

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Etienne Segonzac (:etienne) from comment #14)
> (In reply to Antonio Manuel Amaya Calvo from comment #13)
> > (In reply to Etienne Segonzac (:etienne) from comment #12)
> > > 
> > > Here is a proposed revised spec:
> > > 1. List all the apps except a static list of HIDDEN_APPS (this is what the
> > > homescreen does) [1]
> > 
> > If appStatus is visible from GAIA, you can exclude all the certified apps
> > (appStatus==3) since they currently don't have any explicit permissions.
> 
> We don't have access to the app status.

You can use app.manifest.type (and just present the ones that are !== "certified" )

Comment 16

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #15)
> (In reply to Etienne Segonzac (:etienne) from comment #14)
> > (In reply to Antonio Manuel Amaya Calvo from comment #13)
> > > (In reply to Etienne Segonzac (:etienne) from comment #12)
> > > > 
> > > > Here is a proposed revised spec:
> > > > 1. List all the apps except a static list of HIDDEN_APPS (this is what the
> > > > homescreen does) [1]
> > > 
> > > If appStatus is visible from GAIA, you can exclude all the certified apps
> > > (appStatus==3) since they currently don't have any explicit permissions.
> > 
> > We don't have access to the app status.
> 
> You can use app.manifest.type (and just present the ones that are !==
> "certified" )

Won't that not work in the case where the app manifest type is privileged but the appStatus is not?

That possibly could happen if I say, installed a packaged app with type "privileged" that was not signed. The app status would be web, but the app manifest type would be privileged.

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Antonio Manuel Amaya Calvo from comment #15)
> > (In reply to Etienne Segonzac (:etienne) from comment #14)
> > > (In reply to Antonio Manuel Amaya Calvo from comment #13)
> > > > (In reply to Etienne Segonzac (:etienne) from comment #12)
> > > > > 
> > > > > Here is a proposed revised spec:
> > > > > 1. List all the apps except a static list of HIDDEN_APPS (this is what the
> > > > > homescreen does) [1]
> > > > 
> > > > If appStatus is visible from GAIA, you can exclude all the certified apps
> > > > (appStatus==3) since they currently don't have any explicit permissions.
> > > 
> > > We don't have access to the app status.
> > 
> > You can use app.manifest.type (and just present the ones that are !==
> > "certified" )
> 
> Won't that not work in the case where the app manifest type is privileged
> but the appStatus is not?
> 
> That possibly could happen if I say, installed a packaged app with type
> "privileged" that was not signed. The app status would be web, but the app
> manifest type would be privileged.

What I would happen if I tried to install an app that says it's 'privileged' but isn't signed is to the installation to fail and the app to not be installed. Doesn't it do so?

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

To answer myself, I've been checking the app install code, and apparently you cannot install (correctly so) an app that's not signed that request a privileged status:

if (AppsUtils.getAppManifestStatus(aManifest) > maxStatus) {
  throw "INVALID_SECURITY_LEVEL";
}

So if the app's installed, then the app.manifest.type should correspond with the actual principal.appStatus value.

Comment 19

[Jason Smith [:jsmith]]

Got it. Thanks for checking.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

We need to move forward with this bug so I would say:
 - Show the full list of applications (except the hiddens one)
 - Do not show some permissions for certified apps (we miss l10n keys for them and they can be shown in a later release if needed)
 - Use a static version of Permissions list.

I would stamp a patch with such impl.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Vivien Nicolas (:vingtetun) from comment #20)
> We need to move forward with this bug so I would say:
>  - Show the full list of applications (except the hiddens one)

We can filter on app.manifest.type... and it should make the app considerably faster at least for now (since most apps at the start are certified anyway).

>  - Do not show some permissions for certified apps (we miss l10n keys for
> them and they can be shown in a later release if needed)
>  - Use a static version of Permissions list.

The code to generate the permission list is done already. If you prefer to have it static, I guess that just running it once and converting the output to an static object is easy enough :)


> 
> I would stamp a patch with such impl.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Antonio Manuel Amaya Calvo from comment #21)
> (In reply to Vivien Nicolas (:vingtetun) from comment #20)
> > We need to move forward with this bug so I would say:
> >  - Show the full list of applications (except the hiddens one)
> 
> We can filter on app.manifest.type... and it should make the app
> considerably faster at least for now (since most apps at the start are
> certified anyway).
>

Etienne told me that there is cases where the app is listed and there is nothing in the sub-menu. So I'm fine to hide them in this case since you can't even uninstall such apps.

> >  - Do not show some permissions for certified apps (we miss l10n keys for
> > them and they can be shown in a later release if needed)
> >  - Use a static version of Permissions list.
> 
> The code to generate the permission list is done already. If you prefer to
> have it static, I guess that just running it once and converting the output
> to an static object is easy enough :)
> 
> 
> > 
> > I would stamp a patch with such impl.

Comment 23

[Josh Carpenter [:jcarpenter]]

Sorry for the slight response delay, guys. I was travelling Wednesday and Thursday, and today is early family Christmas :p

Regarding the proposals above:

> Etienne told me that there is cases where the app is listed and there is nothing in the sub-menu. 
> So I'm fine to hide them in this case since you can't even uninstall such apps.

(Refers to showing certified apps). Agreed. I am fine not displaying them.

> 3. For those permissions, disable the select if the permission is implicit (the permission 
> won't be editable) 
> 3bis. Alternatively we can completely hide implicit permissions (no technical issue, just a
> UX decision)

I prefer 3B. That was always the plan. See page 9 of the specs, here: 
https://www.dropbox.com/sh/frgu76pka2h3qlj/7pwHYd8vhd

> 5. If the app is not removable, disable the un-install button

Agree.

Nice work guys, you made it easy to chime in here. Hope that this feedback has gotten to you in time to be helpful!

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Josh Carpenter [:jcarpenter] from comment #23)
> Nice work guys, you made it easy to chime in here. Hope that this feedback
> has gotten to you in time to be helpful!

It is :)

Comment 25

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

New patch proposal, to spec \o/

This brings some late-l10n, just waiting for the code to be reviewed before asking :stas for review in order to ask on the final patch.

Comment 26

[Fabien Cazenave [:kaze]]

Comment on attachment 695201 [details] [diff] [review]
Patch proposal

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

r=me but please rename permissionsTable.json so it matches our guidelines.

::: apps/settings/resources/permissionsTable.json
@@ +1,3 @@
> +{
> +   "composedPermissions" : [
> +      "contacts",

please name it “permission_table.json” or “permissions.json” instead and use a two-space indentation

::: build/applications-data.js
@@ +127,1 @@
>  init = getFile(GAIA_DIR, GAIA_CORE_APP_SRCDIR, 'homescreen', 'js', 'hiddenapps.js');

<3

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 695201 [details] [diff] [review]
Patch proposal

A small warning first, I'm on vacation this week (not a big problem by itself) and away from my build machine (that OTOH is :) ) so I've reviewed this on my phone. Sorry if I missed something. 

That said, I have a question. If I'm reading this correctly it seems that we allow modification of all the granted permissions, regardless if they're implicit or explicit (if it's set to allow then the select is shown). This is incorrect on two things: I would swear UX wanted us to only show the permissions that can be actually changed, and if the permission isn't explicit it cannot be changed (the platform won't allow it anymore). 

Oh, on the same line, mozPermissionSettings.set throws if the change isn't allowed.

Comment 28

[Etienne Segonzac (:etienne)]

(In reply to Antonio Manuel Amaya Calvo from comment #27)
> Comment on attachment 695201 [details] [diff] [review]
> Patch proposal
> 
> A small warning first, I'm on vacation this week (not a big problem by
> itself) and away from my build machine (that OTOH is :) ) so I've reviewed
> this on my phone. Sorry if I missed something. 

How brave :)

> 
> That said, I have a question. If I'm reading this correctly it seems that we
> allow modification of all the granted permissions, regardless if they're
> implicit or explicit 

Must be hard to read on a phone :)
If isExplicit if false we bail out right away.

> Oh, on the same line, mozPermissionSettings.set throws if the change isn't
> allowed.

Added a try catch with logs.

Comment 29

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Comments addressed, should be ready for l10n-review.

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #29)
> Created attachment 695229 [details] [diff] [review]
> Patch v2
> 
> Comments addressed, should be ready for l10n-review.

Kaze is the guy that as spent more time than anybody else on this project on l10n. I tend to think his review is enough for l10n issues.

Comment 31

[Etienne Segonzac (:etienne)]

(In reply to Vivien Nicolas (:vingtetun) from comment #30)
> (In reply to Etienne Segonzac (:etienne) from comment #29)
> > Created attachment 695229 [details] [diff] [review]
> > Patch v2
> > 
> > Comments addressed, should be ready for l10n-review.
> 
> Kaze is the guy that as spent more time than anybody else on this project on
> l10n. I tend to think his review is enough for l10n issues.

I'll take that for approval :)

Comment 32

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/59c97fbfa702814bdc84d2c691dd0d92472c0d48

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Etienne Segonzac (:etienne) from comment #28)
> (In reply to Antonio Manuel Amaya Calvo from comment #27)
> > Comment on attachment 695201 [details] [diff] [review]
> > Patch proposal
> > 
> > That said, I have a question. If I'm reading this correctly it seems that we
> > allow modification of all the granted permissions, regardless if they're
> > implicit or explicit 
> 
> Must be hard to read on a phone :)
> If isExplicit if false we bail out right away.
> 

He, indeed, I missed half of the condition. On that line, though, you can just do

if (value === 'unknown')
  return false;

// call and return whatever isExplicit returns otherwise

That'll save you a call to isExplicit()  and a search on the permissions array.  If a permission is on the manifest then it's value will be allow, deny, or prompt but never unknown.

Comment 35

[Axel Hecht [:Pike]]

Comment on attachment 695229 [details] [diff] [review]
Patch v2

post-mortem r=me, but it'd be nice to actually cancel review requests if you think you don't need them anymore.

Comment 36

[Jason Smith [:jsmith]]

Verified by testing this with a hosted app that called out support both explicit and non-explicit perms - only the explicit perms showed up. Additionally did a similar test with a privileged app. Looked fine.