787439 - Ensure applications permissions are updated when doing a system update

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Andrew Overholt [:overholt]]

Definitely a blocker.

Is this something you'll do, Fabrice?

Comment 2

[[:fabrice] Fabrice Desré]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Definitely a blocker.
> 
> Is this something you'll do, Fabrice?

Yes.

Comment 3

[[:fabrice] Fabrice Desré]

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.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: wip patch

wip (because I can't hook up yet the permission management operations).

Works fine for me on destkop and device (tested in "user" variant).

Comment 5

[[:fabrice] Fabrice Desré]

Marshall, if this patch does not apply on your tree, take the one in bug 777204 before.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: wip v2

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.

Comment 7

[Marshall Culpepper [:marshall_law]]

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? :)

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7276747d5260

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to :Ms2ger from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/7276747d5260

Hu, why did you push this?

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: Gecko Patch

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

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: Gaia Patch

This patch updates all the manifests to the correct permission syntax, and remove the script that prebuilds permissions.

Comment 12

[[:fabrice] Fabrice Desré]

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.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: Gecko patch v2

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

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: Gaia Patch v2

I forgot to update manifest of some test_apps in the first patch.

Comment 16

[Marshall Culpepper [:marshall_law]]

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?

Comment 17

[[:fabrice] Fabrice Desré]

Attachment: Gecko patch v3

I moved the isFirstRun() function in AppsUtils.jsm to make it reusable, and simplified it a bit to just return a boolean.

Comment 18

[Marshall Culpepper [:marshall_law]]

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?

Comment 19

[Marshall Culpepper [:marshall_law]]

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 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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?

Comment 21

[[:fabrice] Fabrice Desré]

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

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: Gecko patch v4

David, can you take a look at the changes in PermissionInstaller.jsm ?

Comment 23

[David Dahl :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?

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 25

[[:fabrice] Fabrice Desré]

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

Comment 26

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06c8b4cd2f9e

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Remember to land this on aurora too.

Comment 28

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8048cd69f71d

Comment 29

[[:fabrice] Fabrice Desré]

Gaia PR to be merged when the builds are ready: https://github.com/mozilla-b2g/gaia/pull/5752

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/06c8b4cd2f9e