1107674 - Per-settings permissions not working for privileged apps

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 1

[Gregor Wagner [:gwagner]]

Shouldn't we fix this?

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, probably should. I'll get a test app going to see what the problem is here. Also asking for 2.2 blocking.

Comment 3

[Gregor Wagner [:gwagner]]

Broken feature.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 6

[Gregor Wagner [:gwagner]]

(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.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Change settings:wallpaper.image permission from PROMPT to ALLOW for privileged apps

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.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Gaia UI Test for Privileged access to Settings API

Comment 9

[:gerard-majax]

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.

Comment 10

[:gerard-majax]

Kyle, I don't get any error when testing this on a Gecko that do not has the patch for granting permission.

Comment 11

[:gerard-majax]

(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.

Comment 12

[:gerard-majax]

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

Comment 13

[:gerard-majax]

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)

Comment 14

[:gerard-majax]

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.

Comment 15

[:gerard-majax]

And to make it clear, I did the steps of comment 12 first, then I applied comment 13.

Comment 16

[:gerard-majax]

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)

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Gaia UI Test for Privileged access to Settings API

Fixed error printing, added test to make sure we can't get to other settings.

Comment 19

[:gerard-majax]

Thanks for investigating why we had the strange behavior :)

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

gaia: https://github.com/mozilla-b2g/gaia/commit/3c2c240075f74b85da46c3ef9a8fbac9e4723818

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e37e5a3677b9

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please nominate these patches for b2g37/v2.2 approval when you get a chance.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b707f68027a3

v2.2: https://github.com/mozilla-b2g/gaia/commit/981d33536ad23eb025deec8578cdc655a00e61fd