Bug 1379560 - Part 2 - Add support for custom default permissions in SitePermissions.jsm. r=Paolo draft
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 10 Jul 2017 23:33:37 +0200
changeset 681935 2c8da24f849cee53e17be8897c0b320ca9e39e7e
parent 681934 9fbcfc740a85c02cf4245956e69ae13c8f90b5ab
child 736258 94a62474885c3093361059b92bfc4cd69db71f9d
push id84952
push userbmo:jhofmann@mozilla.com
push dateTue, 17 Oct 2017 22:01:10 +0000
reviewersPaolo
bugs1379560
milestone58.0a1
Bug 1379560 - Part 2 - Add support for custom default permissions in SitePermissions.jsm. r=Paolo Part 1 added support for changing default permissions via pref. This patch adds support in the frontend code, which is required to actually make it work for most permission prompts. This patch introduces the concept of SitePermissions.PROMPT (which already exists in the permission manager) to distinguish between the default UNKNOWN state and the explicit PROMPT state. They both have the same effect (always asking the user for confirmation). MozReview-Commit-ID: 2Gg9uwigter
browser/base/content/pageinfo/permissions.js
browser/components/preferences/sitePermissions.js
browser/locales/en-US/chrome/browser/sitePermissions.properties
browser/modules/SitePermissions.jsm
browser/modules/test/unit/test_SitePermissions.js
--- a/browser/base/content/pageinfo/permissions.js
+++ b/browser/base/content/pageinfo/permissions.js
@@ -71,24 +71,24 @@ function initRow(aPartId) {
     return;
   }
 
   createRow(aPartId);
 
   var checkbox = document.getElementById(aPartId + "Def");
   var command  = document.getElementById("cmd_" + aPartId + "Toggle");
   var {state} = SitePermissions.get(gPermURI, aPartId);
+  let defaultState = SitePermissions.getDefault(aPartId);
 
-  if (state != SitePermissions.UNKNOWN) {
+  if (state != defaultState) {
     checkbox.checked = false;
     command.removeAttribute("disabled");
   } else {
     checkbox.checked = true;
     command.setAttribute("disabled", "true");
-    state = SitePermissions.getDefault(aPartId);
   }
   setRadioState(aPartId, state);
 
   if (aPartId == "indexedDB") {
     initIndexedDBRow();
   }
 }
 
@@ -164,17 +164,17 @@ function onCheckboxClick(aPartId) {
 
 function onPluginRadioClick(aEvent) {
   onRadioClick(aEvent.originalTarget.getAttribute("id").split("#")[0]);
 }
 
 function onRadioClick(aPartId) {
   var radioGroup = document.getElementById(aPartId + "RadioGroup");
   var id = radioGroup.selectedItem.id;
-  var permission = id.split("#")[1];
+  var permission = parseInt(id.split("#")[1]);
   SitePermissions.set(gPermURI, aPartId, permission);
 }
 
 function setRadioState(aPartId, aValue) {
   var radio = document.getElementById(aPartId + "#" + aValue);
   if (radio) {
     radio.radioGroup.selectedItem = radio;
   }
--- a/browser/components/preferences/sitePermissions.js
+++ b/browser/components/preferences/sitePermissions.js
@@ -155,18 +155,25 @@ var gSitePermissionsManager = {
     let menulist = document.createElement("menulist");
     let menupopup = document.createElement("menupopup");
     menulist.setAttribute("flex", "1");
     menulist.setAttribute("width", "50");
     menulist.setAttribute("class", "website-status");
     menulist.appendChild(menupopup);
     let states = SitePermissions.getAvailableStates(permission.type);
     for (let state of states) {
-      if (state == SitePermissions.UNKNOWN)
+      // Work around the (rare) edge case when a user has changed their
+      // default permission type back to UNKNOWN while still having a
+      // PROMPT permission set for an origin.
+      if (state == SitePermissions.UNKNOWN &&
+          permission.capability == SitePermissions.PROMPT) {
+        state = SitePermissions.PROMPT;
+      } else if (state == SitePermissions.UNKNOWN) {
         continue;
+      }
       let m = document.createElement("menuitem");
       m.setAttribute("label", this._getCapabilityString(state));
       m.setAttribute("value", state);
       menupopup.appendChild(m);
     }
     menulist.value = permission.capability;
 
     menulist.addEventListener("select", () => {
--- a/browser/locales/en-US/chrome/browser/sitePermissions.properties
+++ b/browser/locales/en-US/chrome/browser/sitePermissions.properties
@@ -9,16 +9,17 @@
 #                    state.current.blocked):
 # This label is used to display active permission states in the site
 # identity popup (which does not have a lot of screen space).
 state.current.allowed = Allowed
 state.current.allowedForSession = Allowed for Session
 state.current.allowedTemporarily = Allowed Temporarily
 state.current.blockedTemporarily = Blocked Temporarily
 state.current.blocked = Blocked
+state.current.prompt = Always Ask
 
 # LOCALIZATION NOTE (state.multichoice.alwaysAsk,
 #                    state.multichoice.allow,
 #                    state.multichoice.allowForSession,
 #                    state.multichoice.block):
 # Used to label permission state checkboxes in the page info dialog.
 state.multichoice.alwaysAsk = Always Ask
 state.multichoice.allow = Allow
--- a/browser/modules/SitePermissions.jsm
+++ b/browser/modules/SitePermissions.jsm
@@ -136,24 +136,27 @@ const TemporaryBlockedPermissions = {
  * Some methods have the side effect of dispatching a "PermissionStateChange"
  * event on changes to temporary permissions, as mentioned in the respective docs.
  */
 this.SitePermissions = {
   // Permission states.
   UNKNOWN: Services.perms.UNKNOWN_ACTION,
   ALLOW: Services.perms.ALLOW_ACTION,
   BLOCK: Services.perms.DENY_ACTION,
+  PROMPT: Services.perms.PROMPT_ACTION,
   ALLOW_COOKIES_FOR_SESSION: Components.interfaces.nsICookiePermission.ACCESS_SESSION,
 
   // Permission scopes.
   SCOPE_REQUEST: "{SitePermissions.SCOPE_REQUEST}",
   SCOPE_TEMPORARY: "{SitePermissions.SCOPE_TEMPORARY}",
   SCOPE_SESSION: "{SitePermissions.SCOPE_SESSION}",
   SCOPE_PERSISTENT: "{SitePermissions.SCOPE_PERSISTENT}",
 
+  _defaultPrefBranch: Services.prefs.getBranch("permissions.default."),
+
   /**
    * Gets all custom permissions for a given URI.
    * Install addon permission is excluded, check bug 1303108.
    *
    * @return {Array} a list of objects with the keys:
    *          - id: the permissionId of the permission
    *          - scope: the scope of the permission (e.g. SitePermissions.SCOPE_TEMPORARY)
    *          - state: a constant representing the current permission state
@@ -273,36 +276,42 @@ this.SitePermissions = {
    *
    * @return {Array<SitePermissions state>} an array of all permission states.
    */
   getAvailableStates(permissionID) {
     if (permissionID in gPermissionObject &&
         gPermissionObject[permissionID].states)
       return gPermissionObject[permissionID].states;
 
+    /* Since the permissions we are dealing with have adopted the convention
+     * of treating UNKNOWN == PROMPT, we only include one of either UNKNOWN
+     * or PROMPT in this list, to avoid duplicating states. */
     if (this.getDefault(permissionID) == this.UNKNOWN)
       return [ SitePermissions.UNKNOWN, SitePermissions.ALLOW, SitePermissions.BLOCK ];
 
-    return [ SitePermissions.ALLOW, SitePermissions.BLOCK ];
+    return [ SitePermissions.PROMPT, SitePermissions.ALLOW, SitePermissions.BLOCK ];
   },
 
   /**
    * Returns the default state of a particular permission.
    *
    * @param {string} permissionID
    *        The ID to get the default for.
    *
    * @return {SitePermissions.state} the default state.
    */
   getDefault(permissionID) {
+    // If the permission has custom logic for getting its default value,
+    // try that first.
     if (permissionID in gPermissionObject &&
         gPermissionObject[permissionID].getDefault)
       return gPermissionObject[permissionID].getDefault();
 
-    return this.UNKNOWN;
+    // Otherwise try to get the default preference for that permission.
+    return this._defaultPrefBranch.getIntPref(permissionID, this.UNKNOWN);
   },
 
   /**
    * Returns the state and scope of a particular permission for a given URI.
    *
    * This method will NOT dispatch a "PermissionStateChange" event on the specified
    * browser if a temporary permission was removed because it has expired.
    *
@@ -315,17 +324,18 @@ this.SitePermissions = {
    *
    * @return {Object} an object with the keys:
    *           - state: The current state of the permission
    *             (e.g. SitePermissions.ALLOW)
    *           - scope: The scope of the permission
    *             (e.g. SitePermissions.SCOPE_PERSISTENT)
    */
   get(uri, permissionID, browser) {
-    let result = { state: this.UNKNOWN, scope: this.SCOPE_PERSISTENT };
+    let defaultState = this.getDefault(permissionID);
+    let result = { state: defaultState, scope: this.SCOPE_PERSISTENT };
     if (this.isSupportedURI(uri)) {
       let permission = null;
       if (permissionID in gPermissionObject &&
         gPermissionObject[permissionID].exactHostMatch) {
         permission = Services.perms.getPermissionObjectForURI(uri, permissionID, true);
       } else {
         permission = Services.perms.getPermissionObjectForURI(uri, permissionID, false);
       }
@@ -333,17 +343,17 @@ this.SitePermissions = {
       if (permission) {
         result.state = permission.capability;
         if (permission.expireType == Services.perms.EXPIRE_SESSION) {
           result.scope = this.SCOPE_SESSION;
         }
       }
     }
 
-    if (!result.state) {
+    if (result.state == defaultState) {
       // If there's no persistent permission saved, check if we have something
       // set temporarily.
       let value = TemporaryBlockedPermissions.get(browser, permissionID);
 
       if (value) {
         result.state = value.state;
         result.scope = this.SCOPE_TEMPORARY;
       }
@@ -366,17 +376,17 @@ this.SitePermissions = {
    *        The state of the permission.
    * @param {SitePermissions scope} scope (optional)
    *        The scope of the permission. Defaults to SCOPE_PERSISTENT.
    * @param {Browser} browser (optional)
    *        The browser object to set temporary permissions on.
    *        This needs to be provided if the scope is SCOPE_TEMPORARY!
    */
   set(uri, permissionID, state, scope = this.SCOPE_PERSISTENT, browser = null) {
-    if (state == this.UNKNOWN) {
+    if (state == this.UNKNOWN || state == this.getDefault(permissionID)) {
       this.remove(uri, permissionID, browser);
       return;
     }
 
     if (state == this.ALLOW_COOKIES_FOR_SESSION && permissionID != "cookie") {
       throw "ALLOW_COOKIES_FOR_SESSION can only be set on the cookie permission";
     }
 
@@ -483,16 +493,17 @@ this.SitePermissions = {
    *        The state to get the label for.
    *
    * @return {String|null} the localized label or null if an
    *         unknown state was passed.
    */
   getMultichoiceStateLabel(state) {
     switch (state) {
       case this.UNKNOWN:
+      case this.PROMPT:
         return gStringBundle.GetStringFromName("state.multichoice.alwaysAsk");
       case this.ALLOW:
         return gStringBundle.GetStringFromName("state.multichoice.allow");
       case this.ALLOW_COOKIES_FOR_SESSION:
         return gStringBundle.GetStringFromName("state.multichoice.allowForSession");
       case this.BLOCK:
         return gStringBundle.GetStringFromName("state.multichoice.block");
       default:
@@ -508,16 +519,18 @@ this.SitePermissions = {
    * @param {SitePermissions scope} scope (optional)
    *        The scope to get the label for.
    *
    * @return {String|null} the localized label or null if an
    *         unknown state was passed.
    */
   getCurrentStateLabel(state, scope = null) {
     switch (state) {
+      case this.PROMPT:
+        return gStringBundle.GetStringFromName("state.current.prompt");
       case this.ALLOW:
         if (scope && scope != this.SCOPE_PERSISTENT)
           return gStringBundle.GetStringFromName("state.current.allowedTemporarily");
         return gStringBundle.GetStringFromName("state.current.allowed");
       case this.ALLOW_COOKIES_FOR_SESSION:
         return gStringBundle.GetStringFromName("state.current.allowedForSession");
       case this.BLOCK:
         if (scope && scope != this.SCOPE_PERSISTENT)
@@ -548,20 +561,17 @@ var gPermissionObject = {
    *    e.g. "desktop-notification2" to use permission.desktop-notification2.label
    *
    *  - states
    *    Array of permission states to be exposed to the user.
    *    Defaults to ALLOW, BLOCK and the default state (see getDefault).
    */
 
   "image": {
-    getDefault() {
-      return Services.prefs.getIntPref("permissions.default.image") == 2 ?
-               SitePermissions.BLOCK : SitePermissions.ALLOW;
-    }
+    states: [ SitePermissions.ALLOW, SitePermissions.BLOCK ],
   },
 
   "cookie": {
     states: [ SitePermissions.ALLOW, SitePermissions.ALLOW_COOKIES_FOR_SESSION, SitePermissions.BLOCK ],
     getDefault() {
       if (Services.prefs.getIntPref("network.cookie.cookieBehavior") == 2)
         return SitePermissions.BLOCK;
 
@@ -589,24 +599,26 @@ var gPermissionObject = {
     exactHostMatch: true,
     states: [ SitePermissions.UNKNOWN, SitePermissions.BLOCK ],
   },
 
   "popup": {
     getDefault() {
       return Services.prefs.getBoolPref("dom.disable_open_during_load") ?
                SitePermissions.BLOCK : SitePermissions.ALLOW;
-    }
+    },
+    states: [ SitePermissions.ALLOW, SitePermissions.BLOCK ],
   },
 
   "install": {
     getDefault() {
       return Services.prefs.getBoolPref("xpinstall.whitelist.required") ?
                SitePermissions.BLOCK : SitePermissions.ALLOW;
-    }
+    },
+    states: [ SitePermissions.ALLOW, SitePermissions.BLOCK ],
   },
 
   "geo": {
     exactHostMatch: true
   },
 
   "indexedDB": {},
 
--- a/browser/modules/test/unit/test_SitePermissions.js
+++ b/browser/modules/test/unit/test_SitePermissions.js
@@ -61,16 +61,24 @@ add_task(async function testGetAllByURI(
 });
 
 add_task(async function testGetAvailableStates() {
   Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
                    [ SitePermissions.UNKNOWN,
                      SitePermissions.ALLOW,
                      SitePermissions.BLOCK ]);
 
+  // Test available states with a default permission set.
+  Services.prefs.setIntPref("permissions.default.camera", SitePermissions.ALLOW);
+  Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
+                   [ SitePermissions.PROMPT,
+                     SitePermissions.ALLOW,
+                     SitePermissions.BLOCK ]);
+  Services.prefs.clearUserPref("permissions.default.camera");
+
   Assert.deepEqual(SitePermissions.getAvailableStates("cookie"),
                    [ SitePermissions.ALLOW,
                      SitePermissions.ALLOW_COOKIES_FOR_SESSION,
                      SitePermissions.BLOCK ]);
 
   Assert.deepEqual(SitePermissions.getAvailableStates("popup"),
                    [ SitePermissions.ALLOW,
                      SitePermissions.BLOCK ]);
@@ -91,26 +99,80 @@ add_task(async function testExactHostMat
   let nonExactHostMatched = ["image", "cookie", "popup", "install", "indexedDB"];
 
   let permissions = SitePermissions.listPermissions();
   for (let permission of permissions) {
     SitePermissions.set(uri, permission, SitePermissions.ALLOW);
 
     if (exactHostMatched.includes(permission)) {
       // Check that the sub-origin does not inherit the permission from its parent.
-      Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.UNKNOWN);
+      Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.UNKNOWN,
+        `${permission} should exact-host match`);
     } else if (nonExactHostMatched.includes(permission)) {
       // Check that the sub-origin does inherit the permission from its parent.
-      Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.ALLOW);
+      Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.ALLOW,
+        `${permission} should not exact-host match`);
     } else {
       Assert.ok(false, `Found an unknown permission ${permission} in exact host match test.` +
                        "Please add new permissions from SitePermissions.jsm to this test.");
     }
 
     // Check that the permission can be made specific to the sub-origin.
-    SitePermissions.set(subUri, permission, SitePermissions.BLOCK);
-    Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.BLOCK);
+    SitePermissions.set(subUri, permission, SitePermissions.PROMPT);
+    Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.PROMPT);
     Assert.equal(SitePermissions.get(uri, permission).state, SitePermissions.ALLOW);
 
     SitePermissions.remove(subUri, permission);
     SitePermissions.remove(uri, permission);
   }
 });
+
+add_task(function* testDefaultPrefs() {
+  let uri = Services.io.newURI("https://example.com")
+
+  // Check that without a pref the default return value is UNKNOWN.
+  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
+    state: SitePermissions.UNKNOWN,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+
+  // Check that the default return value changed after setting the pref.
+  Services.prefs.setIntPref("permissions.default.camera", SitePermissions.BLOCK);
+  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
+    state: SitePermissions.BLOCK,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+
+  // Check that other permissions still return UNKNOWN.
+  Assert.deepEqual(SitePermissions.get(uri, "microphone"), {
+    state: SitePermissions.UNKNOWN,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+
+  // Check that the default return value changed after changing the pref.
+  Services.prefs.setIntPref("permissions.default.camera", SitePermissions.ALLOW);
+  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
+    state: SitePermissions.ALLOW,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+
+  // Check that the preference is ignored if there is a value.
+  SitePermissions.set(uri, "camera", SitePermissions.BLOCK);
+  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
+    state: SitePermissions.BLOCK,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+
+  // The preference should be honored again, after resetting the permissions.
+  SitePermissions.remove(uri, "camera");
+  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
+    state: SitePermissions.ALLOW,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+
+  // Should be UNKNOWN after clearing the pref.
+  Services.prefs.clearUserPref("permissions.default.camera");
+  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
+    state: SitePermissions.UNKNOWN,
+    scope: SitePermissions.SCOPE_PERSISTENT,
+  });
+});
+