Bug 1444294 implement browser.permissions onAdded/Removed r=robwu

Differential Revision: https://phabricator.services.mozilla.com/D71231

toolkit/components/extensions/Extension.jsm

@@ -766,12 +766,11 @@ class ExtensionData {
     );
 
     let removed = oldPerms.filter(x => !permSet.has(x));
-    if (removed.length) {
-      await Management.asyncLoadSettingsModules();
-      for (let name of removed) {
-        await ExtensionPreferencesManager.removeSettingsForPermission(id, name);
-      }
-    }
+    // Force the removal here to ensure the settings are removed prior
+    // to startup.  This will remove both required or optional permissions,
+    // whereas the call from within ExtensionPermissions would only result
+    // in a removal for optional permissions that were removed.
+    await ExtensionPreferencesManager.removeSettingsForPermissions(id, removed);
 
     // Remove any optional permissions that have been removed from the manifest.
     await ExtensionPermissions.remove(id, {

toolkit/components/extensions/ExtensionPermissions.jsm

@@ -21,6 +21,12 @@ XPCOMUtils.defineLazyGetter(
   () => ExtensionParent.StartupCache
 );
 
+XPCOMUtils.defineLazyGetter(
+  this,
+  "Management",
+  () => ExtensionParent.apiManager
+);
+
 var EXPORTED_SYMBOLS = ["ExtensionPermissions"];
 
 const FILE_NAME = "extension-preferences.json";
@@ -123,6 +129,7 @@ var ExtensionPermissions = {
 
     if (added.permissions.length || added.origins.length) {
       await this._update(extensionId, { permissions, origins });
+      Management.emit("change-permissions", { extensionId, added });
       if (emitter) {
         emitter.emit("add-permissions", added);
       }
@@ -162,6 +169,7 @@ var ExtensionPermissions = {
 
     if (removed.permissions.length || removed.origins.length) {
       await this._update(extensionId, { permissions, origins });
+      Management.emit("change-permissions", { extensionId, removed });
       if (emitter) {
         emitter.emit("remove-permissions", removed);
       }
@@ -172,8 +180,13 @@ var ExtensionPermissions = {
     await lazyInit();
     StartupCache.permissions.delete(extensionId);
     if (prefs.data[extensionId]) {
+      let removed = prefs.data[extensionId];
       delete prefs.data[extensionId];
       prefs.saveSoon();
+      Management.emit("change-permissions", {
+        extensionId,
+        removed,
+      });
     }
   },
 

toolkit/components/extensions/ExtensionPreferencesManager.jsm

@@ -76,6 +76,18 @@ Management.on("enabling", async (type, id) => {
   await Management.asyncLoadSettingsModules();
   return ExtensionPreferencesManager.enableAll(id);
 });
+
+Management.on("change-permissions", (type, change) => {
+  // Called for added or removed, but we only care about removed here.
+  if (!change.removed) {
+    return;
+  }
+  ExtensionPreferencesManager.removeSettingsForPermissions(
+    change.extensionId,
+    change.removed.permissions
+  );
+});
+
 /* eslint-enable mozilla/balanced-listeners */
 
 const STORE_TYPE = "prefs";
@@ -414,13 +426,18 @@ this.ExtensionPreferencesManager = {
    * 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.
+   * @param {array<string>}
+   *                 permissions   The permission name from the extension manifest.
    * @returns {Promise}           A promise that resolves when all related settings are removed.
    */
-  removeSettingsForPermission(id, permission) {
+  async removeSettingsForPermissions(id, permissions) {
+    if (!permissions || !permissions.length) {
+      return;
+    }
+    await Management.asyncLoadSettingsModules();
     let removePromises = [];
     settingsMap.forEach((setting, name) => {
-      if (setting.permission == permission) {
+      if (permissions.includes(setting.permission)) {
         removePromises.push(this.removeSetting(id, name));
       }
     });

toolkit/components/extensions/parent/ext-browserSettings.js

@@ -189,17 +189,6 @@ this.browserSettings = class extends ExtensionAPI {
   getAPI(context) {
     let { extension } = context;
 
-    // eslint-disable-next-line mozilla/balanced-listeners
-    extension.on("remove-permissions", (ignoreEvent, permissions) => {
-      if (!permissions.permissions.includes("browserSettings")) {
-        return;
-      }
-      ExtensionPreferencesManager.removeSettingsForPermission(
-        extension.id,
-        "browserSettings"
-      );
-    });
-
     return {
       browserSettings: {
         allowPopupsForUserEvents: getSettingsAPI({

toolkit/components/extensions/parent/ext-permissions.js

@@ -4,16 +4,10 @@
 
 "use strict";
 
-ChromeUtils.defineModuleGetter(
-  this,
-  "ExtensionPermissions",
-  "resource://gre/modules/ExtensionPermissions.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "Services",
-  "resource://gre/modules/Services.jsm"
-);
+XPCOMUtils.defineLazyModuleGetters(this, {
+  ExtensionPermissions: "resource://gre/modules/ExtensionPermissions.jsm",
+  Services: "resource://gre/modules/Services.jsm",
+});
 
 var { ExtensionError } = ExtensionUtils;
 
@@ -23,9 +17,18 @@ XPCOMUtils.defineLazyPreferenceGetter(
   "extensions.webextOptionalPermissionPrompts"
 );
 
+function normalizePermissions(perms) {
+  perms = { ...perms };
+  perms.permissions = perms.permissions.filter(
+    perm => !perm.startsWith("internal:")
+  );
+  return perms;
+}
+
 this.permissions = class extends ExtensionAPI {
   getAPI(context) {
     let { extension } = context;
+
     return {
       permissions: {
         async request(perms) {
@@ -96,11 +99,8 @@ this.permissions = class extends ExtensionAPI {
         },
 
         async getAll() {
-          let perms = { ...context.extension.activePermissions };
+          let perms = normalizePermissions(context.extension.activePermissions);
           delete perms.apis;
-          perms.permissions = perms.permissions.filter(
-            perm => !perm.startsWith("internal:")
-          );
           return perms;
         },
 
@@ -132,6 +132,46 @@ this.permissions = class extends ExtensionAPI {
           );
           return true;
         },
+
+        onAdded: new EventManager({
+          context,
+          name: "permissions.onAdded",
+          register: fire => {
+            let callback = (event, change) => {
+              if (change.extensionId == extension.id && change.added) {
+                let perms = normalizePermissions(change.added);
+                if (perms.permissions.length || perms.origins.length) {
+                  fire.async(perms);
+                }
+              }
+            };
+
+            extensions.on("change-permissions", callback);
+            return () => {
+              extensions.off("change-permissions", callback);
+            };
+          },
+        }).api(),
+
+        onRemoved: new EventManager({
+          context,
+          name: "permissions.onRemoved",
+          register: fire => {
+            let callback = (event, change) => {
+              if (change.extensionId == extension.id && change.removed) {
+                let perms = normalizePermissions(change.removed);
+                if (perms.permissions.length || perms.origins.length) {
+                  fire.async(perms);
+                }
+              }
+            };
+
+            extensions.on("change-permissions", callback);
+            return () => {
+              extensions.off("change-permissions", callback);
+            };
+          },
+        }).api(),
       },
     };
   }

toolkit/components/extensions/parent/ext-privacy.js

@@ -258,19 +258,6 @@ ExtensionPreferencesManager.addSetting("network.tlsVersionRestriction", {
 
 this.privacy = class extends ExtensionAPI {
   getAPI(context) {
-    let { extension } = context;
-
-    // eslint-disable-next-line mozilla/balanced-listeners
-    extension.on("remove-permissions", (ignoreEvent, permissions) => {
-      if (!permissions.permissions.includes("privacy")) {
-        return;
-      }
-      ExtensionPreferencesManager.removeSettingsForPermission(
-        extension.id,
-        "privacy"
-      );
-    });
-
     return {
       privacy: {
         network: {

toolkit/components/extensions/schemas/permissions.json

@@ -128,7 +128,6 @@
       {
         "name": "onAdded",
         "type": "function",
-        "unsupported": true,
         "description": "Fired when the extension acquires new permissions.",
         "parameters": [
           {
@@ -140,7 +139,6 @@
       {
         "name": "onRemoved",
         "type": "function",
-        "unsupported": true,
         "description": "Fired when permissions are removed from the extension.",
         "parameters": [
           {

toolkit/components/extensions/test/xpcshell/test_ext_permissions_api.js

@@ -1,5 +1,23 @@
 "use strict";
 
+Services.prefs.setBoolPref(
+  "extensions.webextensions.background-delayed-startup",
+  false
+);
+
+const { AddonManager } = ChromeUtils.import(
+  "resource://gre/modules/AddonManager.jsm"
+);
+const { ExtensionPermissions } = ChromeUtils.import(
+  "resource://gre/modules/ExtensionPermissions.jsm"
+);
+
+ChromeUtils.defineModuleGetter(
+  this,
+  "ExtensionParent",
+  "resource://gre/modules/ExtensionParent.jsm"
+);
+
 AddonTestUtils.init(this);
 AddonTestUtils.overrideCertDB();
 AddonTestUtils.createAppInfo(
@@ -9,6 +27,8 @@ AddonTestUtils.createAppInfo(
   "42"
 );
 
+let OptionalPermissions;
+
 add_task(async function setup() {
   Services.prefs.setBoolPref(
     "extensions.webextOptionalPermissionPrompts",
@@ -19,56 +39,161 @@ add_task(async function setup() {
   });
   await AddonTestUtils.promiseStartupManager();
   AddonTestUtils.usePrivilegedSignatures = false;
+
+  // We want to get a list of optional permissions prior to loading an extension,
+  // so we'll get ExtensionParent to do that for us.
+  await ExtensionParent.apiManager.lazyInit();
+
+  // These permissions have special behaviors and/or are not mapped directly to an
+  // api namespace.  They will have their own tests for specific behavior.
+  let ignore = [
+    "activeTab",
+    "clipboardRead",
+    "clipboardWrite",
+    "downloads.open",
+    "geolocation",
+    "management",
+    "menus.overrideContext",
+    "search",
+    "tabHide",
+    "tabs",
+    "webRequestBlocking",
+  ];
+  OptionalPermissions = Schemas.getPermissionNames([
+    "OptionalPermission",
+    "OptionalPermissionNoPrompt",
+  ]).filter(n => !ignore.includes(n));
 });
 
 add_task(async function test_api_on_permissions_changed() {
   async function background() {
-    browser.test.onMessage.addListener(async opts => {
-      for (let perm of opts.optional_permissions) {
-        let permObj = { permissions: [perm] };
-        browser.test.assertFalse(
-          browser[perm],
-          `${perm} API is not available at start`
-        );
-        await browser.permissions.request(permObj);
-        browser.test.assertTrue(
-          browser[perm],
-          `${perm} API is available after permission request`
-        );
-        await browser.permissions.remove(permObj);
-        browser.test.assertFalse(
-          browser[perm],
-          `${perm} API is not available after permission remove`
+    let manifest = browser.runtime.getManifest();
+    let permObj = { permissions: manifest.optional_permissions, origins: [] };
+
+    function verifyPermissions(enabled) {
+      for (let perm of manifest.optional_permissions) {
+        browser.test.assertEq(
+          enabled,
+          !!browser[perm],
+          `${perm} API is ${
+            enabled ? "injected" : "removed"
+          } after permission request`
         );
       }
-      browser.test.notifyPass("done");
+    }
+
+    browser.permissions.onAdded.addListener(details => {
+      browser.test.assertEq(
+        JSON.stringify(details.permissions),
+        JSON.stringify(manifest.optional_permissions),
+        "expected permissions added"
+      );
+      verifyPermissions(true);
+      browser.test.sendMessage("added");
+    });
+
+    browser.permissions.onRemoved.addListener(details => {
+      browser.test.assertEq(
+        JSON.stringify(details.permissions),
+        JSON.stringify(manifest.optional_permissions),
+        "expected permissions removed"
+      );
+      verifyPermissions(false);
+      browser.test.sendMessage("removed");
+    });
+
+    browser.test.onMessage.addListener((msg, enabled) => {
+      if (msg === "request") {
+        browser.permissions.request(permObj);
+      } else if (msg === "verify_access") {
+        verifyPermissions(enabled);
+        browser.test.sendMessage("verified");
+      } else if (msg === "revoke") {
+        browser.permissions.remove(permObj);
+      }
     });
   }
 
-  let optional_permissions = [
-    "downloads",
-    "cookies",
-    "privacy",
-    "webRequest",
-    "webNavigation",
-    "browserSettings",
-    "idle",
-    "notifications",
-  ];
-
   let extension = ExtensionTestUtils.loadExtension({
     background,
     manifest: {
-      optional_permissions,
+      optional_permissions: OptionalPermissions,
     },
     useAddonManager: "permanent",
   });
   await extension.startup();
 
+  function addPermissions() {
+    extension.sendMessage("request");
+    return extension.awaitMessage("added");
+  }
+
+  function removePermissions() {
+    extension.sendMessage("revoke");
+    return extension.awaitMessage("removed");
+  }
+
+  function verifyPermissions(enabled) {
+    extension.sendMessage("verify_access", enabled);
+    return extension.awaitMessage("verified");
+  }
+
   await withHandlingUserInput(extension, async () => {
-    extension.sendMessage({ optional_permissions });
-    await extension.awaitFinish("done");
+    await addPermissions();
+    await removePermissions();
+    await addPermissions();
   });
+
+  // reset handlingUserInput for the restart
+  extensionHandlers.delete(extension);
+
+  // Verify access on restart
+  await AddonTestUtils.promiseRestartManager();
+  await extension.awaitStartup();
+  await verifyPermissions(true);
+
+  await withHandlingUserInput(extension, async () => {
+    await removePermissions();
+  });
+
+  // Add private browsing to be sure it doesn't come through.
+  let permObj = {
+    permissions: OptionalPermissions.concat("internal:privateBrowsingAllowed"),
+    origins: [],
+  };
+
+  // enable the permissions while the addon is running
+  await ExtensionPermissions.add(extension.id, permObj, extension.extension);
+  await extension.awaitMessage("added");
+  await verifyPermissions(true);
+
+  // disable the permissions while the addon is running
+  await ExtensionPermissions.remove(extension.id, permObj, extension.extension);
+  await extension.awaitMessage("removed");
+  await verifyPermissions(false);
+
+  // Add private browsing to test internal permission.  If it slips through,
+  // we would get an error for an additional added message.
+  await ExtensionPermissions.add(
+    extension.id,
+    { permissions: ["internal:privateBrowsingAllowed"], origins: [] },
+    extension.extension
+  );
+
+  // disable the addon and re-test revoking permissions.
+  await withHandlingUserInput(extension, async () => {
+    await addPermissions();
+  });
+  let addon = await AddonManager.getAddonByID(extension.id);
+  await addon.disable();
+  await ExtensionPermissions.remove(extension.id, permObj);
+  await addon.enable();
+  await extension.awaitStartup();
+
+  await verifyPermissions(false);
+  let perms = await ExtensionPermissions.get(extension.id);
+  equal(perms.permissions.length, 0, "no permissions on startup");
+
   await extension.unload();
 });
 
@@ -188,7 +313,23 @@ add_task(async function test_browserSetting_permissions() {
     extension.sendMessage("remove");
     await extension.awaitMessage("done");
     ok(cacheIsEnabled(), "setting is reset after remove");
+
+    extension.sendMessage("request");
+    await extension.awaitMessage("done");
+    ok(!cacheIsEnabled(), "setting was set after request");
   });
+
+  await ExtensionPermissions._uninit();
+  extensionHandlers.delete(extension);
+  await AddonTestUtils.promiseRestartManager();
+  await extension.awaitStartup();
+
+  await withHandlingUserInput(extension, async () => {
+    extension.sendMessage("remove");
+    await extension.awaitMessage("done");
+    ok(cacheIsEnabled(), "setting is reset after remove");
+  });
+
   await extension.unload();
 });
 
@@ -230,6 +371,22 @@ add_task(async function test_privacy_permissions() {
     extension.sendMessage("remove");
     await extension.awaitMessage("done");
     ok(!hasSetting(), "setting is reset after remove");
+
+    extension.sendMessage("request");
+    await extension.awaitMessage("done");
+    ok(hasSetting(), "setting was set after request");
   });
+
+  await ExtensionPermissions._uninit();
+  extensionHandlers.delete(extension);
+  await AddonTestUtils.promiseRestartManager();
+  await extension.awaitStartup();
+
+  await withHandlingUserInput(extension, async () => {
+    extension.sendMessage("remove");
+    await extension.awaitMessage("done");
+    ok(!hasSetting(), "setting is reset after remove");
+  });
+
   await extension.unload();
 });