From 6a64e4c3cfeb6d6eda2516cc80277d59082a613c Mon Sep 17 00:00:00 2001 From: Paul Zuehlcke Date: Thu, 6 May 2021 17:46:32 +0000 Subject: [PATCH] Bug 1597751 - Import default permissions for privateBrowsing. r=timhuang Differential Revision: https://phabricator.services.mozilla.com/D98200 --- browser/app/permissions | 1 + extensions/permissions/PermissionManager.cpp | 54 ++++++++++++-- .../test/unit/test_permmanager_defaults.js | 70 ++++++++++++++++--- 3 files changed, 110 insertions(+), 15 deletions(-) diff --git a/browser/app/permissions b/browser/app/permissions index 991284081d9e..c33d0155a15e 100644 --- a/browser/app/permissions +++ b/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 diff --git a/extensions/permissions/PermissionManager.cpp b/extensions/permissions/PermissionManager.cpp index 1fb93852d31f..521324516c07 100644 --- a/extensions/permissions/PermissionManager.cpp +++ b/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 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 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; diff --git a/extensions/permissions/test/unit/test_permmanager_defaults.js b/extensions/permissions/test/unit/test_permmanager_defaults.js index 0c0ab76d9d47..5a817a813be6 100644 --- a/extensions/permissions/test/unit/test_permmanager_defaults.js +++ b/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(