Bug 1597751 - Import default permissions for privateBrowsing. r=timhuang

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

browser/app/permissions

@@ -4,6 +4,7 @@
 # * "origin" should be used for matchtype, "host" is supported for legacy reasons
 # * type is a string that identifies the type of permission (e.g. "cookie")
 # * permission is an integer between 1 and 15
+# Permissions defined here will also be set for private browsing.
 # See PermissionManager.cpp for more...
 
 # UITour

extensions/permissions/PermissionManager.cpp

@@ -1598,8 +1598,10 @@ nsresult PermissionManager::AddInternal(
     }
   }
 
-  // For private browsing only store permissions for the session
-  if (aExpireType != EXPIRE_SESSION) {
+  // For private browsing only store permissions for the session. Except for
+  // default permissions which are stored in-memory only and imported each
+  // startup.
+  if (aID != cIDPermissionIsDefault && aExpireType != EXPIRE_SESSION) {
     uint32_t privateBrowsingId =
         nsScriptSecurityManager::DEFAULT_PRIVATE_BROWSING_ID;
     nsresult rv = aPrincipal->GetPrivateBrowsingId(&privateBrowsingId);
@@ -3412,12 +3414,31 @@ nsresult PermissionManager::ImportLatestDefaults() {
                 GetPrincipalFromOrigin(aOrigin, IsOAForceStripPermission(aType),
                                        getter_AddRefs(principal));
             NS_ENSURE_SUCCESS(rv, rv);
+            rv =
+                AddInternal(principal, aType, aPermission,
+                            cIDPermissionIsDefault, aExpireType, aExpireTime,
+                            aModificationTime, PermissionManager::eDontNotify,
+                            PermissionManager::eNoDBOperation, false, &aOrigin);
+            NS_ENSURE_SUCCESS(rv, rv);
 
-            return AddInternal(
-                principal, aType, aPermission, cIDPermissionIsDefault,
-                aExpireType, aExpireTime, aModificationTime,
-                PermissionManager::eDontNotify,
-                PermissionManager::eNoDBOperation, false, &aOrigin);
+            if (StaticPrefs::permissions_isolateBy_privateBrowsing()) {
+              // Also import the permission for private browsing.
+              OriginAttributes attrs =
+                  OriginAttributes(principal->OriginAttributesRef());
+              attrs.mPrivateBrowsingId = 1;
+              nsCOMPtr<nsIPrincipal> pbPrincipal =
+                  BasePrincipal::Cast(principal)->CloneForcingOriginAttributes(
+                      attrs);
+
+              rv = AddInternal(
+                  pbPrincipal, aType, aPermission, cIDPermissionIsDefault,
+                  aExpireType, aExpireTime, aModificationTime,
+                  PermissionManager::eDontNotify,
+                  PermissionManager::eNoDBOperation, false, &aOrigin);
+              NS_ENSURE_SUCCESS(rv, rv);
+            }
+
+            return NS_OK;
           });
 
       if (NS_FAILED(rv)) {
@@ -3447,6 +3468,25 @@ nsresult PermissionManager::ImportLatestDefaults() {
     if (NS_FAILED(rv)) {
       NS_WARNING("There was a problem importing an origin permission");
     }
+
+    if (StaticPrefs::permissions_isolateBy_privateBrowsing()) {
+      // Also import the permission for private browsing.
+      OriginAttributes attrs =
+          OriginAttributes(principal->OriginAttributesRef());
+      attrs.mPrivateBrowsingId = 1;
+      nsCOMPtr<nsIPrincipal> pbPrincipal =
+          BasePrincipal::Cast(principal)->CloneForcingOriginAttributes(attrs);
+
+      rv = AddInternal(pbPrincipal, entry.mType, entry.mPermission,
+                       cIDPermissionIsDefault,
+                       nsIPermissionManager::EXPIRE_NEVER, 0, modificationTime,
+                       eDontNotify, eNoDBOperation);
+      if (NS_FAILED(rv)) {
+        NS_WARNING(
+            "There was a problem importing an origin permission for private "
+            "browsing");
+      }
+    }
   }
 
   return NS_OK;

extensions/permissions/test/unit/test_permmanager_defaults.js

@@ -60,6 +60,9 @@ add_task(async function do_test() {
   let permIsolateUserContext = Services.prefs.getBoolPref(
     "permissions.isolateBy.userContext"
   );
+  let permIsolatePrivateBrowsing = Services.prefs.getBoolPref(
+    "permissions.isolateBy.privateBrowsing"
+  );
 
   let pm = Services.perms;
 
@@ -92,7 +95,12 @@ add_task(async function do_test() {
   );
 
   attrs = { userContextId: 1 };
-  let principal6 = Services.scriptSecurityManager.createContentPrincipal(
+  let principal1UserContext = Services.scriptSecurityManager.createContentPrincipal(
+    TEST_ORIGIN,
+    attrs
+  );
+  attrs = { privateBrowsingId: 1 };
+  let principal1PrivateBrowsing = Services.scriptSecurityManager.createContentPrincipal(
     TEST_ORIGIN,
     attrs
   );
@@ -123,6 +131,16 @@ add_task(async function do_test() {
     Ci.nsIPermissionManager.ALLOW_ACTION,
     pm.testPermissionFromPrincipal(principal4, TEST_PERMISSION)
   );
+  // Depending on the prefs there are two scenarios here:
+  // 1. We isolate by private browsing: The permission mgr should
+  //    add default permissions for these principals too.
+  // 2. We don't isolate by private browsing: The permission
+  //    check will strip the private browsing origin attribute.
+  //    In this case the used internally for the lookup is always principal1.
+  Assert.equal(
+    Ci.nsIPermissionManager.ALLOW_ACTION,
+    pm.testPermissionFromPrincipal(principal1PrivateBrowsing, TEST_PERMISSION)
+  );
 
   // Didn't add
   Assert.equal(
@@ -158,13 +176,18 @@ add_task(async function do_test() {
     Ci.nsIPermissionManager.ALLOW_ACTION,
     pm.testPermissionFromPrincipal(principal4, TEST_PERMISSION)
   );
+  // Default permission should have also been added for private browsing.
+  Assert.equal(
+    Ci.nsIPermissionManager.ALLOW_ACTION,
+    pm.testPermissionFromPrincipal(principal1PrivateBrowsing, TEST_PERMISSION)
+  );
   // make sure principals with userContextId use the same / different permissions
   // depending on pref state
   Assert.equal(
     permIsolateUserContext
       ? Ci.nsIPermissionManager.UNKNOWN_ACTION
       : Ci.nsIPermissionManager.ALLOW_ACTION,
-    pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
+    pm.testPermissionFromPrincipal(principal1UserContext, TEST_PERMISSION)
   );
   // make sure principals with a firstPartyDomain use different permissions
   Assert.equal(
@@ -187,7 +210,15 @@ add_task(async function do_test() {
   // (Should be unknown with and without OA stripping )
   Assert.equal(
     Ci.nsIPermissionManager.UNKNOWN_ACTION,
-    pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
+    pm.testPermissionFromPrincipal(principal1UserContext, TEST_PERMISSION)
+  );
+  // If we isolate by private browsing, the permission should still be present
+  // for the private browsing principal.
+  Assert.equal(
+    permIsolatePrivateBrowsing
+      ? Ci.nsIPermissionManager.ALLOW_ACTION
+      : Ci.nsIPermissionManager.UNKNOWN_ACTION,
+    pm.testPermissionFromPrincipal(principal1PrivateBrowsing, TEST_PERMISSION)
   );
   // and we should have this UNKNOWN_ACTION reflected in the DB
   await checkCapabilityViaDB(Ci.nsIPermissionManager.UNKNOWN_ACTION);
@@ -201,12 +232,17 @@ add_task(async function do_test() {
     Ci.nsIPermissionManager.ALLOW_ACTION,
     pm.testPermissionFromPrincipal(principal, TEST_PERMISSION)
   );
+  // Make sure default imports work for private browsing after removeAll.
+  Assert.equal(
+    Ci.nsIPermissionManager.ALLOW_ACTION,
+    pm.testPermissionFromPrincipal(principal1PrivateBrowsing, TEST_PERMISSION)
+  );
   // make sure principals with userContextId share permissions depending on pref state
   Assert.equal(
     permIsolateUserContext
       ? Ci.nsIPermissionManager.UNKNOWN_ACTION
       : Ci.nsIPermissionManager.ALLOW_ACTION,
-    pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
+    pm.testPermissionFromPrincipal(principal1UserContext, TEST_PERMISSION)
   );
   // make sure principals with firstPartyDomain use different permissions
   Assert.equal(
@@ -232,12 +268,21 @@ add_task(async function do_test() {
     Ci.nsIPermissionManager.DENY_ACTION,
     pm.testPermissionFromPrincipal(principal, TEST_PERMISSION)
   );
-  // make sure principals with userContextId share permissions depending on pref state
+  // make sure principals with userContextId use the same / different permissions
+  // depending on pref state
   Assert.equal(
     permIsolateUserContext
       ? Ci.nsIPermissionManager.UNKNOWN_ACTION
       : Ci.nsIPermissionManager.DENY_ACTION,
-    pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
+    pm.testPermissionFromPrincipal(principal1UserContext, TEST_PERMISSION)
+  );
+  // If we isolate by private browsing, we should still have the default perm
+  // for the private browsing principal.
+  Assert.equal(
+    permIsolatePrivateBrowsing
+      ? Ci.nsIPermissionManager.ALLOW_ACTION
+      : Ci.nsIPermissionManager.DENY_ACTION,
+    pm.testPermissionFromPrincipal(principal1PrivateBrowsing, TEST_PERMISSION)
   );
   // make sure principals with firstPartyDomain use different permissions
   Assert.equal(
@@ -264,12 +309,21 @@ add_task(async function do_test() {
     Ci.nsIPermissionManager.PROMPT_ACTION,
     pm.testPermissionFromPrincipal(principal, TEST_PERMISSION)
   );
-  // make sure principals with userContextId share permissions depending on pref state
+  // make sure principals with userContextId use the same / different permissions
+  // depending on pref state
   Assert.equal(
     permIsolateUserContext
       ? Ci.nsIPermissionManager.UNKNOWN_ACTION
       : Ci.nsIPermissionManager.PROMPT_ACTION,
-    pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
+    pm.testPermissionFromPrincipal(principal1UserContext, TEST_PERMISSION)
+  );
+  // If we isolate by private browsing, we should still have the default perm
+  // for the private browsing principal.
+  Assert.equal(
+    permIsolatePrivateBrowsing
+      ? Ci.nsIPermissionManager.ALLOW_ACTION
+      : Ci.nsIPermissionManager.PROMPT_ACTION,
+    pm.testPermissionFromPrincipal(principal1PrivateBrowsing, TEST_PERMISSION)
   );
   // make sure principals with firstPartyDomain use different permissions
   Assert.equal(