Closed Bug 860782 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 25
Tracking Status
fennec + ---

People

(Reporter: wesj, Assigned: jhugman)

References

Details

(Whiteboard: [A4A] [packagedapps])

Attachments

(3 files, 1 obsolete file)

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.
Component: General → Web Apps
QA Contact: aaron.train
Whiteboard: A4A
(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.
Priority: -- → P1
Whiteboard: A4A → [A4A]
OS: Windows 7 → Android
Hardware: x86_64 → ARM
Version: unspecified → Trunk
tracking-fennec: --- → ?
tracking-fennec: ? → +
Whiteboard: [A4A] → [A4A] [packagedapps]
Blocks: 835405
Blocks: 865736
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).
Assignee: nobody → jhugman
No longer blocks: 835405
Attachment #760359 - Flags: review?(wjohnston)
Attachment #760359 - Flags: review?(mark.finkle)
Attachment #760359 - Flags: review?(fabrice)
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.
(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 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, "webapprt-mobile@mozilla.org");
>       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?
Attachment #760359 - Flags: review?(fabrice)
(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.
Fwiw, the DOM code is in Webapp.js, and it's fine. We have coupling issues in the backend.
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.
Attachment #760359 - Flags: review?(mark.finkle) → feedback+
(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.
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
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()?
Attachment #760359 - Flags: review?(wjohnston) → review+
After consultation with bwalker, replacing the inefficient code should be tracked in an Android specific bug. Please see bug 883121.
Corrected nits from wesj, and #ifdef from fabrice.
Attachment #760359 - Attachment is obsolete: true
Attachment #762781 - Flags: review?(fabrice)
Attachment #762781 - Attachment is patch: true
Attachment #762781 - Attachment mime type: text/x-patch → text/plain
Attachment #762781 - Flags: review?(fabrice) → review+
> // 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.
We're trying to get the desktop tests for this (at least, I hope this is covered in there...) running in bug 873567.
Looks like the basic mochitest smoke test of this feature failed.

dom/tests/browser/browser_webapps_permissions.js
Attachment #766966 - Flags: review?(mark.finkle)
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
Attachment #766969 - Flags: review?(fabrice)
Attachment #766966 - Flags: review?(mark.finkle) → review+
Comment on attachment 766969 [details] [diff] [review]
Patch 3/3 - Disable test

Trying to move this forward.
Attachment #766969 - Flags: review?(fabrice) → review?(mark.finkle)
Comment on attachment 766969 [details] [diff] [review]
Patch 3/3 - Disable test

Given Wes' explanation, this patch makes sense.
Attachment #766969 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/d1fd89491708
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Depends on: 889123
See bug 889123 for a followup. This just broke a workflow for Gaia development on Firefox OS.
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?
(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
You need to log in before you can comment on or make changes to this bug.