Remember my permissions for PROMPT_ACTION composed permission (permission-read, permission-write) WebAPIs are lost on an app update

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: amac, Assigned: amac)

Tracking

({dataloss})

Trunk
mozilla33
ARM
Gonk (Firefox OS)
dataloss
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Steps:

1. Install a packaged app calling out contacts: {access: read} support
2. Access the contacts 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 contacts in the updated app

Expected:

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

Actual:

The contacts-read permission reverts to ask. So remembering my permissions for allow is lost on an update of the app.
(Assignee)

Comment 1

4 years ago
Created attachment 8449545 [details] [diff] [review]
V1. Check the correct permission name on the update

The problem here is that I was checking the original permission name (instead of the expanded permission name) to see if the user had granted the permission previously. That works for normal permissions, but won't work for composed permissions, or permissions that are substitued.

Added a test case for this also... better late than never I guess :)

Try run is at https://tbpl.mozilla.org/?tree=Try&rev=356616c57606
(Assignee)

Comment 2

4 years ago
Cancelled the try... now I remember why that test wasn't written, contacts-read is privileged and the test installs web apps... Will try with a substituted permission instead and update the patch.
(Assignee)

Comment 3

4 years ago
And bar that also... there are only three web prompt permissions, which were the ones I used on the original patch... and none of them are composed in any way.
Do we need this for v2.0 (for the mobile Loop app)?
Flags: needinfo?(oteo)
Flags: needinfo?(amac)
(Assignee)

Comment 5

4 years ago
Created attachment 8449652 [details] [diff] [review]
v2. Now with tests that actually pass. I hope

Try run at https://tbpl.mozilla.org/?tree=Try&rev=12386f0e1fc9
Assignee: nobody → amac
Attachment #8449545 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8449652 - Flags: review?(fabrice)
(Assignee)

Comment 6

4 years ago
(In reply to Maire Reavy [:mreavy] (Plz needinfo me, PTO on Jun 23) from comment #4)
> Do we need this for v2.0 (for the mobile Loop app)?

Yes, this is needed for Loop. It's also needed for any privileged app that uses the contacts, device storage, or any other privileged explicit API that has subpermissions (readonly, readwrite...).

Since without this we're losing user information, I think this should be a blocker. Otherwise let me know and I'll ask for approval (the patch is really just changing a parameter in a call, so it's very low risk anyway).
blocking-b2g: --- → 2.0?
Flags: needinfo?(amac)
Flags: needinfo?(oteo)

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8449652 [details] [diff] [review]
v2. Now with tests that actually pass. I hope

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

::: dom/apps/src/PermissionsTable.jsm
@@ +342,5 @@
>                               certified: PROMPT_ACTION
> +                           },
> +                           // This permission doesn't actually grant access to
> +                           // anything. It exists only to check the correctness
> +                           // of web prompt composed permissions.

nit: add " in tests."
Attachment #8449652 - Flags: review?(fabrice) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8452265 [details] [diff] [review]
v3, with nit fixed

r=fabrice
Attachment #8449652 - Attachment is obsolete: true
Attachment #8452265 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/bff799535eb7
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bff799535eb7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
https://hg.mozilla.org/releases/mozilla-aurora/rev/8b04bdcbd916
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
> Steps:
> 
> 1. Install a packaged app calling out contacts: {access: read} support

Hi Reporter,
    Could you provide the packaged app's file or name for me to verify this bug?

Thank you!
Flags: needinfo?(amac.bug)
(Assignee)

Comment 13

3 years ago
You can try with the FirefoxOS loop app, for example: https://github.com/mozilla-b2g/firefoxos-loop-client/
Flags: needinfo?(amac.bug)

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.