1022791 - Remember my permissions for PROMPT_ACTION WebAPIs are lost on an system update

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

+++ This bug was initially created as a clone of Bug #833633 +++

This is a continuation bug for bug 833633, to ensure that remembered prompt permissions are not lost on a system (OS) update (via OTA or FOTA).

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Sorry about the delay in taking this. Somehow I had forgotten completely about this. Anyway, I've done a small table of all the possibilities at [1] and unless I'm missing something, this might be even easier. In the table OLD is the permission assigned in PermissionsTable.jsm before the update, NEW is the permission in PermissionsTable.jsm after the update, STORED_PERMISSION is the permission a given APP had stored for that permission before the update, and AFTER_UPDATE is the the value it should have after the update has been applied.

The yellow cells are pre-update configurations that should not happen (basically storing a DENY for a permission that was ALLOW for a given permission and app type). As for the rest, there are four possible problematic rows:

Rows 6 and 12: On those two we switch from a PROMPT permission to an ALLOW/DENY permission and the user has selected the reverse option. For example, the user had selected DENY always for a permission that goes from PROMPT to ALLOW. In that case, we will overrule the user's decision and switch the permission to ALLOW.

Rows 16 and 18: A permission goes from ALLOW or DENY to PROMPT. In that case we could store the permission as PROMPT, or leave it as is (ALLOW/DENY). The user could then change the permission in Settings if he were inclined to do so. 

In any case, and if we take the decisions I propose on the table, then this would be quite easy to fix (since basically it's the same solution we took for app updates): If the new permission is ALLOW or DENY, we just write that. If the new permission is PROMPT, we leave whatever is actually written for the app, or PROMPT if there's nothing written.

So what do you think?


[1] https://docs.google.com/spreadsheets/d/1z4Q3u-LbpPBmMBunlYMDJiqNrd-_oRzBP8lVesvvzdk/edit?usp=sharing

Comment 2

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #1)
 
> Rows 6 and 12: On those two we switch from a PROMPT permission to an
> ALLOW/DENY permission and the user has selected the reverse option. For
> example, the user had selected DENY always for a permission that goes from
> PROMPT to ALLOW. In that case, we will overrule the user's decision and
> switch the permission to ALLOW.

I'm not a big fan of overruling a user choice, so I'm on the fence on this one. Of course ending up with a broken app because a permission is now denied while it's expected to be allowed is not a great user experience either. I wonder if we should notify the user somehow in these cases.
 
> Rows 16 and 18: A permission goes from ALLOW or DENY to PROMPT. In that case
> we could store the permission as PROMPT, or leave it as is (ALLOW/DENY). The
> user could then change the permission in Settings if he were inclined to do
> so. 

I would go for prompt but no big feeling here.

Comment 3

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

If anything, I think we should add a warning to the user that when they do system update, they permission settings will be reset. 

> > Rows 6 and 12: On those two we switch from a PROMPT permission to an
> > ALLOW/DENY permission and the user has selected the reverse option. For
> > example, the user had selected DENY always for a permission that goes from
> > PROMPT to ALLOW. In that case, we will overrule the user's decision and
> > switch the permission to ALLOW.
> 
> I'm not a big fan of overruling a user choice, so I'm on the fence on this
> one. Of course ending up with a broken app because a permission is now
> denied while it's expected to be allowed is not a great user experience
> either. I wonder if we should notify the user somehow in these cases.

We are explicitly overuling user choice here regardless - prompt -> allow/deny means that we have decided that this is a not an interesting permission for the user to make a decision on. 

prompt-> deny probably means this permission is deprecated or API is going away from this app type (or its deemed too dangerous). Either way I'm  OK with overrulling here. (otherwise we end up in a dangerous situation where the permission is forbidden, but an app has it, because it got it before it was forbidden)


Prompt -> allow means that we don't think users should have to make this decision any more. We _could_ notify, but I feel like a general warning as part of the regular system update process might be enough? Again I think we should overrule though, just mainly for consistency. 

>  
> > Rows 16 and 18: A permission goes from ALLOW or DENY to PROMPT. In that case
> > we could store the permission as PROMPT, or leave it as is (ALLOW/DENY). The
> > user could then change the permission in Settings if he were inclined to do
> > so. 
> 
> I would go for prompt but no big feeling here.

I would go with prompt - if we are switching to prompt, we are doing so to raise awareness of the permission. 
This also fits better with a overall warning that permissions are reset during system update.


Thats my 2 cents anyways.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Sorry for the time answering, I was waiting for Jonas to pipe in. So... it seems that we're ok with overruling when we go from prompt to allow or deny, with the caveat that maybe we should warn the users about that (maybe on the FTE and as a separate bug when/if a change of that type is done). 

About the change when a permission goes from ALLOW or DENY to PROMPT, the problem is that with the current data available at boot time, there´s no difference between:

[1] Pre-update Perm: PROMPT Post-update Perm: PROMPT. Stored Value: ALLOW

and

[2] Pre-update Perm: ALLOW Post-update Perm: PROMPT. Stored Value: ALLOW


If we decide just to leave the permission as it is (which is after all a valid value) then there's no problem. The app will continue working (or not working if the store value was DENY) as it was but the user now can go to the Settings app and change that value (again, we could inform him during the post-update FTE). But if we want to change that to PROMPT for [2] but not for [1], then we need to add some extra data to Gecko, to track the permission changes (between all possible update paths too, at least starting from 1.3).

So I was trying to find a compromise where we can keep the system in a secure state, and still allow the user any new freedom we might have added.

WDYT?

Comment 5

[[:fabrice] Fabrice Desré]

I don't see any issue keeping the permission as it is in this case.

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Remove dead code, mostly

The good news is that with the conditions described on comment#1, the patch is easy. The strange news is that the patch is basically irrelevant anyway, since PermissionsInstaller.installPermission expects to receive a boolean indicating if it's a system update as a parameter, and we're passing it inside of the aApp object instead (and we don't use that attribute).

So this is mostly cleanup of dead code, the code already behaves as we want it to (by chance but it does :)).

Comment 7

[[:fabrice] Fabrice Desré]

Hm, did we lose a call to updatePermissionsForApp() while doing normal updates?

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

No, the isSystemUpdate was being passed, only on an incorrect place. I don't know where was that introduced and it might even be my fault and have been incorrect from the start. 

In any case, and for blind luck, it turns out the behavior is the desired one.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3e1c091bac11

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e1c091bac11

Comment 11

[Loli (:lolimartinezcr)]

Tested and working. 
flame
user
master
188based
Gecko-aefcebe
Gaia-7256858

I have changed settings (NFC ON, Bluetooth ON, Geolocation OFF, etc), after I have updated Firefox and the "Settings" are set correctly (NFC ON, Bluetooth ON, Geolocation OFF, etc)