Skip to content

Commit

Permalink
[Extensions Refactor] Move SiteAccess struct to PermissionsManager
Browse files Browse the repository at this point in the history 
ScriptingPermissionsModifier::SiteAccess struct and GetSiteAccess() are
only used by SitePermissionsHelper. Since we want to consolidate
permissions logic in one place, and allow potential use of permissions
outside Chrome, moved SiteAccess and related methods to
PermissionsManager. This will also allow easier checks between user
permissions and extension permissions.

This involved:
a) Moving GetSiteAccess and SiteAccess struct to PermissionsManager
b) Implementing GetRuntimePermissionsFromPrefs() and
HasWithheldHostPermissions in PermissionsManager. This methods are
called on various other places. Thus, leave placeholders in
ScriptingPermissionsModifier and finish the migration in a follow up CL

Bug: 1289441
Change-Id: Ie97e9843a3529b842462d9fdef48dad14927d33f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3466639
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Commit-Queue: Emilia Paz <emiliapaz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983419}
  • Loading branch information
@emilia-paz
emilia-paz authored and Chromium LUCI CQ committed Mar 21, 2022
1 parent 8de8632 commit 41345b3
Show file tree
Hide file tree
Showing 10 changed files with 440 additions and 303 deletions.
7 changes: 5 additions & 2 deletions chrome/browser/extensions/crx_installer_browsertest.cc
View file
Expand Up @@ -55,6 +55,7 @@
#include "extensions/browser/install/sandboxed_unpacker_failure_reason.h"
#include "extensions/browser/management_policy.h"
#include "extensions/browser/notification_types.h"
#include "extensions/browser/permissions_manager.h"
#include "extensions/browser/renderer_startup_helper.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
Expand Down Expand Up @@ -1140,8 +1141,10 @@ IN_PROC_BROWSER_TEST_P(ExtensionCrxInstallerTestWithWithholdingUI,
GetInstalledExtension(mock_prompt->extension_id());
ScriptingPermissionsModifier modifier(browser()->profile(), extension);
EXPECT_EQ(should_check_box, modifier.HasWithheldHostPermissions());
const ScriptingPermissionsModifier::SiteAccess site_access =
modifier.GetSiteAccess(GURL("https://google.com"));

const PermissionsManager::ExtensionSiteAccess site_access =
PermissionsManager::Get(profile())->GetSiteAccess(
*extension, GURL("https://google.com"));
EXPECT_EQ(should_check_box, site_access.withheld_site_access);
EXPECT_EQ(!should_check_box, site_access.has_site_access);
}
Expand Down
9 changes: 5 additions & 4 deletions chrome/browser/extensions/extension_context_menu_model_unittest.cc
View file
Expand Up @@ -43,6 +43,7 @@
#include "extensions/browser/extension_dialog_auto_confirm.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/permissions_manager.h"
#include "extensions/browser/test_extension_registry_observer.h"
#include "extensions/browser/test_management_policy.h"
#include "extensions/common/api/extension_action/action_info.h"
Expand Down Expand Up @@ -1494,8 +1495,8 @@ TEST_F(ExtensionContextMenuModelTest,
}

{
ScriptingPermissionsModifier::SiteAccess site_access =
modifier.GetSiteAccess(b_com);
PermissionsManager::ExtensionSiteAccess site_access =
PermissionsManager::Get(profile())->GetSiteAccess(*extension, b_com);
EXPECT_FALSE(site_access.has_site_access);
EXPECT_FALSE(site_access.withheld_site_access);
}
Expand Down Expand Up @@ -1547,8 +1548,8 @@ TEST_F(ExtensionContextMenuModelTest,
// TODO(devlin): We should fix that, so that toggling access on a.com doesn't
// revoke access on b.com.
const GURL b_com("https://b.com");
ScriptingPermissionsModifier::SiteAccess site_access =
modifier.GetSiteAccess(b_com);
PermissionsManager::ExtensionSiteAccess site_access =
PermissionsManager::Get(profile())->GetSiteAccess(*extension, b_com);
EXPECT_FALSE(site_access.has_site_access);
EXPECT_TRUE(site_access.withheld_site_access);
}
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/extensions/permissions_updater.cc
View file
Expand Up @@ -533,12 +533,12 @@ void PermissionsUpdater::GrantActivePermissions(const Extension* extension) {
void PermissionsUpdater::InitializePermissions(const Extension* extension) {
std::unique_ptr<const PermissionSet> bounded_wrapper;
const PermissionSet* bounded_active = nullptr;
ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context_);
// If |extension| is a transient dummy extension, we do not want to look for
// it in preferences.
if (init_flag_ & INIT_FLAG_TRANSIENT) {
bounded_active = &extension->permissions_data()->active_permissions();
} else {
ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context_);
std::unique_ptr<const PermissionSet> active_permissions =
prefs->GetActivePermissions(extension->id());
bounded_wrapper =
Expand All @@ -547,8 +547,8 @@ void PermissionsUpdater::InitializePermissions(const Extension* extension) {
}

std::unique_ptr<const PermissionSet> granted_permissions =
ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
*extension, *prefs, *bounded_active);
ScriptingPermissionsModifier(browser_context_, extension)
.WithholdPermissionsIfNecessary(*bounded_active);

if (GetDelegate())
GetDelegate()->InitializePermissions(extension, &granted_permissions);
Expand Down
174 changes: 20 additions & 154 deletions chrome/browser/extensions/scripting_permissions_modifier.cc
View file
Expand Up @@ -11,6 +11,7 @@
#include "extensions/browser/extension_prefs.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_util.h"
#include "extensions/browser/permissions_manager.h"
#include "extensions/common/extension.h"
#include "extensions/common/manifest_handlers/permissions_parser.h"
#include "extensions/common/permissions/permission_set.h"
Expand Down Expand Up @@ -79,77 +80,6 @@ bool ShouldConsiderExtension(const Extension& extension) {
return true;
}

// Retrieves the effective list of runtime-granted permissions for a given
// |extension| from the |prefs|. ExtensionPrefs doesn't store the valid schemes
// for URLPatterns, which results in the chrome:-scheme being included for
// <all_urls> when retrieving it directly from the prefs; this then causes
// CHECKs to fail when validating that permissions being revoked are present
// (see https://crbug.com/930062).
// Returns null if there are no stored runtime-granted permissions.
// TODO(https://crbug.com/931881): ExtensionPrefs should return properly-bounded
// permissions.
std::unique_ptr<const PermissionSet> GetRuntimePermissionsFromPrefs(
const Extension& extension,
const ExtensionPrefs& prefs) {
std::unique_ptr<const PermissionSet> permissions =
prefs.GetRuntimeGrantedPermissions(extension.id());

// If there are no stored permissions, there's nothing to adjust.
if (!permissions)
return nullptr;

// If the extension is allowed to run on chrome:// URLs, then we don't have
// to adjust anything.
if (PermissionsData::AllUrlsIncludesChromeUrls(extension.id()))
return permissions;

// We need to adjust a pattern if it matches all URLs and includes the
// chrome:-scheme. These patterns would otherwise match hosts like
// chrome://settings, which should not be allowed.
// NOTE: We don't need to adjust for the file scheme, because
// ExtensionPrefs properly does that based on the extension's file access.
auto needs_chrome_scheme_adjustment = [](const URLPattern& pattern) {
return pattern.match_all_urls() &&
((pattern.valid_schemes() & URLPattern::SCHEME_CHROMEUI) != 0);
};

// NOTE: We don't need to check scriptable_hosts, because the default
// scriptable_hosts scheme mask omits the chrome:-scheme in normal
// circumstances (whereas the default explicit scheme does not, in order to
// allow for patterns like chrome://favicon).

bool needs_adjustment = std::any_of(permissions->explicit_hosts().begin(),
permissions->explicit_hosts().end(),
needs_chrome_scheme_adjustment);
// If no patterns need adjustment, return the original set.
if (!needs_adjustment)
return permissions;

// Otherwise, iterate over the explicit hosts, and modify any that need to be
// tweaked, adding back in permitted chrome:-scheme hosts. This logic mirrors
// that in PermissionsParser, and is also similar to logic in
// permissions_api_helpers::UnpackOriginPermissions(), and has some overlap
// to URLPatternSet::Populate().
// TODO(devlin): ^^ Ouch. Refactor so that this isn't duplicated.
URLPatternSet new_explicit_hosts;
for (const auto& pattern : permissions->explicit_hosts()) {
if (!needs_chrome_scheme_adjustment(pattern)) {
new_explicit_hosts.AddPattern(pattern);
continue;
}

URLPattern new_pattern(pattern);
int new_valid_schemes =
pattern.valid_schemes() & ~URLPattern::SCHEME_CHROMEUI;
new_pattern.SetValidSchemes(new_valid_schemes);
new_explicit_hosts.AddPattern(std::move(new_pattern));
}

return std::make_unique<PermissionSet>(
permissions->apis().Clone(), permissions->manifest_permissions().Clone(),
std::move(new_explicit_hosts), permissions->scriptable_hosts().Clone());
}

} // namespace

ScriptingPermissionsModifier::ScriptingPermissionsModifier(
Expand Down Expand Up @@ -184,7 +114,8 @@ void ScriptingPermissionsModifier::SetWithholdHostPermissions(
bool ScriptingPermissionsModifier::HasWithheldHostPermissions() const {
DCHECK(CanAffectExtension());

return extension_prefs_->GetWithholdingPermissions(extension_->id());
return PermissionsManager::Get(browser_context_)
->HasWithheldHostPermissions(extension_->id());
}

bool ScriptingPermissionsModifier::CanAffectExtension() const {
Expand All @@ -203,74 +134,6 @@ bool ScriptingPermissionsModifier::CanAffectExtension() const {
.is_empty();
}

ScriptingPermissionsModifier::SiteAccess
ScriptingPermissionsModifier::GetSiteAccess(const GURL& url) const {
SiteAccess access;
ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context_);

// Awkward holder object because permission sets are immutable, and when
// return from prefs, ownership is passed.
std::unique_ptr<const PermissionSet> permission_holder;

const PermissionSet* granted_permissions = nullptr;
if (!HasWithheldHostPermissions()) {
// If the extension doesn't have any withheld permissions, we look at the
// current active permissions.
// TODO(devlin): This is clunky. It would be nice to have runtime-granted
// permissions be correctly populated in all cases, rather than looking at
// two different sets.
// TODO(devlin): This won't account for granted permissions that aren't
// currently active, even though the extension may re-request them (and be
// silently granted them) at any time.
granted_permissions = &extension_->permissions_data()->active_permissions();
} else {
permission_holder = GetRuntimePermissionsFromPrefs(*extension_, *prefs);
granted_permissions = permission_holder.get();
}

DCHECK(granted_permissions);

const bool is_restricted_site =
extension_->permissions_data()->IsRestrictedUrl(url, /*error=*/nullptr);

// For indicating whether an extension has access to a site, we look at the
// granted permissions, which could include patterns that weren't explicitly
// requested. However, we should still indicate they are granted, so that the
// user can revoke them (and because if the extension does request them and
// they are already granted, they are silently added).
// The extension should never have access to restricted sites (even if a
// pattern matches, as it may for e.g. the webstore).
if (!is_restricted_site &&
granted_permissions->effective_hosts().MatchesSecurityOrigin(url)) {
access.has_site_access = true;
}

const PermissionSet& withheld_permissions =
extension_->permissions_data()->withheld_permissions();

// Be sure to check |access.has_site_access| in addition to withheld
// permissions, so that we don't indicate we've withheld permission if an
// extension is granted https://a.com/*, but has *://*/* withheld.
// We similarly don't show access as withheld for restricted sites, since
// withheld permissions should only include those that are conceivably
// grantable.
if (!is_restricted_site && !access.has_site_access &&
withheld_permissions.effective_hosts().MatchesSecurityOrigin(url)) {
access.withheld_site_access = true;
}

constexpr bool include_api_permissions = false;
if (granted_permissions->ShouldWarnAllHosts(include_api_permissions))
access.has_all_sites_access = true;

if (withheld_permissions.ShouldWarnAllHosts(include_api_permissions) &&
!access.has_all_sites_access) {
access.withheld_all_sites_access = true;
}

return access;
}

void ScriptingPermissionsModifier::GrantHostPermission(const GURL& url) {
DCHECK(CanAffectExtension());
// Check that we don't grant host permission to a restricted URL.
Expand All @@ -295,14 +158,14 @@ bool ScriptingPermissionsModifier::HasGrantedHostPermission(
const GURL& url) const {
DCHECK(CanAffectExtension());

return GetRuntimePermissionsFromPrefs(*extension_, *extension_prefs_)
return GetRuntimePermissionsFromPrefs()
->effective_hosts()
.MatchesSecurityOrigin(url);
}

bool ScriptingPermissionsModifier::HasBroadGrantedHostPermissions() {
std::unique_ptr<const PermissionSet> runtime_permissions =
GetRuntimePermissionsFromPrefs(*extension_, *extension_prefs_);
GetRuntimePermissionsFromPrefs();

// Don't consider API permissions in this case.
constexpr bool kIncludeApiPermissions = false;
Expand All @@ -314,9 +177,8 @@ void ScriptingPermissionsModifier::RemoveGrantedHostPermission(
DCHECK(CanAffectExtension());
DCHECK(HasGrantedHostPermission(url));

ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context_);
std::unique_ptr<const PermissionSet> runtime_permissions =
GetRuntimePermissionsFromPrefs(*extension_, *prefs);
GetRuntimePermissionsFromPrefs();

URLPatternSet explicit_hosts;
for (const auto& pattern : runtime_permissions->explicit_hosts()) {
Expand All @@ -341,7 +203,7 @@ void ScriptingPermissionsModifier::RemoveBroadGrantedHostPermissions() {
DCHECK(CanAffectExtension());

std::unique_ptr<const PermissionSet> runtime_permissions =
GetRuntimePermissionsFromPrefs(*extension_, *extension_prefs_);
GetRuntimePermissionsFromPrefs();

URLPatternSet explicit_hosts;
for (const auto& pattern : runtime_permissions->explicit_hosts()) {
Expand Down Expand Up @@ -369,27 +231,25 @@ void ScriptingPermissionsModifier::RemoveAllGrantedHostPermissions() {
WithholdHostPermissions();
}

// static
std::unique_ptr<const PermissionSet>
ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
const Extension& extension,
const ExtensionPrefs& extension_prefs,
const PermissionSet& permissions) {
if (!ShouldConsiderExtension(extension)) {
if (!ShouldConsiderExtension(*extension_)) {
// The withhold creation flag should never have been set in cases where
// withholding isn't allowed.
DCHECK(!(extension.creation_flags() & Extension::WITHHOLD_PERMISSIONS));
DCHECK(!(extension_->creation_flags() & Extension::WITHHOLD_PERMISSIONS));
return permissions.Clone();
}

if (permissions.effective_hosts().is_empty())
return permissions.Clone(); // No hosts to withhold.

bool should_withhold = false;
if (extension.creation_flags() & Extension::WITHHOLD_PERMISSIONS) {
if (extension_->creation_flags() & Extension::WITHHOLD_PERMISSIONS) {
should_withhold = true;
} else {
should_withhold = extension_prefs.GetWithholdingPermissions(extension.id());
should_withhold =
extension_prefs_->GetWithholdingPermissions(extension_->id());
}

if (!should_withhold)
Expand All @@ -399,7 +259,7 @@ ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
// runtime through the runtime host permissions feature or the optional
// permissions API.
std::unique_ptr<const PermissionSet> runtime_granted_permissions =
GetRuntimePermissionsFromPrefs(extension, extension_prefs);
GetRuntimePermissionsFromPrefs();
// If there were no runtime granted permissions found in the prefs, default to
// a new empty set.
if (!runtime_granted_permissions) {
Expand All @@ -424,7 +284,7 @@ ScriptingPermissionsModifier::GetRevokablePermissions() const {
// host permissions.
const PermissionSet* current_granted_permissions = nullptr;
std::unique_ptr<const PermissionSet> runtime_granted_permissions =
GetRuntimePermissionsFromPrefs(*extension_, *extension_prefs_);
GetRuntimePermissionsFromPrefs();
std::unique_ptr<const PermissionSet> union_set;
if (runtime_granted_permissions) {
union_set = PermissionSet::CreateUnion(
Expand Down Expand Up @@ -463,4 +323,10 @@ void ScriptingPermissionsModifier::WithholdHostPermissions() {
base::DoNothing());
}

std::unique_ptr<const PermissionSet>
ScriptingPermissionsModifier::GetRuntimePermissionsFromPrefs() const {
return PermissionsManager::Get(browser_context_)
->GetRuntimePermissionsFromPrefs(*extension_);
}

} // namespace extensions

0 comments on commit 41345b3

Please sign in to comment.