833633 - Remember my permissions for PROMPT_ACTION WebAPIs are lost on an app update

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 1/22/2013
Device: Unagi

Steps:

1. Install a packaged app calling out geolocation support
2. Access the geolocation API and remember my permissions to allow
3. Update the packaged app on the server
4. Check for updates (manual or automatic)
5. Update the app
6. Check the settings app perm UI or try to access geolocation in the updated app

Expected:

The geolocation permission should be to allow, not ask. So we should retain permissions remembered on an update.

Actual:

The geolocation permission reverts to ask. So remembering my permissions for allow is lost on an update of the app.

Comment 1

[Jason Smith [:jsmith]]

I actually don't know if this is worth blocking on or not, given that the workaround is to re-access the API again. I'd definitely track this.

Jonas - What do you think? Is this bad enough to consider noming or can we live with the dataloss on update if we had to ship?

Comment 2

[Jason Smith [:jsmith]]

Actually, Gregor answered my question in IRC. Nice to have, but not worth blocking.

Comment 3

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

This is very surprising. Are we losing any other data such add cookies or indexedDB data at update too?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> This is very surprising. Are we losing any other data such add cookies or
> indexedDB data at update too?

That's on my list of things to test. I'll investigate that tomorrow.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

If there is dataloss involved, please nominate for blocking.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Lukas Blakk [:lsblakk] from comment #5)
> If there is dataloss involved, please nominate for blocking.

In that case, I'll nom since this is confirmed data loss.

The data loss is easy to recover from though in the sense that you would have re-request access to the API and remember your perms again.

Comment 7

[Andrew Overholt [:overholt]]

This is affecting me and is very annoying but I'm not sure if I'd hold the release if this was the last bug.  Fabrice, what do you think?

Comment 8

[[:fabrice] Fabrice Desré]

I would not hold the release either, but would take a patch to uplift.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> I would not hold the release either, but would take a patch to uplift.

Yeah, I wouldn't block either. Annoying and temporary data loss, but not release stopping.

Definitely worth tracking, however.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

Mind if I steal this from you, Fabrice?

Also, I kind remember that this was done on purpose, but can't remember for my life the reasoning behind. Just in case, Paul, do you see any problem in remembering the permissions granted by the user through an update?

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

Ok, after checking the code I think I remember this was a feature and not a bug (or it's now that's documented :P). The problem is that the permission update code runs on two cases:

A) on the first run ever, or the first run after a OS update
B) when an app is installed or updated.

And the permissions aren't remembered because of case A). Currently there's no way to know if a OS permission has changed its value on an update (for example, it was ALLOW on 1.2 for privileged and it's PROMPT on 1.3, or it was ALLOW and it's now DENY, or it was DENY and it's now PROMPT or ALLOW). 

So to avoid the case where a permission was ALLOW and is now PROMPT or DENY, the old permissions get overwritten with the default values on installation.

A complete solution would mean keeping track of permission changes. There's a partial solution for this, though, and it's that since the problem is with the OS updates, we can reset the permissions only on OS updates, and just let the current value for PROMT stand on normal app updates.

WDYT, Fabrice, Paul?

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #15)
>
> A complete solution would mean keeping track of permission changes. There's
> a partial solution for this, though, and it's that since the problem is with
> the OS updates, we can reset the permissions only on OS updates, and just
> let the current value for PROMT stand on normal app updates.
> 
> WDYT, Fabrice, Paul?

I'm fine with that plan.

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Keep the prompt permissions that were remembered on an update

This implements the solution proposed. Fernando, since Fabrice is away till the 17th, can you take a look at this?

Comment 18

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

When an app is updated I don't think we should modify any remembered permissions. Neither ALLOW or DENY ones. If a user has made a decision then we should honor that even if an app update happens. Though maybe I am forgetting some attack vector here?


For OS permissions that change from DENY to ALLOW/PROMPT, I don't think we need to worry. Any app that asks for a permission which is listed as DENY will be prevented from being installed. So if a permission is changed from DENY to ALLOW/PROMPT then there should be no app that needs to be updated.


For OS permissions that change from ALLOW/PROMPT to DENY things are a bit trickier. This seems like a pretty odd edge case that I guess could happen if we determine that something that we thought was safe isn't actually safe. Ideally we would have a better blocklisting capability rather than simply relying on OS update though.

But I guess we in this instance could go through any apps of a particular type and make sure to change any nsIPermissionManager entries that they have to DENY. But I'm not sure that this is urgent to do given that we can always do it once we actually roll out an OS version where we need it.

I'd rather spend the time on building a better blocklisting capability.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #18)
> When an app is updated I don't think we should modify any remembered
> permissions. Neither ALLOW or DENY ones. If a user has made a decision then
> we should honor that even if an app update happens. Though maybe I am
> forgetting some attack vector here?

That's actually what the proposed patch does.

> For OS permissions that change from DENY to ALLOW/PROMPT, I don't think we
> need to worry. Any app that asks for a permission which is listed as DENY
> will be prevented from being installed. So if a permission is changed from
> DENY to ALLOW/PROMPT then there should be no app that needs to be updated.

Hmm that's not what happens. What happens if an app requests a permission that's set to DENY is that the permission is installed but set to DENY, not that the app is not installed. So I can install a hosted app that requests SMS permission, and it'll be installed. It'll just not be granted the SMS permission. 

So those apps wouldn't work if a OS update didn't reset the permissions to the default value (the proposed patch still does that though).

> For OS permissions that change from ALLOW/PROMPT to DENY things are a bit
> trickier. This seems like a pretty odd edge case that I guess could happen
> if we determine that something that we thought was safe isn't actually safe.
> Ideally we would have a better blocklisting capability rather than simply
> relying on OS update though.
> 
> But I guess we in this instance could go through any apps of a particular
> type and make sure to change any nsIPermissionManager entries that they have
> to DENY. But I'm not sure that this is urgent to do given that we can always
> do it once we actually roll out an OS version where we need it.
> 
> I'd rather spend the time on building a better blocklisting capability.

What the proposed patch does is:

* If it's an app update, for permissions that are PROMPT, keep the value that the user gave it, if it has a value already, or "prompt" if it hasn't. For all the other permissions, set the appropriate value.
* If it's an OS update, reset all the app permissions to the default value (that is, "prompt" for all prompt permissions).

Comment 20

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

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #19)
> Hmm that's not what happens.

You are correct. That's a bit of a bummer. Seems silly to write anything at all into the permission manager since effectively "no value" must be the same as DENY for these APIs. Otherwise apps could get access to the APIs by *not* listing them in the manifest :)

> What the proposed patch does is:
> 
> * If it's an app update, for permissions that are PROMPT, keep the value
> that the user gave it, if it has a value already, or "prompt" if it hasn't.
> For all the other permissions, set the appropriate value.

Sounds good.

> * If it's an OS update, reset all the app permissions to the default value
> (that is, "prompt" for all prompt permissions).

I'm less excited about this. I don't want people to avoid applying an OS update just because it means that they get new prompts again.

Could we do this:
* When installing an app, never write DENY into the permission manager. If the PermissionsTable says DENY, write nothing to the permission manager.
* When updating the OS, for any entries that say PROMPT, never overwrite an existing DENY or ALLOW value. Only write a PROMPT value if no value exists in the permission manager.
* When updating the OS, for any entries that say DENY, always remove any stored value.
* When updating the OS, for any entries that say ALLOW, never overwrite an existing DENY value. Only write a ALLOW value if no value exists or if the existing value is PROMPT.

I don't think we need to worry too much about existing apps that have DENY values in the permission database due to asking for permissions that they can't get. The marketplace wouldn't accept any apps anyway since they validate the permissions list.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #20)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #19)
> 
> > What the proposed patch does is:
> > 
> > * If it's an app update, for permissions that are PROMPT, keep the value
> > that the user gave it, if it has a value already, or "prompt" if it hasn't.
> > For all the other permissions, set the appropriate value.
> 
> Sounds good.
> 
> > * If it's an OS update, reset all the app permissions to the default value
> > (that is, "prompt" for all prompt permissions).
> 
> I'm less excited about this. I don't want people to avoid applying an OS
> update just because it means that they get new prompts again.
> 
> Could we do this:
> * When installing an app, never write DENY into the permission manager. If
> the PermissionsTable says DENY, write nothing to the permission manager.
> * When updating the OS, for any entries that say PROMPT, never overwrite an
> existing DENY or ALLOW value. Only write a PROMPT value if no value exists
> in the permission manager.
> * When updating the OS, for any entries that say DENY, always remove any
> stored value.
> * When updating the OS, for any entries that say ALLOW, never overwrite an
> existing DENY value. Only write a ALLOW value if no value exists or if the
> existing value is PROMPT.

I think there's at least one case for which the previous procedure fails. If the permission was PROMPT on the old version && the user had denied it, then we have DENY stored. If the permission is changed to ALLOW, according to the last point it won't be changed (so it'll be kept as DENY). Since the permission is not prompt anymore, the user cannot change his mind either since the permission won't be shown on the settings app anymore.

> I don't think we need to worry too much about existing apps that have DENY
> values in the permission database due to asking for permissions that they
> can't get. The marketplace wouldn't accept any apps anyway since they
> validate the permissions list.

Yeah, but packaged apps (non privileged) also can request all the permissions in the table, and they'll be stored as DENY (they'll be that way on 1.1, and on the new devices which are 1.3). I don't think we can really assume that, nor take shortcuts. If we want to get rid of this resetting we can just introduce a new table with the permissions that have actually changed (which will be few, hopefully) and do the changes correctly. And we can do that for 2.1 :).

As for the users not updating from 1.3 to 2.0 (which would be the earliest time the OS updating code will run) because of being asked again for the permissions of any installed app... well, we can just make it 2.0 awesome enough that everybody will want to update :P.

Or if you consider this a serious enough risk I can run a comparison between the permission table for 1.3 and 2.0 and create the permission change table now. But considering 2.0 FL is next week, this seems kinda risky.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

Bar my previous phrase, I would have to create the table between 1.0.1 and 2.0, 1.1 and 2.0 and 1.3 and 2.0 just to be sure (I don't think 1.0.1 and 1.1 devices are going to be updated to 2.0 but...)

Comment 23

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

Note that most of the changes discussed here are theoretical. We haven't made any changes from PROMPT to ALLOW, nor from ALLOW/PROMPT to DENY. And I can't really see any circumstances where we would.

The only changes that we've done is from DENY to ALLOW/PROMPT. In fact, I think we've only changed permissions from DENY to ALLOW.

So I think that's the only change that we really need to deal with.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8432484 [details] [diff] [review]
V1. Keep the prompt permissions that were remembered on an update

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

Thanks Antonio. LGTM.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +147,5 @@
>                              newManifest.permissions[permName].access);
>          for (let idx in expandedPermNames) {
> +
> +          let isPromptPermission =
> +            PermissionsTable[permName][appStatus] === PROMPT_ACTION;

Thanks :)

@@ +163,5 @@
>                  : PermissionsTable[permName][appStatus];
>  
> +          let permValue = PERM_TO_STRING[permission];
> +          if (!aIsSystemUpdate && isPromptPermission) {
> +            // It it's not a system update, then we should keep the prompt

typo: "If it's..."

@@ +173,5 @@
> +                                                     false);
> +            if (permValue === "unknown") {
> +              permValue = PERM_TO_STRING[permission];
> +            }
> +          } 

nit: trailing whitespace

::: dom/apps/src/Webapps.jsm
@@ +600,5 @@
>            if (!this.webapps[id]) {
>              continue;
>            }
>            this.updateOfflineCacheForApp(id);
> +          this.updatePermissionsForApp(id, isPreinstalled, true);

this.updatePermissionsForApp(id, isPreinstalled, true /* isSystemUpdate */);

::: dom/apps/tests/test_packaged_app_update.html
@@ +198,5 @@
> +                                                   permissionsToSet[permission],
> +                                                   gApp.manifestURL,
> +                                                   gApp.origin, false);
> +      } catch (e) {
> +        ok(false, "mozPermissionSettings.set failed for " + permission);

ok(false, "mozPermissionSettings.set failed for " + permission + " - " + e);

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

Are you ok if we land this as is and do the system update changes on a followup, Jonas?

Comment 26

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

Yup

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2. Keep the prompt permissions that were remembered on an update

Addressed nit comments.

r=ferjm

Try at: https://tbpl.mozilla.org/?tree=Try&rev=6ee79d35ffe0

Comment 28

[Antonio Manuel Amaya Calvo (:amac)]

Tests run good, requesting checkin

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8437059 [details] [diff] [review]
V2. Keep the prompt permissions that were remembered on an update

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Without this patch, every time a user updates an app from the marketplace (or from any other source) any PROMPT permission he might have granted (and remembered) will be reset.
Testing completed (on m-c, etc.): Yes, and the patch also includes some new mochitests.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None

Comment 30

[Ryan VanderMeulen [:RyanVM]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #27)
> Try at: https://tbpl.mozilla.org/?tree=Try&rev=6ee79d35ffe0

This looks maybe-real to me?
https://tbpl.mozilla.org/php/getParsedLog.php?id=41378768&tree=Try

Comment 31

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #30)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #27)
> > Try at: https://tbpl.mozilla.org/?tree=Try&rev=6ee79d35ffe0
> 
> This looks maybe-real to me?
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41378768&tree=Try

Agh yes. The good news is that the test that is failing doesn't run any new code, and so that isn't really a failure (will change it anyway). The bad news is that I forgot again that mozPermissionSettings looks synchronous but isn't. I'll send a new try with a changed test in a few.

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V3. Keep the prompt permissions that were remembered on an update

Try run at https://tbpl.mozilla.org/?tree=Try&rev=7233f566b966

Didn't change the code, fixed the test so it gives a known-fail when the permission isn't saved at the first check time (since that's just because of the way the mozPermissionSettings API works).

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V4. Keep the prompt permissions that were remembered on an update

r=ferjm

s/todo/todo_is

Try run only for B2G emulator at: https://tbpl.mozilla.org/?tree=Try&rev=78f2fcb14ff1 and a full one at: https://tbpl.mozilla.org/?tree=Try&rev=6d11d0e46bb1

Comment 34

[Antonio Manuel Amaya Calvo (:amac)]

Test fixed for the B2G emulator, re-requesting checkin :)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5d40127c9830

Thanks for the Try runs. Try to be more conscientious about what platforms/test suites you run, though. For example, I assume your "all" run was to make sure your test change didn't break other platforms. In that case, specifying just the one mochitest suite would have saved a lot of time and resources while accomplishing the same results. If you need some more assistance, check out the page below or feel free to ping on IRC. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> https://hg.mozilla.org/integration/b2g-inbound/rev/5d40127c9830
> 
> Thanks for the Try runs. Try to be more conscientious about what
> platforms/test suites you run, though. For example, I assume your "all" run
> was to make sure your test change didn't break other platforms. 

Yep!

> In that
> case, specifying just the one mochitest suite would have saved a lot of time
> and resources while accomplishing the same results. If you need some more
> assistance, check out the page below or feel free to ping on IRC. Thanks!
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Will do, thank you!

Comment 37

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8438378 [details] [diff] [review]
V4. Keep the prompt permissions that were remembered on an update

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Without this patch, every time a user updates an app from the marketplace (or from any other source) any PROMPT permission he might have granted (and remembered) will be reset back to PROMPT.
Testing completed (on m-c, etc.): Yes, and the patch also includes some new mochitests.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d40127c9830

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8438378 [details] [diff] [review]
V4. Keep the prompt permissions that were remembered on an update

Ditto Ryan's comment on thanks for the try runs. Seems like you flushed out some of the bugs via the few landings and now look stable. Aurora approval granted.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/abd8f8a2645e

Comment 41

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

Looks good to me.