Automatically add/grant ALLOW_ACTION permission for all requested PROMPT_ACTION permissions for privileged gaia apps that ship with devices

RESOLVED FIXED in mozilla32

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: asuth, Assigned: amac)

Tracking

unspecified
mozilla32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

There are APIs like mozContacts that are automatically allowed (ALLOW_ACTION) for certified apps, but require prompting the user (PROMPT_ACTION) for privileged apps.  We should automatically add these permissions for the Gaia apps that ship with the device because:

- We are de facto vouching for the apps and it's bad/weird UX to prompt the user.

- This will let us downgrade more apps from certified to privileged which is good for security and good for making it more possible for replacement apps to come into being.

- Fabrice says it "should be easy to do at first run time when we set the permissions for the preloaded apps".
clarification: I am suggesting we should only grant the permissions that our apps ask for at initial build/install time.
Summary: Automatically add/grant ALLOW_ACTION permission for all PROMPT_ACTION permissions for privileged gaia apps that ship with devices → Automatically add/grant ALLOW_ACTION permission for all requested PROMPT_ACTION permissions for privileged gaia apps that ship with devices
Component: General → DOM: Apps
OS: Linux → All
Product: Firefox OS → Core
Hardware: x86_64 → All
So what we need to do is at first run, when we set permissions for preloaded apps:
if (app.origin == systemApp.origin && permission == PROMPT) {
  permission = ALLOW;
}
Assignee: nobody → amac
Try run is at https://tbpl.mozilla.org/?tree=Try&rev=10504a81863d

I tried this with the uitest-privileged app. Without the patch I get the following permissions:

uitest-privileged.gaiamobile.org|audio-capture|PROMPT
uitest-privileged.gaiamobile.org|video-capture|PROMPT
uitest-privileged.gaiamobile.org|contacts-read|PROMPT
uitest-privileged.gaiamobile.org|contacts-create|PROMPT
uitest-privileged.gaiamobile.org|contacts-write|PROMPT
uitest-privileged.gaiamobile.org|device-storage:music-read|PROMPT
uitest-privileged.gaiamobile.org|device-storage:music-create|PROMPT
uitest-privileged.gaiamobile.org|device-storage:music-write|PROMPT
uitest-privileged.gaiamobile.org|device-storage:pictures-read|PROMPT
uitest-privileged.gaiamobile.org|device-storage:pictures-create|PROMPT
uitest-privileged.gaiamobile.org|device-storage:pictures-write|PROMPT
uitest-privileged.gaiamobile.org|device-storage:sdcard-read|PROMPT
uitest-privileged.gaiamobile.org|device-storage:sdcard-create|PROMPT
uitest-privileged.gaiamobile.org|device-storage:sdcard-write|PROMPT
uitest-privileged.gaiamobile.org|device-storage:videos-read|PROMPT
uitest-privileged.gaiamobile.org|device-storage:videos-create|PROMPT
uitest-privileged.gaiamobile.org|device-storage:videos-write|PROMPT
uitest-privileged.gaiamobile.org|geolocation|PROMPT

And with the patch:

uitest-privileged.gaiamobile.org|audio-capture|PROMPT
uitest-privileged.gaiamobile.org|video-capture|PROMPT
uitest-privileged.gaiamobile.org|contacts-read|ALLOW
uitest-privileged.gaiamobile.org|contacts-create|ALLOW
uitest-privileged.gaiamobile.org|contacts-write|ALLOW
uitest-privileged.gaiamobile.org|device-storage:music-read|ALLOW
uitest-privileged.gaiamobile.org|device-storage:music-create|ALLOW
uitest-privileged.gaiamobile.org|device-storage:music-write|ALLOW
uitest-privileged.gaiamobile.org|device-storage:pictures-read|ALLOW
uitest-privileged.gaiamobile.org|device-storage:pictures-create|ALLOW
uitest-privileged.gaiamobile.org|device-storage:pictures-write|ALLOW
uitest-privileged.gaiamobile.org|device-storage:sdcard-read|ALLOW
uitest-privileged.gaiamobile.org|device-storage:sdcard-create|ALLOW
uitest-privileged.gaiamobile.org|device-storage:sdcard-write|ALLOW
uitest-privileged.gaiamobile.org|device-storage:videos-read|ALLOW
uitest-privileged.gaiamobile.org|device-storage:videos-create|ALLOW
uitest-privileged.gaiamobile.org|device-storage:videos-write|ALLOW
uitest-privileged.gaiamobile.org|geolocation|PROMPT

Note that the permissions for audi-capture, video-capture, and geolocation are still PROMPT (as they should be, since they're prompt for certified also).
Attachment #8428639 - Flags: review?(fabrice)
Comment on attachment 8428639 [details] [diff] [review]
V1. Silently upgrade PROMPT permissions for privileged preinstalled apps

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

thanks! Can we add a test for that? I think we already have a privileged app in the default test profile to which we could add a prompt permission.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +155,5 @@
> +            aApp.isPreinstalled &&
> +            PermissionsTable[permName][appStatus] === PROMPT_ACTION &&
> +            appStatus === "privileged" ?
> +                PermissionsTable[permName]["certified"] :
> +                PermissionsTable[permName][appStatus];

Nit: indentation is weird here. I usually find it easier to read when '?' and ':' are aligned.

::: dom/apps/src/Webapps.jsm
@@ +409,3 @@
>      }
>  
> +    // Beyond this point we know it's really a preinstalled app

Nit: full stop at the end of the comment.
Attachment #8428639 - Flags: review?(fabrice) → review+
Is the default profile [1]? 

If so, there are some privileged apps there, but they're husks (there's no real manifest). This code is ran only at first run time (and at first run time after an update). And I don't know what "coreAppsDir" is going to have on the test environment. I'm going to try a couple of things but I'm not sure I can write a test for this (and there's actually no test that I could find for loadAndUpdateApps either). 

[1] http://mxr.mozilla.org/mozilla-central/source/testing/profiles/webapps_mochitest.json
r=fabrice
Attachment #8428639 - Attachment is obsolete: true
(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #5)
> Is the default profile [1]? 
> 
> If so, there are some privileged apps there, but they're husks (there's no
> real manifest). This code is ran only at first run time (and at first run
> time after an update). And I don't know what "coreAppsDir" is going to have
> on the test environment. I'm going to try a couple of things but I'm not
> sure I can write a test for this (and there's actually no test that I could
> find for loadAndUpdateApps either). 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/testing/profiles/
> webapps_mochitest.json

Hm, yes. So please file a followup to get tests for that (including what's happening in loadAndUpdateApps).

thanks!
Blocks: 1016503
Blocks: 1016889
Ok, I filed bug 1016889 to write the unit tests. I'll take that bug if none else does (since I already started the first step at least), but probably not on the 2.0 timeframe.

Can we checkin this then? (would love to get this on 2.0 for loop if nothing else)
Yes, please land.
https://hg.mozilla.org/mozilla-central/rev/e2e81f6206ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
This works great for an on-device install. I am running into problems with email as a privileged app (which wants contacts access) in these two scenarios:

1) DEBUG=1 scenarios where the generated profile is loaded in desktop nightly firefox. I see this error a few times:

System JS : ERROR resource://gre/modules/PermissionPromptHelper.jsm:106 - ReferenceError: permissionPromptService is not defined

2) For marionettejs tests run via bin/gaia-marionette. When the app runs from that test profile, the permission prompt for contacts now shows when running email in a test, which stops the test from completing.

Is it possible to get a similar auto-allow for those two cases?
I think the problem is that the permissions aren't being loaded at all. I ran into that problem while writing the test for bug 833633. You can try setting "dom.webapps.useCurrentProfile" to true and seeing if that lets you run the tests.
Setting that pref did not seem to help. I set it in build/config/custom-prefs.js, I tried both this style:

pref("dom.webapps.useCurrentProfile", true);

and

user_pref("dom.webapps.useCurrentProfile", true);

and in both cases confirmed the profile-test/user.js contained that pref. However the tests still prompted for contacts access.

Is there a mechanism to seed the allow of the specific contact permission for email as part of a build/profile setup? Some pref or file I could write on the file system?
Depends on: 1021732
I filed bug 1022760 to track the DEBUG=1 issue, and bug 1022768 to track the tests issue.
Note to self, see if this is the issue with the email FTU start up slowing down.
Flags: needinfo?(mchang)
Flags: needinfo?(mchang)

Updated

5 years ago
See Also: → 1024513
Blocks: 1219695
Blocks: 1227011

Updated

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