860782 - Packaged apps permissions need to be copied into the app profile at install time

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Wesley Johnston (:wesj)]

Packaged apps set up their permissions when they're installed. Unfortunately, that means they're currently setting themselves up in our Fennec profile. We need to somehow copies those to the right profile.

This may be a dupe of bug 832960 but I wanted it filed until we were sure.

Comment 1

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #0)
> This may be a dupe of bug 832960 but I wanted it filed until we were sure.

I don't think it's a dupe. That bug talks more about a flow within the permission installer. This one is general profile management setup during install for apps claiming permissions in their manifest.

Note - this applies to hosted apps, as well.

Comment 2

[[:fabrice] Fabrice Desré]

This also applies to app updates.

The big problem here is that when installing from the Fennec process, the permission manager initialize its DB to store the permissions.sqlite file early on (https://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#413) and nothing allows changing that on the fly safely.

One option is to *not* do any permission setting in Fennec at install time, but to grant them at first run of the app itself. That should also happen at updates. You will be able to know if an app has been updated by using the updateTime property of the app (https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/nsIDOMApplicationRegistry.idl#51).

Comment 3

[James Hugman [:jhugman] [@jhugman]]

Attachment: First cut at moving permissions installation to first time run.

Comment 4

[James Hugman [:jhugman] [@jhugman]]

Also submitted to try. https://tbpl.mozilla.org/?tree=Try&rev=a16ecab12ace

Comment 5

[Jason Smith [:jsmith]]

DOM APIs are supposed intentionally aim to be cross-platform as much as possible with light exceptions. However, this patch pulls business logic from the DOM API into the front-end code and locks certain code away in the DOM to only execute in B2G. On that note, I don't think this patch makes a whole lot of sense to me on that regard.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #5)
> DOM APIs are supposed intentionally aim to be cross-platform as much as
> possible with light exceptions. However, this patch pulls business logic
> from the DOM API into the front-end code and locks certain code away in the
> DOM to only execute in B2G. On that note, I don't think this patch makes a
> whole lot of sense to me on that regard.

I need to clarify my wording here.

Should mean to say "cross-platform" in the sense for APIs that are intended to be cross-platform in certain layers of the code. For certain APIs, at certain levels it makes sense that the API goes off and does platform-specific logic. But at JS level in DOM Apps, I don't think the changes here make a whole lot of sense to turn off code on FxAndroid and pulling it up into the front-end code.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 760359 [details] [diff] [review]
First cut at moving permissions installation to first time run.


>     // Install the permissions for this app, as if we were updating
>     // to cleanup the old ones if needed.
>+    // TODO It's not clear what this should do when there are mutiple profiles.
>+#ifdef MOZ_WIDGET_GONK

All these should be MOZ_B2G. MOZ_WIDGET_GONK is only true on device, so this would break b2g desktop builds.
I'm not sure either that this is the right thing to do overall since the desktop runtime will need something similar to firefox for android.

That will be cleaner once I'll get to refactor the webapps code, so I won't block on this in this bug.

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js
>--- a/mobile/android/chrome/content/WebAppRT.js
>+++ b/mobile/android/chrome/content/WebAppRT.js
>@@ -3,16 +3,17 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> let Cc = Components.classes;
> let Ci = Components.interfaces;
> let Cu = Components.utils;
>
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/FileUtils.jsm");
> Cu.import("resource://gre/modules/NetUtil.jsm");
>+Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
>
> function pref(name, value) {
>   return {
>     name: name,
>     value: value
>   }
> }
>
>@@ -46,58 +47,85 @@
>       Services.perms.add(uri, "offline-app", Ci.nsIPermissionManager.ALLOW_ACTION);
>       Services.perms.add(uri, "indexedDB", Ci.nsIPermissionManager.ALLOW_ACTION);
>       Services.perms.add(uri, "indexedDB-unlimited", Ci.nsIPermissionManager.ALLOW_ACTION);
>
>       // update the blocklist url to use a different app id
>       let blocklist = Services.prefs.getCharPref("extensions.blocklist.url");
>       blocklist = blocklist.replace(/%APP_ID%/g, "[e-mail]");
>       Services.prefs.setCharPref("extensions.blocklist.url", blocklist);
>+
>+      this.getManifestFor(aUrl, function (aManifest, aManifestURL) {
>+        if (aManifest) {
>+          PermissionsInstaller.installPermissions(
>+            { manifest: aManifest,
>+              origin: aUrl,
>+              manifestURL: aManifestURL },
>+            true);
>+        }
>+      });
>     }
>
>     this.findManifestUrlFor(aUrl, aCallback);
>   },
>
>-  findManifestUrlFor: function(aUrl, aCallback) {
>+  getManifestFor: function (aUrl, aCallback) {
>     let request = navigator.mozApps.mgmt.getAll();

I know this was here before, but that is very inefficient. You can probably poke at some Webapps.jsm internal to optimize this search.

>+# Enable the production cert for verifying signed packaged apps.
>+MOZ_B2G_CERTDATA=1

I don't think this belongs to this patch?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > DOM APIs are supposed intentionally aim to be cross-platform as much as
> > possible with light exceptions. However, this patch pulls business logic
> > from the DOM API into the front-end code and locks certain code away in the
> > DOM to only execute in B2G. On that note, I don't think this patch makes a
> > whole lot of sense to me on that regard.
> 
> I need to clarify my wording here.
> 
> Should mean to say "cross-platform" in the sense for APIs that are intended
> to be cross-platform in certain layers of the code. For certain APIs, at
> certain levels it makes sense that the API goes off and does
> platform-specific logic. But at JS level in DOM Apps, I don't think the
> changes here make a whole lot of sense to turn off code on FxAndroid and
> pulling it up into the front-end code.

IMO the DOM code crossed a line, a blurry line for sure, when it started making assumptions about profiles. As Fabrice said, we'll need to rework the DOM code for the desktop runtime as well. The WebAppRT.js code is meant to be "startup/update" code, and is not front-end code.

This will keep evolving, but I agree with Fabrice, we should not block on it in this patch.

Comment 9

[[:fabrice] Fabrice Desré]

Fwiw, the DOM code is in Webapp.js, and it's fine. We have coupling issues in the backend.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 760359 [details] [diff] [review]
First cut at moving permissions installation to first time run.

Feedback+ from me on the concept. I'm OK with using WebAppRT.js until some other approach is created.

Wes and Fabrice are the real reviewers here.

Comment 11

[James Hugman [:jhugman] [@jhugman]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> >diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js
> >--- a/mobile/android/chrome/content/WebAppRT.js
> >+++ b/mobile/android/chrome/content/WebAppRT.js

> >     this.findManifestUrlFor(aUrl, aCallback);
> >   },
> >
> >-  findManifestUrlFor: function(aUrl, aCallback) {
> >+  getManifestFor: function (aUrl, aCallback) {
> >     let request = navigator.mozApps.mgmt.getAll();
> 
> I know this was here before, but that is very inefficient. You can probably
> poke at some Webapps.jsm internal to optimize this search.
> 

I'm having trouble with this approach. The WebAppsRT.jsm module is running in a different profile/process to Webapps.jsm, so doesn't have access to the same instance of the module, so can't see the state which would be used to resolve the correct manifest. 

I know I'm missing something here.

Comment 12

[[:fabrice] Fabrice Desré]

The navigator.mozApps.mgmt.getAll() call will end up looking up data in the current process webappsDir. For fennec, this is a shared directory according to https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#39

So I looks to me that Webapps.jsm will load the same data from all processes, main fennec or app specific.

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 760359 [details] [diff] [review]
First cut at moving permissions installation to first time run.

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

Seems good if you can explain (remove/simplify?) the url opening logic.

+    // TODO It's not clear what this should do when there are mutiple profiles.
Nit: Spelling

+      this.getManifestFor(aUrl, function (aManifest, aManifestURL) {
+        if (aManifest) {
+          PermissionsInstaller.installPermissions(
+            { manifest: aManifest,
+              origin: aUrl,
+              manifestURL: aManifestURL },
+            true);
I'd like to move the orientation permission to here as well, but that can be done separately.


+      } else if (aManifest.fullLaunchPath() == aUrl) {
+        // Otherwise, see if the apps launch path is this url
+        aCallback(aUrl);
+        return;
+      } else {
// Hmm... In this case we're loading a "random" url with the permissions of a particular app?
// What's the use case here where we have a manifest, but we don't want to launch with fullLaunchPath()?

Comment 14

[James Hugman [:jhugman] [@jhugman]]

After consultation with bwalker, replacing the inefficient code should be tracked in an Android specific bug. Please see bug 883121.

Comment 15

[James Hugman [:jhugman] [@jhugman]]

Attachment: Second patch following feedback.

Corrected nits from wesj, and #ifdef from fabrice.

Comment 16

[James Hugman [:jhugman] [@jhugman]]

> // What's the use case here where we have a manifest, but we don't want to
> launch with fullLaunchPath()?


 * opening the app directly onto a non-front page. e.g. opening a profile page in the twitter app.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72affddbc575

Is this something we can test?

Comment 18

[Wesley Johnston (:wesj)]

We're trying to get the desktop tests for this (at least, I hope this is covered in there...) running in bug 873567.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/05697f33c32c

https://tbpl.mozilla.org/php/getParsedLog.php?id=24389797&tree=Mozilla-Inbound

Comment 20

[Jason Smith [:jsmith]]

Looks like the basic mochitest smoke test of this feature failed.

dom/tests/browser/browser_webapps_permissions.js

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Use app when requesting permissions

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Disable test

So after looking at this test, it expects the permissions to be set in the Firefox profile at install time. That should only be happening on B2G though (its the only runtime that runs Firefox and Apps in the same process). As such, I think we should disable the test on Desktop (and Android) for now, and write some new tests for their runtimes. That's going to be... hard, so for now this just disables the test on desktop to unblock us.

Try looked fine: https://tbpl.mozilla.org/?tree=Try&rev=3958a0e4fe15

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 766969 [details] [diff] [review]
Patch 3/3 - Disable test

Trying to move this forward.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 766969 [details] [diff] [review]
Patch 3/3 - Disable test

Given Wes' explanation, this patch makes sense.

Comment 25

[Wesley Johnston (:wesj)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Jason Smith [:jsmith]]

See bug 889123 for a followup. This just broke a workflow for Gaia development on Firefox OS.

Comment 28

[Nick Alexander :nalexander [he/him]]

The attached patch is not valid JS: http://hg.mozilla.org/mozilla-central/rev/7e01fb914a16#l2.14 should have a trailing comma or nothing, but not a semi-colon.  Correct?

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #28)
> The attached patch is not valid JS:
> http://hg.mozilla.org/mozilla-central/rev/7e01fb914a16#l2.14 should have a
> trailing comma or nothing, but not a semi-colon.  Correct?

Right, but that was landed in bug 888836, and should be fixed in bug 890976