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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

clarification: I am suggesting we should only grant the permissions that our apps ask for at initial build/install time.

Comment 2

[[:fabrice] Fabrice Desré]

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;
}

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Silently upgrade PROMPT permissions for privileged preinstalled apps

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

Comment 4

[[:fabrice] Fabrice Desré]

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.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

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

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2, with nit fixes

r=fabrice

Comment 7

[[:fabrice] Fabrice Desré]

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

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

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)

Comment 9

[[:fabrice] Fabrice Desré]

Yes, please land.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e81f6206ab

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

[James Burke [:jrburke]]

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?

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 14

[James Burke [:jrburke]]

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?

Comment 15

[James Burke [:jrburke]]

I filed bug 1022760 to track the DEBUG=1 issue, and bug 1022768 to track the tests issue.

Comment 16

[Mason Chang [Inactive] [:mchang]]

Note to self, see if this is the issue with the email FTU start up slowing down.