Bug 1618500 handle permissions and preferences changes on addon update r=aswan
authorShane Caraveo <scaraveo@mozilla.com>
Sun, 08 Mar 2020 19:29:55 +0000
changeset 517480 cfca069fc4b2fd4c6ab406fb5b513ffab3c2e07e
parent 517479 671bdfb7cb9fc97ec83961c86b9e10683ad04e65
child 517481 13400b21b6c37818142d4766f1545864b487598a
push id37194
push usershindli@mozilla.com
push dateMon, 09 Mar 2020 03:45:29 +0000
treeherdermozilla-central@2540a369a5a8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1618500
milestone75.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1618500 handle permissions and preferences changes on addon update r=aswan If an addon is updated and moves permissions between required to optional, we want to retain the previously granted permission so the extension does not have to request the permission from the user again. We also handle permission removal and changes to preferences based on permissions. Differential Revision: https://phabricator.services.mozilla.com/D64696
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionPreferencesManager.jsm
toolkit/components/extensions/test/xpcshell/test_ext_permissions_api.js
toolkit/components/extensions/test/xpcshell/test_ext_permissions_migrate.js
toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
toolkit/mozapps/extensions/internal/XPIDatabase.jsm
toolkit/mozapps/extensions/internal/XPIInstall.jsm
toolkit/mozapps/extensions/internal/XPIProvider.jsm
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -47,16 +47,18 @@ XPCOMUtils.defineLazyModuleGetters(this,
   AddonManager: "resource://gre/modules/AddonManager.jsm",
   AddonManagerPrivate: "resource://gre/modules/AddonManager.jsm",
   AddonSettings: "resource://gre/modules/addons/AddonSettings.jsm",
   AMTelemetry: "resource://gre/modules/AddonManager.jsm",
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   E10SUtils: "resource://gre/modules/E10SUtils.jsm",
   ExtensionPermissions: "resource://gre/modules/ExtensionPermissions.jsm",
+  ExtensionPreferencesManager:
+    "resource://gre/modules/ExtensionPreferencesManager.jsm",
   ExtensionProcessScript: "resource://gre/modules/ExtensionProcessScript.jsm",
   ExtensionStorage: "resource://gre/modules/ExtensionStorage.jsm",
   ExtensionStorageIDB: "resource://gre/modules/ExtensionStorageIDB.jsm",
   ExtensionTelemetry: "resource://gre/modules/ExtensionTelemetry.jsm",
   FileSource: "resource://gre/modules/L10nRegistry.jsm",
   L10nRegistry: "resource://gre/modules/L10nRegistry.jsm",
   LightweightThemeManager: "resource://gre/modules/LightweightThemeManager.jsm",
   Localization: "resource://gre/modules/Localization.jsm",
@@ -578,38 +580,55 @@ class ExtensionData {
   get restrictSchemes() {
     // ExtensionData can't check the signature (as it is not yet passed to its constructor
     // as it is for the Extension class, where this getter is overridden to check both the
     // signature and the permissions).
     return !this.hasPermission("mozillaAddons");
   }
 
   /**
+   * Given an array of host and permissions, generate a structured permissions object
+   * that contains seperate host origins and permissions arrays.
+   *
+   * @param {Array} permissionsArray
+   * @returns {Object} permissions object
+   */
+  permissionsObject(permissionsArray) {
+    let permissions = new Set();
+    let origins = new Set();
+    let { restrictSchemes, isPrivileged } = this;
+    for (let perm of permissionsArray || []) {
+      let type = classifyPermission(perm, restrictSchemes, isPrivileged);
+      if (type.origin) {
+        origins.add(perm);
+      } else if (type.permission) {
+        permissions.add(perm);
+      }
+    }
+    return {
+      permissions,
+      origins,
+    };
+  }
+
+  /**
    * Returns an object representing any capabilities that the extension
    * has access to based on fixed properties in the manifest.  The result
    * includes the contents of the "permissions" property as well as other
    * capabilities that are derived from manifest fields that users should
    * be informed of (e.g., origins where content scripts are injected).
    */
   get manifestPermissions() {
     if (this.type !== "extension") {
       return null;
     }
 
-    let permissions = new Set();
-    let origins = new Set();
-    let { restrictSchemes, isPrivileged } = this;
-    for (let perm of this.manifest.permissions || []) {
-      let type = classifyPermission(perm, restrictSchemes, isPrivileged);
-      if (type.origin) {
-        origins.add(perm);
-      } else if (type.permission) {
-        permissions.add(perm);
-      }
-    }
+    let { permissions, origins } = this.permissionsObject(
+      this.manifest.permissions
+    );
 
     if (this.manifest.devtools_page) {
       permissions.add("devtools");
     }
 
     for (let entry of this.manifest.content_scripts || []) {
       for (let origin of entry.matches) {
         origins.add(origin);
@@ -617,16 +636,26 @@ class ExtensionData {
     }
 
     return {
       permissions: Array.from(permissions),
       origins: Array.from(origins),
     };
   }
 
+  get manifestOptionalPermissions() {
+    let { permissions, origins } = this.permissionsObject(
+      this.manifest.optional_permissions
+    );
+    return {
+      permissions: Array.from(permissions),
+      origins: Array.from(origins),
+    };
+  }
+
   /**
    * Returns an object representing all capabilities this extension has
    * access to, including fixed ones from the manifest as well as dynamically
    * granted permissions.
    */
   get activePermissions() {
     if (this.type !== "extension") {
       return null;
@@ -663,16 +692,84 @@ class ExtensionData {
           )
       ),
       permissions: newPermissions.permissions.filter(
         perm => !oldPermissions.permissions.includes(perm)
       ),
     };
   }
 
+  // Return those permissions in oldPermissions that also exist in newPermissions.
+  static intersectPermissions(oldPermissions, newPermissions) {
+    let matcher = new MatchPatternSet(newPermissions.origins, {
+      restrictSchemes: false,
+    });
+
+    return {
+      origins: oldPermissions.origins.filter(perm =>
+        matcher.subsumesDomain(
+          new MatchPattern(perm, { restrictSchemes: false })
+        )
+      ),
+      permissions: oldPermissions.permissions.filter(perm =>
+        newPermissions.permissions.includes(perm)
+      ),
+    };
+  }
+
+  /**
+   * When updating the addon, find and migrate permissions that have moved from required
+   * to optional.  This also handles any updates required for permission removal.
+   *
+   * @param {string} id The id of the addon being updated
+   * @param {Object} oldPermissions
+   * @param {Object} oldOptionalPermissions
+   * @param {Object} newPermissions
+   * @param {Object} newOptionalPermissions
+   */
+  static async migratePermissions(
+    id,
+    oldPermissions,
+    oldOptionalPermissions,
+    newPermissions,
+    newOptionalPermissions
+  ) {
+    let migrated = ExtensionData.intersectPermissions(
+      oldPermissions,
+      newOptionalPermissions
+    );
+    // If a permission is optional in this version and was mandatory in the previous
+    // version, it was already accepted by the user at install time so add it to the
+    // list of granted optional permissions now.
+    await ExtensionPermissions.add(id, migrated);
+
+    // Now we need to update ExtensionPreferencesManager, removing any settings
+    // for old permissions that no longer exist.
+    let permSet = new Set(
+      newPermissions.permissions.concat(newOptionalPermissions.permissions)
+    );
+    let oldPerms = oldPermissions.permissions.concat(
+      oldOptionalPermissions.permissions
+    );
+
+    let removed = oldPerms.filter(x => !permSet.has(x));
+    if (removed.length) {
+      await Management.asyncLoadSettingsModules();
+      for (let name of removed) {
+        await ExtensionPreferencesManager.removeSettingsForPermission(id, name);
+      }
+    }
+
+    // Remove any optional permissions that have been removed from the manifest.
+    await ExtensionPermissions.remove(id, {
+      permissions: removed,
+      origins: [],
+    });
+  }
+
   canUseExperiment(manifest) {
     return this.experimentsAllowed && manifest.experiment_apis;
   }
 
   /**
    * Load a locale and return a localized manifest.  The extension must
    * be initialized, and manifest parsed prior to calling.
    *
@@ -1536,17 +1633,26 @@ class BootstrapScope {
 
   fetchState() {
     if (this.extension) {
       return { state: this.extension.state };
     }
     return null;
   }
 
-  update(data, reason) {
+  async update(data, reason) {
+    // Retain any previously granted permissions that may have migrated into the optional list.
+    await ExtensionData.migratePermissions(
+      data.id,
+      data.oldPermissions,
+      data.oldOptionalPermissions,
+      data.userPermissions,
+      data.optionalPermissions
+    );
+
     return Management.emit("update", {
       id: data.id,
       resourceURI: data.resourceURI,
     });
   }
 
   startup(data, reason) {
     // eslint-disable-next-line no-use-before-define
--- a/toolkit/components/extensions/ExtensionPreferencesManager.jsm
+++ b/toolkit/components/extensions/ExtensionPreferencesManager.jsm
@@ -410,23 +410,26 @@ this.ExtensionPreferencesManager = {
     await Promise.all(removePromises);
   },
 
   /**
    * Removes a set of settings that are available under certain addon permissions.
    *
    * @param {string} id           The extension id.
    * @param {string} permission   The permission name from the extension manifest.
+   * @returns {Promise}           A promise that resolves when all related settings are removed.
    */
-  async removeSettingsForPermission(id, permission) {
+  removeSettingsForPermission(id, permission) {
+    let removePromises = [];
     settingsMap.forEach((setting, name) => {
       if (setting.permission == permission) {
-        this.removeSetting(id, name);
+        removePromises.push(this.removeSetting(id, name));
       }
     });
+    return Promise.all(removePromises);
   },
 
   /**
    * Return the currently active value for a setting.
    *
    * @param {string} name
    *        The unique id of the setting.
    *
--- a/toolkit/components/extensions/test/xpcshell/test_ext_permissions_api.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_permissions_api.js
@@ -84,16 +84,17 @@ add_task(async function test_geo_permiss
       let result = await browser.permissions.contains(permObj);
       browser.test.sendMessage("done", result);
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background,
     manifest: {
+      applications: { gecko: { id: "geo-test@test" } },
       optional_permissions: ["geolocation"],
     },
     useAddonManager: "permanent",
   });
   await extension.startup();
 
   let policy = WebExtensionPolicy.getByID(extension.id);
   let principal = policy.extension.principal;
@@ -115,17 +116,41 @@ add_task(async function test_geo_permiss
     extension.sendMessage("remove");
     ok(!(await extension.awaitMessage("done")), "permission revoked");
 
     equal(
       Services.perms.testPermissionFromPrincipal(principal, "geo"),
       Services.perms.UNKNOWN_ACTION,
       "geolocation not allowed after removed"
     );
+
+    // re-grant to test update removal
+    extension.sendMessage("request");
+    ok(await extension.awaitMessage("done"), "permission granted");
+    equal(
+      Services.perms.testPermissionFromPrincipal(principal, "geo"),
+      Services.perms.ALLOW_ACTION,
+      "geolocation allowed after re-requested"
+    );
   });
+
+  // We should not have geo permission after this upgrade.
+  await extension.upgrade({
+    manifest: {
+      applications: { gecko: { id: "geo-test@test" } },
+    },
+    useAddonManager: "permanent",
+  });
+
+  equal(
+    Services.perms.testPermissionFromPrincipal(principal, "geo"),
+    Services.perms.UNKNOWN_ACTION,
+    "geolocation not allowed after upgrade"
+  );
+
   await extension.unload();
 });
 
 add_task(async function test_browserSetting_permissions() {
   async function background() {
     const permObj = { permissions: ["browserSettings"] };
     browser.test.onMessage.addListener(async msg => {
       if (msg === "request") {
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_permissions_migrate.js
@@ -0,0 +1,180 @@
+"use strict";
+
+AddonTestUtils.init(this);
+AddonTestUtils.overrideCertDB();
+AddonTestUtils.createAppInfo(
+  "xpcshell@tests.mozilla.org",
+  "XPCShell",
+  "1",
+  "42"
+);
+
+add_task(async function setup() {
+  Services.prefs.setBoolPref(
+    "extensions.webextOptionalPermissionPrompts",
+    false
+  );
+  registerCleanupFunction(() => {
+    Services.prefs.clearUserPref("extensions.webextOptionalPermissionPrompts");
+  });
+  await AddonTestUtils.promiseStartupManager();
+  AddonTestUtils.usePrivilegedSignatures = false;
+});
+
+add_task(async function test_migrated_permission_to_optional() {
+  let id = "permission-upgrade@test";
+  let extensionData = {
+    manifest: {
+      version: "1.0",
+      applications: { gecko: { id } },
+      permissions: [
+        "webRequest",
+        "tabs",
+        "http://example.net/*",
+        "http://example.com/*",
+      ],
+    },
+    useAddonManager: "permanent",
+  };
+
+  function checkPermissions() {
+    let policy = WebExtensionPolicy.getByID(id);
+    ok(policy.hasPermission("webRequest"), "addon has webRequest permission");
+    ok(policy.hasPermission("tabs"), "addon has tabs permission");
+    ok(
+      policy.canAccessURI(Services.io.newURI("http://example.net/")),
+      "addon has example.net host permission"
+    );
+    ok(
+      policy.canAccessURI(Services.io.newURI("http://example.com/")),
+      "addon has example.com host permission"
+    );
+    ok(
+      !policy.canAccessURI(Services.io.newURI("http://other.com/")),
+      "addon does not have other.com host permission"
+    );
+  }
+
+  let extension = ExtensionTestUtils.loadExtension(extensionData);
+  await extension.startup();
+  checkPermissions();
+
+  // Move to using optional permission
+  extensionData.manifest.version = "2.0";
+  extensionData.manifest.permissions = ["tabs", "http://example.net/*"];
+  extensionData.manifest.optional_permissions = [
+    "webRequest",
+    "http://example.com/*",
+    "http://other.com/*",
+  ];
+
+  await extension.upgrade(extensionData);
+
+  equal(extension.version, "2.0", "Expected extension version");
+  checkPermissions();
+
+  await extension.unload();
+});
+
+// This tests that settings are removed if a required permission is removed.
+// We use two settings APIs to make sure the one we keep permission to is not
+// removed inadvertantly.
+add_task(async function test_required_permissions_removed() {
+  function cacheIsEnabled() {
+    return (
+      Services.prefs.getBoolPref("browser.cache.disk.enable") &&
+      Services.prefs.getBoolPref("browser.cache.memory.enable")
+    );
+  }
+
+  let extData = {
+    background() {
+      if (browser.browserSettings) {
+        browser.browserSettings.cacheEnabled.set({ value: false });
+      }
+      browser.privacy.services.passwordSavingEnabled.set({ value: false });
+    },
+    manifest: {
+      applications: { gecko: { id: "pref-test@test" } },
+      permissions: ["tabs", "browserSettings", "privacy", "http://test.com/*"],
+    },
+    useAddonManager: "permanent",
+  };
+  let extension = ExtensionTestUtils.loadExtension(extData);
+  ok(
+    Services.prefs.getBoolPref("signon.rememberSignons"),
+    "privacy setting intial value as expected"
+  );
+  await extension.startup();
+  ok(!cacheIsEnabled(), "setting is set after startup");
+
+  extData.manifest.permissions = ["tabs"];
+  extData.manifest.optional_permissions = ["privacy"];
+  await extension.upgrade(extData);
+  ok(cacheIsEnabled(), "setting is reset after upgrade");
+  ok(
+    !Services.prefs.getBoolPref("signon.rememberSignons"),
+    "privacy setting is still set after upgrade"
+  );
+
+  await extension.unload();
+});
+
+// This tests that settings are removed if a granted permission is removed.
+// We use two settings APIs to make sure the one we keep permission to is not
+// removed inadvertantly.
+add_task(async function test_granted_permissions_removed() {
+  function cacheIsEnabled() {
+    return (
+      Services.prefs.getBoolPref("browser.cache.disk.enable") &&
+      Services.prefs.getBoolPref("browser.cache.memory.enable")
+    );
+  }
+
+  let extData = {
+    async background() {
+      browser.test.onMessage.addListener(async msg => {
+        await browser.permissions.request({ permissions: msg.permissions });
+        if (browser.browserSettings) {
+          browser.browserSettings.cacheEnabled.set({ value: false });
+        }
+        browser.privacy.services.passwordSavingEnabled.set({ value: false });
+        browser.test.sendMessage("done");
+      });
+    },
+    // "tabs" is never granted, it is included to exercise the removal code
+    // that called during the upgrade.
+    manifest: {
+      applications: { gecko: { id: "pref-test@test" } },
+      optional_permissions: [
+        "tabs",
+        "browserSettings",
+        "privacy",
+        "http://test.com/*",
+      ],
+    },
+    useAddonManager: "permanent",
+  };
+  let extension = ExtensionTestUtils.loadExtension(extData);
+  ok(
+    Services.prefs.getBoolPref("signon.rememberSignons"),
+    "privacy setting intial value as expected"
+  );
+  await extension.startup();
+  await withHandlingUserInput(extension, async () => {
+    extension.sendMessage({ permissions: ["browserSettings", "privacy"] });
+    await extension.awaitMessage("done");
+  });
+  ok(!cacheIsEnabled(), "setting is set after startup");
+
+  extData.manifest.permissions = ["privacy"];
+  delete extData.manifest.optional_permissions;
+  await extension.upgrade(extData);
+  ok(cacheIsEnabled(), "setting is reset after upgrade");
+  ok(
+    !Services.prefs.getBoolPref("signon.rememberSignons"),
+    "privacy setting is still set after upgrade"
+  );
+
+  await extension.unload();
+});
--- a/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
@@ -191,16 +191,18 @@ skip-if = appname == "thunderbird"
 [test_ext_xhr_capabilities.js]
 [test_native_manifests.js]
 subprocess = true
 skip-if = os == "android"
 [test_ext_permissions.js]
 skip-if = appname == "thunderbird" || os == "android" # Bug 1350559
 [test_ext_permissions_api.js]
 skip-if = appname == "thunderbird" || os == "android" # Bug 1350559
+[test_ext_permissions_migrate.js]
+skip-if = appname == "thunderbird" || os == "android" # Bug 1350559
 [test_ext_permissions_uninstall.js]
 skip-if = appname == "thunderbird" || os == "android" # Bug 1350559
 [test_proxy_listener.js]
 [test_proxy_incognito.js]
 skip-if = os == "android" # incognito not supported on android
 [test_proxy_info_results.js]
 [test_proxy_userContextId.js]
 [test_webRequest_ancestors.js]
--- a/toolkit/mozapps/extensions/internal/XPIDatabase.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIDatabase.jsm
@@ -145,16 +145,17 @@ const PROP_JSON_FIELDS = [
   "locales",
   "targetApplications",
   "targetPlatforms",
   "signedState",
   "seen",
   "dependencies",
   "incognito",
   "userPermissions",
+  "optionalPermissions",
   "icons",
   "iconURL",
   "blocklistState",
   "blocklistURL",
   "startupData",
   "previewImage",
   "hidden",
   "installTelemetryInfo",
@@ -1195,16 +1196,20 @@ AddonWrapper = class {
     let addon = addonFor(this);
     return addon.location.name == KEY_APP_PROFILE;
   }
 
   get userPermissions() {
     return addonFor(this).userPermissions;
   }
 
+  get optionalPermissions() {
+    return addonFor(this).optionalPermissions;
+  }
+
   isCompatibleWith(aAppVersion, aPlatformVersion) {
     return addonFor(this).isCompatibleWith(aAppVersion, aPlatformVersion);
   }
 
   async uninstall(alwaysAllowUndo) {
     let addon = addonFor(this);
     return XPIInstall.uninstallAddon(addon, alwaysAllowUndo);
   }
--- a/toolkit/mozapps/extensions/internal/XPIInstall.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIInstall.jsm
@@ -518,17 +518,17 @@ async function loadManifestFromWebManife
 
     addon.optionsBrowserStyle = manifest.options_ui.browser_style;
   }
 
   // WebExtensions don't use iconURLs
   addon.iconURL = null;
   addon.icons = manifest.icons || {};
   addon.userPermissions = extension.manifestPermissions;
-
+  addon.optionalPermissions = extension.manifestOptionalPermissions;
   addon.applyBackgroundUpdates = AddonManager.AUTOUPDATE_DEFAULT;
 
   function getLocale(aLocale) {
     // Use the raw manifest, here, since we need values with their
     // localization placeholders still in place.
     let rawManifest = extension.rawManifest;
 
     // As a convenience, allow author to be set if its a string bug 1313567.
@@ -3707,16 +3707,17 @@ var XPIInstall = {
   // An array of currently active AddonInstalls
   installs: new Set(),
 
   createLocalInstall,
   flushJarCache,
   newVersionReason,
   recursiveRemove,
   syncLoadManifest,
+  loadManifestFromFile,
 
   // Keep track of in-progress operations that support cancel()
   _inProgress: [],
 
   doing(aCancellable) {
     this._inProgress.push(aCancellable);
   },
 
--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -121,17 +121,17 @@ const STARTUP_MTIME_SCOPES = [
   KEY_APP_SYSTEM_USER,
 ];
 
 const NOTIFICATION_FLUSH_PERMISSIONS = "flush-pending-permissions";
 const XPI_PERMISSION = "install";
 
 const XPI_SIGNATURE_CHECK_PERIOD = 24 * 60 * 60;
 
-const DB_SCHEMA = 31;
+const DB_SCHEMA = 32;
 
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "enabledScopesPref",
   PREF_EM_ENABLED_SCOPES,
   AddonManager.SCOPE_ALL
 );
 
@@ -2037,16 +2037,20 @@ class BootstrapScope {
   async update(newAddon, startup = false, updateCallback) {
     let reason = XPIInstall.newVersionReason(
       this.addon.version,
       newAddon.version
     );
     let extraArgs = {
       oldVersion: this.addon.version,
       newVersion: newAddon.version,
+      userPermissions: newAddon.userPermissions,
+      optionalPermissions: newAddon.optionalPermissions,
+      oldPermissions: this.addon.userPermissions,
+      oldOptionalPermissions: this.addon.optionalPermissions,
     };
 
     let callUpdate = this.addon.isWebExtension && newAddon.isWebExtension;
 
     await this._uninstall(reason, callUpdate, extraArgs);
 
     if (updateCallback) {
       await updateCallback();
@@ -2646,20 +2650,26 @@ var XPIProvider = {
 
       let cleanup = () => {
         tempLocation.installer.uninstallAddon(id);
         tempLocation.removeAddon(id);
       };
 
       let promise;
       if (existing) {
-        promise = bootstrap.update(existing, false, () => {
-          cleanup();
-          XPIDatabase.makeAddonLocationVisible(id, existing.location);
-        });
+        // We need manifest data before calling update.
+        promise = XPIInstall.loadManifestFromFile(
+          existing.file,
+          existing.location
+        ).then(existingAddon =>
+          bootstrap.update(existingAddon, false, () => {
+            cleanup();
+            XPIDatabase.makeAddonLocationVisible(id, existing.location);
+          })
+        );
       } else {
         promise = bootstrap.uninstall().then(cleanup);
       }
       AsyncShutdown.profileChangeTeardown.addBlocker(
         `Temporary extension shutdown: ${addon.id}`,
         promise
       );
       promises.push(promise);