Per-settings permissions not working for privileged apps

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

Trunk
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 1 obsolete attachment)

As reported in Bug 846200, privileged apps that try to use a per-settings permission (i.e. settings:wallpaper.image) are not able to see the settings API to access the value.
(Assignee)

Updated

4 years ago
Blocks: 1107982
Whiteboard: [systemsfe]
Shouldn't we fix this?
Flags: needinfo?(kyle)
Yeah, probably should. I'll get a test app going to see what the problem is here. Also asking for 2.2 blocking.
blocking-b2g: --- → 2.2?
Flags: needinfo?(kyle)
Broken feature.
blocking-b2g: 2.2? → 2.2+
Confirmed this via the dev_apps/uitest-priviledged app. We're never prompting for settings access, so what I'm guessing is that we've got a chicken and egg problem with privileges. We need settings-api-* before settings:wallpaper.image, but this doesn't fall back correctly so we never prompt.
Er, ok, worse than I thought. All APIs that require permissions prompting have to handle it themselves, which is something that settings never did, which is why this doesn't work. This should've landed as part of bug 846200.

gwagner, since this has basically never worked, how far back do we want to uplift this? No one is using it yet, obviously.
Flags: needinfo?(anygregor)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #5)
> Er, ok, worse than I thought. All APIs that require permissions prompting
> have to handle it themselves, which is something that settings never did,
> which is why this doesn't work. This should've landed as part of bug 846200.
> 
> gwagner, since this has basically never worked, how far back do we want to
> uplift this? No one is using it yet, obviously.

Lets fix on master and decide based on the complexity.
Flags: needinfo?(anygregor)
Talked to sicking on IRC, apparently we're cool with wallpaper settings be allowed in privileged apps. Gaia uitest-privileged incoming so we have a place to confirm this.
Attachment #8547811 - Flags: review?(jonas)
Attachment #8547833 - Flags: review?(lissyx+mozillians)
Try is green after a couple of retriggers. The code itself is fine by me except a small nit. More to come after playing with the app.
Kyle, I don't get any error when testing this on a Gecko that do not has the patch for granting permission.
Flags: needinfo?(kyle)
(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Kyle, I don't get any error when testing this on a Gecko that do not has the
> patch for granting permission.

I get (expected) errors when the permission is removed from the manifest.

In both case, I did reset-gaia.
(Assignee)

Updated

4 years ago
Flags: needinfo?(kyle)
Version: 25 Branch → Trunk
STR:
 0. curl https://github.com/mozilla-b2g/gaia/pull/27333.patch | git apply
 1. make PRODUCTION=0 reset-gaia
 2. Use the "UI Tests - Privileged" app, Settings section

Expected:
 I see an error message

Actual:
 I see messages stating that the wallpaper has been set
STR:
 0. remove the |"settings:wallpaper.image": { "access": "readwrite"}| permission from |dev_apps/uitest-privileged/manifest.webapp|
 1. make PRODUCTION=0 reset-gaia
 2. Use the "UI Tests - Privileged" app, Settings section

Expected:
 I see an error message

Actual:
 I see no error message, logcat shows:

> 01-13 17:45:29.719 4056 4056 E UI tests - Privileged App: Content JS ERROR: ERROR undefined
> 01-13 17:45:29.719 4056 4056 E UI tests - Privileged App: at settingsTest.onError (app://uitest-privileged.gaiamobile.org/js/API/settings.js:13:4)
> 01-13 17:45:29.719 4056 4056 E UI tests - Privileged App: at settingsTest.runTest (app://uitest-privileged.gaiamobile.org/js/API/settings.js:38:6)
> 01-13 17:45:29.719 4056 4056 E UI tests - Privileged App: at onload (app://uitest-privileged.gaiamobile.org/js/API/settings.js:46:2)
From comment 12, we can infer that the privileged app has been granted mozSettings permission for the wallpaper.image key.

From comment 13, we can see that the application cannot access the API itself, which is a classic symptom of the permission being missing.
And to make it clear, I did the steps of comment 12 first, then I applied comment 13.
STR:
 0. Changing the permission from "readwrite" to "readonly"
 1. make PRODUCTION=0 reset-gaia
 2. 

Expected:
 Success at reading setting, error at setting the setting value

Actual:
 Both did work, just the error feedback is missing from the UI: we get the successfully retrieved message, but we don't get the error saying we could not set wallpaper

The logcat, however, confirms it works:
> 01-13 19:42:41.350  9986  9986 I Gecko   : -*- SettingsRequestManager: set not allowed on wallpaper.image
> 01-13 19:42:41.380 10522 10522 I Gecko   : -*- SettingsManager: error:No permission to set wallpaper.image
> 01-13 19:42:41.400 10522 10522 I Gecko   : -*- SettingsManager: Lock finalize failed: {0f51aa29-3905-42d6-acc4-59e51fcbbee5}
> 01-13 19:42:41.410 10522 10522 E UI tests - Privileged App: Content JS ERROR: ERROR undefined 
> 01-13 19:42:41.410 10522 10522 E UI tests - Privileged App:     at settingsTest.onError (app://uitest-privileged.gaiamobile.org/js/API/settings.js:13:4)
> 01-13 19:42:41.410 10522 10522 E UI tests - Privileged App:     at settingsTest.runTest/req.onsuccess/req2.onerror (app://uitest-privileged.gaiamobile.org/js/API/settings.js:30:10)
Ok, finally figured out what's wrong with the test. Bug 1014410 changes all prompt actions to allow actions for preinstalled apps, meaning that even when gecko is set to PROMPT_ACTION on settings:wallpaper.image, it'll default to ALLOW_ACTION because uitest-privileged is a preinstalled app.

So, the REAL test would be installing a privileged app from the outside. However, I'm going to update the uitest-privileged patch and land it anyways because it's still useful. I added a test to make sure we can't access other settings in it, also.
Fixed error printing, added test to make sure we can't get to other settings.
Attachment #8547833 - Attachment is obsolete: true
Attachment #8547833 - Flags: review?(lissyx+mozillians)
Attachment #8548417 - Flags: review?(lissyx+mozillians)
Thanks for investigating why we had the strange behavior :)
Attachment #8548417 - Flags: review?(lissyx+mozillians) → review+
(Assignee)

Updated

4 years ago
Attachment #8547811 - Flags: review?(jonas) → review?(fabrice)
Attachment #8547811 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/e37e5a3677b9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Please nominate these patches for b2g37/v2.2 approval when you get a chance.
Flags: needinfo?(kyle)
Comment on attachment 8547811 [details] [diff] [review]
Patch 1 (v1) - Change settings:wallpaper.image permission from PROMPT to ALLOW for privileged apps

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 846200
User impact if declined: Privileged apps can't use wallpaper image setting
Testing completed: Test written in gaia privileged ui tests
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Flags: needinfo?(kyle)
Attachment #8547811 - Flags: approval-mozilla-b2g37?
Comment on attachment 8548417 [details] [review]
Patch 2 (v2) - Gaia UI Test for Privileged access to Settings API

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 846200
User impact if declined: Privileged apps can't use wallpaper image setting
Testing completed: Test written in gaia privileged ui tests
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Attachment #8548417 - Flags: approval-mozilla-b2g37?
Attachment #8547811 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8548417 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.