Closed Bug 811281 Opened 12 years ago Closed 12 years ago

Implicit permissions should be listed for certfied apps but they aren't

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Settings
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INVALID
Milestone:
B2G C2 (20nov-10dec)

People

(Reporter: amac, Assigned: etienne)

Details

According to https://wiki.mozilla.org/Apps/Security#Implicitly_granted_permissions implicit permissions can be inspected but not modified. 
But the current settings application allows the modification of implicit permissions. 

I'm filing this on the settings app but it should probably have some kind of control on gecko also, since leaving something that could potentially brick (try removing all the permissions from the settings app) a phone on the UI doesn't seem like a good idea.
Noming for blocking because this must be fixed before shipping.
blocking-basecamp: --- → ?
In production builds you can only change permissions for 3rd party apps from the settings app. So it's not as bad as described. But that's true that nothing prevents that to happen on the platform side.
I just saw that, yeah. But letting aside that the enforcement should be at the platform and not at the UI, the UI behavior isn't correct on the production builds either (or the documentation is incorrect). 

According to the documentation, permissions for the certified apps should be shown, just not modifiable, and on production builds they're not shown at all.
(In reply to Antonio Manuel Amaya Calvo from comment #3)
> I just saw that, yeah. But letting aside that the enforcement should be at
> the platform and not at the UI, the UI behavior isn't correct on the
> production builds either (or the documentation is incorrect). 
> 
> According to the documentation, permissions for the certified apps should be
> shown, just not modifiable, and on production builds they're not shown at
> all.

That's a different bug. And the System app is the platform in some way and I don't see why it should not be allowed to change permissions. 

So I would said this bug is resolved wontfix but if you want to change the title to match the documentation that would be fine to me.

Adding Etienne that did the implementation.
Summary: Implicit permissions should not be modifiable but they are → Implicit permissions should not be listed for certfied apps but they aren't
Summary: Implicit permissions should not be listed for certfied apps but they aren't → Implicit permissions should be listed for certfied apps but they aren't
blocking-basecamp: ? → +
Priority: -- → P1
(In reply to Vivien Nicolas (:vingtetun) from comment #4)
> (In reply to Antonio Manuel Amaya Calvo from comment #3)
> > I just saw that, yeah. But letting aside that the enforcement should be at
> > the platform and not at the UI, the UI behavior isn't correct on the
> > production builds either (or the documentation is incorrect). 
> > 
> > According to the documentation, permissions for the certified apps should be
> > shown, just not modifiable, and on production builds they're not shown at
> > all.
> 
> That's a different bug. And the System app is the platform in some way and I
> don't see why it should not be allowed to change permissions. 

The thing is that permissions for certified apps should not be modified, according to the documentation (and FWIW I think the documentation is right). Since they should not be modifiable at all, it makes sense that the platform should forbid changes and not leave that to the front end implementation. 

I added Lucas and Paul to get their input on this, this is just my opinion. 

> 
> So I would said this bug is resolved wontfix but if you want to change the
> title to match the documentation that would be fine to me.
> 
> Adding Etienne that did the implementation.

I updated the bug title to match the actual problem.
So what needs to happen here is:

* Display all the apps in the settings app instead of only the third party ones
* For the new non-remouvable apps added to this list:
  - disable each permission <select>
  - hide the uninstall button

Am I correct?
(In reply to Etienne Segonzac (:etienne) from comment #6)
> So what needs to happen here is:
> 
> * Display all the apps in the settings app instead of only the third party
> ones
> * For the new non-remouvable apps added to this list:
>   - disable each permission <select>
>   - hide the uninstall button
> 
> Am I correct?

That's how I understand it.
(In reply to Antonio Manuel Amaya Calvo from comment #5)
> (In reply to Vivien Nicolas (:vingtetun) from comment #4)
> > (In reply to Antonio Manuel Amaya Calvo from comment #3)
> > > I just saw that, yeah. But letting aside that the enforcement should be at
> > > the platform and not at the UI, the UI behavior isn't correct on the
> > > production builds either (or the documentation is incorrect). 
> > > 
> > > According to the documentation, permissions for the certified apps should be
> > > shown, just not modifiable, and on production builds they're not shown at
> > > all.
> > 
> > That's a different bug. And the System app is the platform in some way and I
> > don't see why it should not be allowed to change permissions. 
> 
> The thing is that permissions for certified apps should not be modified,
> according to the documentation (and FWIW I think the documentation is
> right). 

This is a UI issue.
(In reply to Vivien Nicolas (:vingtetun) from comment #7)
> (In reply to Etienne Segonzac (:etienne) from comment #6)
> > So what needs to happen here is:
> > 
> > * Display all the apps in the settings app instead of only the third party
> > ones
> > * For the new non-remouvable apps added to this list:
> >   - disable each permission <select>
> >   - hide the uninstall button
> > 
> > Am I correct?
> 
> That's how I understand it.

Actually it should be for the *certified* apps, not for the non removable apps. We can have privileged or even web applications that are non removable (and in fact it can make sense because you can have an operator or builder provided app that cannot/should not be removed but that doesn't need any elevated privileges and thus it can be a normal app). For those apps it should be possible to change the permissions.
(In reply to Antonio Manuel Amaya Calvo from comment #9)
> (In reply to Vivien Nicolas (:vingtetun) from comment #7)
> > (In reply to Etienne Segonzac (:etienne) from comment #6)
> > > So what needs to happen here is:
> > > 
> > > * Display all the apps in the settings app instead of only the third party
> > > ones
> > > * For the new non-remouvable apps added to this list:
> > >   - disable each permission <select>
> > >   - hide the uninstall button
> > > 
> > > Am I correct?
> > 
> > That's how I understand it.
> 
> Actually it should be for the *certified* apps, not for the non removable
> apps. We can have privileged or even web applications that are non removable
> (and in fact it can make sense because you can have an operator or builder
> provided app that cannot/should not be removed but that doesn't need any
> elevated privileges and thus it can be a normal app). For those apps it
> should be possible to change the permissions.

I'm sorry, bar that, I was wrong. It isn't a type of application restriction, is a type of app/type of permission restriction. Implicit permission should not be modifiable for any kind of application according to [1].

[1] https://wiki.mozilla.org/Apps/Security#Implicitly_granted_permissions
(In reply to Vivien Nicolas (:vingtetun) from comment #8)
> (In reply to Antonio Manuel Amaya Calvo from comment #5)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #4)
> > > (In reply to Antonio Manuel Amaya Calvo from comment #3)
> > > > I just saw that, yeah. But letting aside that the enforcement should be at
> > > > the platform and not at the UI, the UI behavior isn't correct on the
> > > > production builds either (or the documentation is incorrect). 
> > > > 
> > > > According to the documentation, permissions for the certified apps should be
> > > > shown, just not modifiable, and on production builds they're not shown at
> > > > all.
> > > 
> > > That's a different bug. And the System app is the platform in some way and I
> > > don't see why it should not be allowed to change permissions. 
> > 
> > The thing is that permissions for certified apps should not be modified,
> > according to the documentation (and FWIW I think the documentation is
> > right). 
> 
> This is a UI issue.

This is an UI issue, right. For the rest I filed bug 812289.
(In reply to Antonio Manuel Amaya Calvo from comment #10)
> I'm sorry, bar that, I was wrong. It isn't a type of application
> restriction, is a type of app/type of permission restriction. Implicit
> permission should not be modifiable for any kind of application according to

So:
* List *all* apps
* Do not display uninstall button if !app.removable
* Disable the permission select if permission is implicit

Gregor, is there a safe way from Gaia to know if a permission is implicit or not?
Flags: needinfo?(anygregor)
(In reply to Antonio Manuel Amaya Calvo from comment #10)
> I'm sorry, bar that, I was wrong. It isn't a type of application
> restriction, is a type of app/type of permission restriction. Implicit
> permission should not be modifiable for any kind of application according to

So:
* List *all* apps
* Do not display uninstall button if !app.removable
* Disable the permission select if permission is implicit

Gregor, is there a safe way from Gaia to know if a permission is implicit or not?
(In reply to Etienne Segonzac (:etienne) from comment #13)
> (In reply to Antonio Manuel Amaya Calvo from comment #10)
> > I'm sorry, bar that, I was wrong. It isn't a type of application
> > restriction, is a type of app/type of permission restriction. Implicit
> > permission should not be modifiable for any kind of application according to
> 
> So:
> * List *all* apps
> * Do not display uninstall button if !app.removable
> * Disable the permission select if permission is implicit
> 
> Gregor, is there a safe way from Gaia to know if a permission is implicit or
> not?

If a certified app doesn't request a certain permission, we don't grant this permission. So all permissions that are set to ALLOW are implicit. There is one exception: if the user allows and remembers a certain prompted (explicit) permission, it will also return ALLOW.
Flags: needinfo?(anygregor)
On the Permission table in [1], all the permissions that have as action ALLOW_ACTION are implicit. I don't think that table is visible from out of the platform though.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#53
(In reply to Gregor Wagner [:gwagner] from comment #14)
> (In reply to Etienne Segonzac (:etienne) from comment #13)
> > (In reply to Antonio Manuel Amaya Calvo from comment #10)
> > > I'm sorry, bar that, I was wrong. It isn't a type of application
> > > restriction, is a type of app/type of permission restriction. Implicit
> > > permission should not be modifiable for any kind of application according to
> > 
> > So:
> > * List *all* apps
> > * Do not display uninstall button if !app.removable
> > * Disable the permission select if permission is implicit
> > 
> > Gregor, is there a safe way from Gaia to know if a permission is implicit or
> > not?
> 
> If a certified app doesn't request a certain permission, we don't grant this
> permission. So all permissions that are set to ALLOW are implicit. There is
> one exception: if the user allows and remembers a certain prompted
> (explicit) permission, it will also return ALLOW.

That's a pretty big exception. If we disable permissions changes when it returns ALLOW so that implicit permissions aren't editable, then the user will never be able to edit a permission once the "remember" checkbox is checked.

I don't think this is acceptable.
(In reply to Etienne Segonzac (:etienne) from comment #13)
> So:
> * List *all* apps
> * Do not display uninstall button if !app.removable

It should actually be Ci.nsIPrincipal.APP_STATUS_CERTIFIED not !app.removable.  As Antonio mention, we shouldn't assume they are synonymous.

> * Disable the permission select if permission is implicit

The problem is that we've failed to differentiate between implicit and explicitly permitted permissions in the code.  We should keep track of the permission type (implicit or explicit) in addition to its state.
(In reply to Lucas Adamski from comment #17)
> (In reply to Etienne Segonzac (:etienne) from comment #13)
> > So:
> > * List *all* apps
> > * Do not display uninstall button if !app.removable
> 
> It should actually be Ci.nsIPrincipal.APP_STATUS_CERTIFIED not
> !app.removable.  As Antonio mention, we shouldn't assume they are synonymous.

The application status from nsIPrincipal is not exposed to content by any API afaict.

> > * Disable the permission select if permission is implicit
> 
> The problem is that we've failed to differentiate between implicit and
> explicitly permitted permissions in the code.  We should keep track of the
> permission type (implicit or explicit) in addition to its state.

I agree. That also makes it harder to fix bug 812289.
Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.
Target Milestone: --- → B2G C2 (20nov-10dec)
Is it not possible for now to simply duplicate the content of http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#53 and to show but disable permission based on that?
We already struggle to keep this table up to date in gecko, so I'd rather not duplicate it anywhere if possible.
This bug is confusing. The summary says that implicit permissions for certified apps are not listed, but that they should be.

The text in comment 0 however says the opposite. That implicit permissions for certified apps can be modified but that that is wrong.


To keep things sane and simple for v1, I definitely things that we should not list certified apps in the application permissions UI at all. So we should neither list which permissions that they have, nor allow them to be changed.
(In reply to Antonio Manuel Amaya Calvo from comment #0)
> According to
> https://wiki.mozilla.org/Apps/Security#Implicitly_granted_permissions
> implicit permissions can be inspected but not modified. 

This document was never intended to be a comprehensive UI spec.
(In reply to Fabrice Desré [:fabrice] from comment #21)
> We already struggle to keep this table up to date in gecko, so I'd rather
> not duplicate it anywhere if possible.

Sure. But are we going to change the permission API to expose 'implicit' permission? If not I'm not sure what else to do.
Here's what I think we should do:

* Don't list any certified apps.
* Don't make any implicit permissions modifiable. (I'm not even sure that we
  should display implicit permissions).
* Only list permissions that the app has stored in the permission database.
* For explicit permissions, it should allow switching between allow/deny/prompt.
  Alternatively show what the current state is and allow moving back to prompt.

What makes things a little tricky here is that for some APIs, like "contacts" there are several permissions in the permissions database that affects the API. For "contacts" the database can contain "contacts-read", "contacts-write" and/or "contacts-create".

If any of these settings are "allow" we should show "allow" in the UI. If none are "allow" but any of them are "deny" we should show "deny" in the UI. If all of them are "prompt" we should show "prompt" in the UI.

If the user modifies the state for "contacts", I.e. sets it to "deny" "allow" or "prompt", we should modify all of the existing "contacts-*" properties to the new state. But we should never create new ones.

So if the permissions database contains "contacts-read: allow" and "contacts-create: allow" but no "contacts-write", and the user sets the permission to "prompt", we the database should be changed to contain "contacts-read: prompt" and "contacts-create: prompt".

This applies to "device-storage:pictures", "device-storage:videos", "device-storage:music", "device-storage:sdcar" and "contacts".
I also don't think we need the platform to expose a list of permissions which are explicit vs. implicit. We can hardcode that in the settings app. We need to hardcode the name that is shown in the UI and which permission in the backend that maps to anyway, so hardcoding which ones are implicit vs. explicit doesn't seem like a problem.
Closing as invalid - see https://bugzilla.mozilla.org/show_bug.cgi?id=816806#c2 along with Jonas's comment. Listing of certified apps in the settings apps permissions UI is by design.

I'm going to open up separate bugs breaking down Jonas's analysis shortly.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
blocking-basecamp: + → ---
(In reply to Jason Smith [:jsmith] from comment #28)
> Closing as invalid - see
> https://bugzilla.mozilla.org/show_bug.cgi?id=816806#c2 along with Jonas's
> comment. Listing of certified apps in the settings apps permissions UI is by
> design.

Not listing certified apps in the settings apps permissions UI I meant.

> 
> I'm going to open up separate bugs breaking down Jonas's analysis shortly.
Based on Jonas's input and comparing it to the existing implementation, I filed:

- bug 817031
- bug 817034

The only small caveat I'd stick as an extension is that if we try to install an app that has a permission that's always denied, then we should not show it in the settings UI at all.

The pieces about the certified apps not showing up currently is implemented. The prompt pieces appear to be implemented, but needs to be limited to PROMPT style permissions only.
You need to log in before you can comment on or make changes to this bug.