Closed Bug 787439 Opened 7 years ago Closed 7 years ago

Ensure applications permissions are updated when doing a system update

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

(Whiteboard: [WebAPI:P1][LOE:S])

Attachments

(2 files, 6 obsolete files)

No description provided.
blocking-basecamp: --- → ?
Definitely a blocker.

Is this something you'll do, Fabrice?
Assignee: nobody → fabrice
blocking-basecamp: ? → +
(In reply to Andrew Overholt [:overholt] from comment #1)
> Definitely a blocker.
> 
> Is this something you'll do, Fabrice?

Yes.
Whiteboard: [WebAPI:P1]
Whiteboard: [WebAPI:P1] → [WebAPI:P1][LOE:S]
Depends on: 758269
Here's the way we'll do it:

* The app registry is located on the data partition and starts out
empty when the phone is first shipped.
* On first startup (first ever, first after update, and after a
factory reset) we go through the list of apps in the system partition
and go through an "install step" for each of them.
* The "install step" looks through the app registry looking for an app
with the same package path. If one exists we get the app id from the
registry. If it doesn't exist we add the app to the app registry and
generate a new app id.
* The "install step" also calls the installPermissions function added
in bug 758269. This will ensure that the permission database is fully
updated for both added and removed permissions.
* It also enumerates all apps which already existed in the app
registry and whose package path points to the system directy. If there
no longer is a package at that path we go through an uninstall step to
remove all data (this isn't technically something we need to do for v1
since this code will only be needed in v2).

This way the identifying property of a system app is its name in
/system/b2g/webapps. When we update gaia we just have to ensure that
the filename for the package remains the same. That will ensure that
it still gets the same app id.
Attached patch wip patch (obsolete) — Splinter Review
wip (because I can't hook up yet the permission management operations).

Works fine for me on destkop and device (tested in "user" variant).
Attachment #661426 - Flags: feedback?(marshall)
Marshall, if this patch does not apply on your tree, take the one in bug 777204 before.
Attached patch wip v2 (obsolete) — Splinter Review
This patch makes sure we fire webapps-registry-ready only after all the activities registered from the manifests are actually registered. That fixes a race I was still seeing sometimes on debug desktop builds.
Attachment #661426 - Attachment is obsolete: true
Attachment #661426 - Flags: feedback?(marshall)
Attachment #661969 - Flags: feedback?(marshall)
Comment on attachment 661969 [details] [diff] [review]
wip v2

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

Looks good. We will need to ensure that dom.mozApps.runUpdate gets reset to true after an update. Can you file a follow up for that? Also, there is a lot of deeply nested code in this patch. Can we break it up a bit? :)
Attachment #661969 - Flags: feedback?(marshall) → feedback+
https://hg.mozilla.org/mozilla-central/rev/7276747d5260
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to :Ms2ger from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/7276747d5260

Hu, why did you push this?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 786293
Attached patch Gecko Patch (obsolete) — Splinter Review
This patch ensures that we set apps permissions at first run, both on desktop builds and on device builds (in user and eng variants).

The missing piece here is that the "runUpdate" check is not working as it should. A gecko+gaia update will not reset the pref used, only a factory reset. Not sure what to use there instead...
Attachment #661969 - Attachment is obsolete: true
Attached patch Gaia Patch (obsolete) — Splinter Review
This patch updates all the manifests to the correct permission syntax, and remove the script that prebuilds permissions.
When writing this patches I found several cases of mismatch between permissions in the manifests and what we have in PermissionInstaller.jsm. I added and/or modified PermissionInstaller.jsm to match what the actual implementation of the APIs needed, but it looks there's some general cleanup to be done around that.
Yeah, the actual permissions in both our code and manifests are terribly chaotic right now.

Doing what you're doing and then doing followups to clean this stuff up seems like the right approach.

However please use DENY_ACTION for 'privileged' apps for now. That is more often than not the right value. And it's a lot better if we accidentally use a too restrictive policy than a too permissive one. Both because it's a lot easier to detect, and because the result to the user is a lot better.
Attached patch Gecko patch v2 (obsolete) — Splinter Review
I changed the new permissions to be DENY_ACTION for privileged apps, and reused code from fx desktop to check that we need to run the permission updates. Works well for me on dekstop and both device variants.

Marshall, can you test on your side and review (you'll need the gaia patch also) ?
Attachment #667739 - Attachment is obsolete: true
Attachment #668184 - Flags: review?(marshall)
Attached patch Gaia Patch v2Splinter Review
I forgot to update manifest of some test_apps in the first patch.
Attachment #667742 - Attachment is obsolete: true
Attachment #668185 - Flags: review?(21)
Comment on attachment 668184 [details] [diff] [review]
Gecko patch v2

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

Just some preliminary feedback while I'm building and digesting this..

::: dom/apps/src/Webapps.jsm
@@ +160,5 @@
> +   *  OVERRIDE_NEW_BUILD_ID if this is the first run with a new build ID of the
> +   *                        same Gecko milestone (i.e. after a nightly upgrade).
> +   *  OVERRIDE_NONE otherwise.
> +   */
> +  needHomepageOverride: function(prefb) {

hrm, the logic looks good here, but I'm not a huge fan of the "homepage override" nomenclature, or the browser pref namespace. What we really care about here is whether we've recently been updated to a new milestone or buildID right? If that's the case, we should probably name the function and prefs that way. 

Would you object to moving this logic into UpdatePrompt.js, and invoking it whenever there's a successful update?
Attached patch Gecko patch v3 (obsolete) — Splinter Review
I moved the isFirstRun() function in AppsUtils.jsm to make it reusable, and simplified it a bit to just return a boolean.
Attachment #668184 - Attachment is obsolete: true
Attachment #668184 - Flags: review?(marshall)
Attachment #668647 - Flags: review?(marshall)
Comment on attachment 668647 [details] [diff] [review]
Gecko patch v3

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

This looks good, but I haven't tested it yet. I'll add another comment once I get a chance today. You might also want to mark someone else for r? on PermissionsInstaller code, as this is the first time I've seen it :)

::: dom/apps/src/AppsUtils.jsm
@@ +219,5 @@
> +
> +    aPrefBranch.setCharPref("gecko.mstone", mstone);
> +    aPrefBranch.setCharPref("gecko.buildID", buildID);
> +
> +    return ((mstone != savedmstone) || (buildID != savedBuildID));

Hrm, I wonder if it makes sense to cache the return value, so the function can be reused elsewhere. Something like..

function isFirstRun(aPrefBranch) {
  if (this._isFirstRun && aPrefBranch in this._isFirstRun) {
    return this._isFirstRun[aPrefBranch];
  }

  // .. query and set mstone / buildID prefs

  if (!this._isFirstRun) {
    this._isFirstRun = {};
  }

  this._isFirstRun[aPrefBranch] = (mstone != savedmstone) || (buildID != savedBuildID);
  return this._isFirstRun[aPrefBranch];
}

::: dom/apps/src/PermissionsInstaller.jsm
@@ +360,5 @@
>                // TODO: use PermSettings.remove, see bug 793204
> +              let list = [AllPossiblePermissions[idx]];
> +              if (AllPossiblePermissions[idx] == "storage") {
> +                list.concat(["indexedDB-unlimited", "offline-app", "pin-app"]);
> +              }

The logic permission logic for "storage" is being reused below on line 403, should we separate it out into a function?
Attachment #668647 - Flags: review?(marshall) → review+
Testing this was a bit of a pain -- it turns out that platformBuildID only gets updated when toolkit/xre/nsAppRunner.cpp is rebuilt (which usually doesn't happen unless you're clobbering).

My test steps:
- ./build.sh && ./flash.sh -- full build / flash
- ensure dialing a phone number works with the dialer
- remove the "telephony" permission from the communications app in gaia/apps/communications/manifest.webapp
- touch gecko/toolkit/xre/nsAppRunner.cpp
- ./build.sh
- ./flash.sh gaia; ./flash.sh gecko -- it's important that you don't do a full ./flash.sh here. we want to reuse the profile /data partition
- go to the dialer, and notice that dialing phone numbers no longer works..
Comment on attachment 668185 [details] [diff] [review]
Gaia Patch v2

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

::: Android.mk
@@ -46,4 @@
>  	rm -rf $(GAIA_PATH)/profile $(GAIA_PATH)/profile.tar.gz
>  endif
>  	$(MAKE) $(GAIA_MAKE_FLAGS) profile
> -	cd $(GAIA_PATH)/profile && tar cfz $(abspath $@) indexedDB webapps permissions.sqlite

Do you really want to remove indexedDB too?
Attachment #668185 - Flags: review?(21) → review+
(In reply to Vivien Nicolas (:vingtetun) from comment #20)
> Comment on attachment 668185 [details] [diff] [review]
> Gaia Patch v2
> 
> Review of attachment 668185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Android.mk
> @@ -46,4 @@
> >  	rm -rf $(GAIA_PATH)/profile $(GAIA_PATH)/profile.tar.gz
> >  endif
> >  	$(MAKE) $(GAIA_MAKE_FLAGS) profile
> > -	cd $(GAIA_PATH)/profile && tar cfz $(abspath $@) indexedDB webapps permissions.sqlite
> 
> Do you really want to remove indexedDB too?

hm, not yet. This is a bad merge from another PR to remove the settings db.
Attached patch Gecko patch v4Splinter Review
David, can you take a look at the changes in PermissionInstaller.jsm ?
Attachment #668647 - Attachment is obsolete: true
Attachment #669349 - Flags: review?(ddahl)
Comment on attachment 669349 [details] [diff] [review]
Gecko patch v4

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

This looks good are there already tests somewhere that verify the "first run" bits in here? Also, this reminds me that we need to debug the "resinstall app" tests in bug 794920

::: dom/apps/src/AppsUtils.jsm
@@ +202,5 @@
> +  /**
> +   * Determines if an update or a factory reset occured.
> +   */
> +  isFirstRun: function isFirstRun(aPrefBranch) {
> +    let savedmstone = null;

Just curious, what does '*mstone' mean?

::: dom/apps/src/PermissionsInstaller.jsm
@@ +70,5 @@
>                               app: DENY_ACTION,
>                               privileged: PROMPT_ACTION,
>                               certified: ALLOW_ACTION
>                             },
> +                           alarms: {

We must file a bug to update the tests in bug 758269, which uses a manifest with "alarm"

@@ +89,5 @@
>                             contacts: {
>                               app: DENY_ACTION,
>                               privileged: PROMPT_ACTION,
>                               certified: ALLOW_ACTION,
> +                             /*access: ["read",

Here as well - bug 758269 expects contacts-read, etc - or, is this not permanent?
Attachment #669349 - Flags: review?(ddahl) → review+
Comment on attachment 668185 [details] [diff] [review]
Gaia Patch v2

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

I encounter an installation error when I apply this patch on user build.
There is no profile/permission.sqlite and adb push will fail eventually.
Should we remove it from install-gaia.py also?
https://github.com/mozilla-b2g/gaia/blob/master/build/install-gaia.py#L148
(In reply to Shih-Chiang Chien [:schien] from comment #24)

> I encounter an installation error when I apply this patch on user build.
> There is no profile/permission.sqlite and adb push will fail eventually.
> Should we remove it from install-gaia.py also?
> https://github.com/mozilla-b2g/gaia/blob/master/build/install-gaia.py#L148

yes, good catch!
Remember to land this on aurora too.
Gaia PR to be merged when the builds are ready: https://github.com/mozilla-b2g/gaia/pull/5752
https://hg.mozilla.org/mozilla-central/rev/06c8b4cd2f9e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
QA Contact: jsmith
QA Contact: jsmith
Keywords: verifyme
QA Contact: jsmith
You need to log in before you can comment on or make changes to this bug.