Bug 1801337 - Fix uninstall on sitepermission AddonCard created on th…

…e AOM onInstalled event. r=rpl. There are 2 folds to this patch. First, the `SitePermsAddonInstalling` was calling its parent constructor (`SitePermsAddonWrapper`), without the expected permissions parameter. This would then lead to the `uninstall` method to not do anything as there were no permissions. This is fixed by passing the permission for which we want to install the addon. Second, if multiple addons were installed for the same origin (e.g. "midi" and "midi-sysex") while about:addons was open, removing the addon would only revoke the "first" permission, as `SitePermsAddonInstalling` only holds one permission. To fix this, we define an `SitePermsAddonInstalling#uninstall` method that checks if we registered a `SitePermsAddonWrapper` instance for this origin, and in such case, uninstall the addon from this instance instead of the `SitePermsAddonInstalling`, one, as the `SitePermsAddonWrapper` instance has the whole set of permissions granted for a given origin. Test cases are added to ensure we don't regress this. Differential Revision: https://phabricator.services.mozilla.com/D162431
dothq · Nov 23, 2022 · 8048a79 · 8048a79
1 parent 50c332d
commit 8048a79
Show file tree

Hide file tree

Showing 4 changed files with 227 additions and 3 deletions.
diff --git a/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs b/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs
@@ -286,12 +286,40 @@ class SitePermsAddonInstalling extends SitePermsAddonWrapper {
    *                                         calling this constructor.
    */
   constructor(siteOriginNoSuffix, install) {
-    super(siteOriginNoSuffix);
+    // SitePermsAddonWrapper expect an array of nsIPermission as its second parameter.
+    // Since we don't have a proper permission here, we pass an object with the properties
+    // being used in the class.
+    const permission = {
+      principal: install.principal,
+      type: install.newSitePerm,
+    };
+
+    super(siteOriginNoSuffix, [permission]);
     this.#install = install;
   }
 
-  get sitePermissions() {
-    return Array.from(new Set([this.#install.newSitePerm]));
+  get existingAddon() {
+    return SitePermsAddonProvider.wrappersMapByOrigin.get(this.siteOrigin);
+  }
+
+  uninstall() {
+    // While about:addons tab is already open, new addon cards for newly installed
+    // addons are created from the `onInstalled` AOM events, for the `SitePermsAddonWrapper`
+    // the `onInstalling` and `onInstalled` events are emitted by `SitePermsAddonInstall`
+    // and the addon instance is going to be a `SitePermsAddonInstalling` instance if
+    // there wasn't an AddonCard for the same addon id yet.
+    //
+    // To make sure that all permissions will be uninstalled if a user uninstall the
+    // addon from an AddonCard created from a `SitePermsAddonInstalling` instance,
+    // we forward calls to the uninstall method of the existing `SitePermsAddonWrapper`
+    // instance being tracked by the `SitePermsAddonProvider`.
+    // If there isn't any then removing only the single permission added along with the
+    // `SitePremsAddonInstalling` is going to be enough.
+    if (this.existingAddon) {
+      return this.existingAddon.uninstall();
+    }
+
+    return super.uninstall();
   }
 
   validInstallOrigins() {

diff --git a/toolkit/mozapps/extensions/test/browser/browser.ini b/toolkit/mozapps/extensions/test/browser/browser.ini
@@ -87,6 +87,7 @@ support-files =
 [browser_html_recommendations.js]
 https_first_disabled = true
 [browser_html_scroll_restoration.js]
+[browser_html_sitepermission_addons.js]
 [browser_html_updates.js]
 https_first_disabled = true
 [browser_html_warning_messages.js]

diff --git a/toolkit/mozapps/extensions/test/browser/browser_html_sitepermission_addons.js b/toolkit/mozapps/extensions/test/browser/browser_html_sitepermission_addons.js
@@ -0,0 +1,183 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const { AddonTestUtils } = ChromeUtils.import(
+  "resource://testing-common/AddonTestUtils.jsm"
+);
+const {
+  SITEPERMS_ADDON_PROVIDER_PREF,
+  SITEPERMS_ADDON_TYPE,
+} = ChromeUtils.importESModule(
+  "resource://gre/modules/addons/siteperms-addon-utils.sys.mjs"
+);
+
+const html = `<!DOCTYPE html><h1>Test midi permission with synthetic site permission addon</h1>`;
+const EXAMPLE_COM_URL = `https://example.com/document-builder.sjs?html=${html}`;
+const EXAMPLE_ORG_URL = `https://example.org/document-builder.sjs?html=${html}`;
+
+async function uninstallAllSitePermissionAddons() {
+  const sitepermAddons = await AddonManager.getAddonsByTypes([
+    SITEPERMS_ADDON_TYPE,
+  ]);
+  for (const addon of sitepermAddons) {
+    await addon.uninstall();
+  }
+}
+
+add_setup(async () => {
+  await SpecialPowers.pushPrefEnv({
+    set: [["midi.prompt.testing", false]],
+  });
+  await SpecialPowers.pushPrefEnv({
+    set: [["midi.testing", true]],
+  });
+
+  registerCleanupFunction(uninstallAllSitePermissionAddons);
+});
+
+add_task(async function testAboutAddonUninstall() {
+  if (!AddonManager.hasProvider("SitePermsAddonProvider")) {
+    ok(
+      !Services.prefs.getBoolPref(SITEPERMS_ADDON_PROVIDER_PREF),
+      "Expect SitePermsAddonProvider to be disabled by prefs"
+    );
+    ok(true, "Skip test on SitePermsAddonProvider disabled");
+    return;
+  }
+
+  // Grant midi permission on example.com so about:addons does have a Site Permissions section
+  await SpecialPowers.addPermission("midi-sysex", true, EXAMPLE_COM_URL);
+
+  info("Open an about:addon tab so AMO event listeners are registered");
+  const aboutAddonWin = await loadInitialView("sitepermission");
+  // loadInitialView sets the about:addon as the active one, so we can grab it here.
+  const aboutAddonTab = gBrowser.selectedTab;
+
+  const addonList = aboutAddonWin.document.querySelector("addon-list");
+  let addonCards = aboutAddonWin.document.querySelectorAll("addon-card");
+  is(
+    addonCards.length,
+    1,
+    "There's a card displayed for the example.com addon"
+  );
+
+  info("Open an example.org tab and install the midi site permission addon");
+  const exampleOrgTab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    EXAMPLE_ORG_URL,
+    true /* waitForLoad */
+  );
+
+  let promiseAddonCardAdded = BrowserTestUtils.waitForEvent(addonList, "add");
+
+  info("Install midi");
+  await testInstallGatedPermission(
+    exampleOrgTab,
+    () => {
+      content.navigator.requestMIDIAccess();
+    },
+    "midi"
+  );
+
+  info("Install midi-sysex as well");
+  const newAddon = await testInstallGatedPermission(
+    exampleOrgTab,
+    () => {
+      content.navigator.requestMIDIAccess({ sysex: true });
+    },
+    "midi-sysex"
+  );
+
+  const newAddonId = newAddon.id;
+  ok(
+    newAddonId,
+    "Got the addon id for the newly installed sitepermission add-on"
+  );
+
+  info("Switch back to about:addon");
+  gBrowser.selectedTab = aboutAddonTab;
+
+  await promiseAddonCardAdded;
+
+  is(
+    aboutAddonWin.document.querySelectorAll("addon-card").length,
+    2,
+    "A new addon card has been added as expected"
+  );
+
+  const exampleOrgAddonCard = getAddonCard(aboutAddonWin, newAddonId);
+
+  info("Remove the example.org addon");
+  const promptService = mockPromptService();
+  promptService._response = 0;
+
+  let promiseRemoved = BrowserTestUtils.waitForEvent(addonList, "remove");
+  exampleOrgAddonCard.querySelector("[action=remove]").click();
+  await promiseRemoved;
+
+  is(
+    aboutAddonWin.document.querySelectorAll("addon-card").length,
+    1,
+    "addon card has been removed as expected"
+  );
+
+  ok(
+    await SpecialPowers.testPermission(
+      "midi",
+      SpecialPowers.Services.perms.UNKNOWN_ACTION,
+      { url: EXAMPLE_ORG_URL }
+    ),
+    "midi permission was revoked"
+  );
+  ok(
+    await SpecialPowers.testPermission(
+      "midi-sysex",
+      SpecialPowers.Services.perms.UNKNOWN_ACTION,
+      { url: EXAMPLE_ORG_URL }
+    ),
+    "midi-sysex permission was revoked as well"
+  );
+
+  await BrowserTestUtils.removeTab(exampleOrgTab);
+  await close_manager(aboutAddonWin);
+  await uninstallAllSitePermissionAddons();
+});
+
+/**
+ *
+ * Execute a function in the tab content page and check that the expected gated permission
+ * is set
+ *
+ * @param {Tab} tab: The tab in which we want to install the gated permission
+ * @param {Function} spawnCallback: function used in `SpecialPowers.spawn` that will trigger
+ *                                  the install
+ * @param {String} expectedPermType: The name of the permission that should be granted
+ * @returns {Promise<Addon>} The installed addon instance
+ */
+async function testInstallGatedPermission(
+  tab,
+  spawnCallback,
+  expectedPermType
+) {
+  let onInstallEnded = AddonTestUtils.promiseInstallEvent("onInstallEnded");
+  let onAddonInstallBlockedNotification = promisePopupNotificationShown(
+    "addon-install-blocked"
+  );
+  await SpecialPowers.spawn(tab.linkedBrowser, [], spawnCallback);
+
+  let addonInstallPanel = await onAddonInstallBlockedNotification;
+  let dialogPromise = promisePopupNotificationShown("addon-webext-permissions");
+  addonInstallPanel.button.click();
+  let installPermsDialog = await dialogPromise;
+  installPermsDialog.button.click();
+
+  ok(
+    await SpecialPowers.testPermission(
+      expectedPermType,
+      SpecialPowers.Services.perms.ALLOW_ACTION,
+      { url: EXAMPLE_ORG_URL }
+    ),
+    `"${expectedPermType}" permission was granted`
+  );
+  return onInstallEnded.then(install => install[0].addon);
+}
diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_sitePermsAddonProvider.js b/toolkit/mozapps/extensions/test/xpcshell/test_sitePermsAddonProvider.js
@@ -479,6 +479,10 @@ add_task(
       "Call installSitePermsAddonFromWebpage for authorized principal and gated permission"
     );
     expectAndHandleInstallPrompts();
+    const onAddonInstalled = AddonTestUtils.promiseInstallEvent(
+      "onInstallEnded"
+    ).then(installs => installs?.[0]?.addon);
+
     await AddonManager.installSitePermsAddonFromWebpage(
       null,
       PRINCIPAL_COM,
@@ -496,6 +500,14 @@ add_task(
       "...and set the permission"
     );
 
+    // The addon we get here is a SitePermsAddonInstalling instance, and we want to assert
+    // that its permissions are correct as it may impact addon uninstall later on
+    Assert.deepEqual(
+      (await onAddonInstalled).sitePermissions,
+      [GATED_SITE_PERM1],
+      "Addon has expected sitePermissions"
+    );
+
     info(
       "Call installSitePermsAddonFromWebpage for private browsing principal and gated permission"
     );