[Extensions] Factor Extension out of CanExecuteScriptEverywhere()

Factor the Extension object out of PermissionsData::CanExecuteScriptEverywhere(), instead passing in the extension ID and location directly. Also clean up a few duplicate calls to the method by locally caching the result. Bug: 842270 Change-Id: I434a3df769d32d9f63e2e77bc393d634dcdba4da Reviewed-on: https://chromium-review.googlesource.com/1056036 Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#558746}
Denger-Network · May 15, 2018 · f2173e4 · f2173e4
1 parent 87a5c00
commit f2173e4
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 30 deletions.
diff --git a/chrome/browser/extensions/scripting_permissions_modifier.cc b/chrome/browser/extensions/scripting_permissions_modifier.cc
@@ -48,7 +48,8 @@ bool ExtensionMustBeAllowedOnAllUrls(const Extension& extension) {
   return !extension.ShouldDisplayInExtensionSettings() ||
          Manifest::IsPolicyLocation(extension.location()) ||
          Manifest::IsComponentLocation(extension.location()) ||
-         PermissionsData::CanExecuteScriptEverywhere(&extension);
+         PermissionsData::CanExecuteScriptEverywhere(extension.id(),
+                                                     extension.location());
 }
 
 // Sets the preference for whether the extension with |id| is allowed to execute

diff --git a/chrome/common/extensions/permissions/permissions_data_unittest.cc b/chrome/common/extensions/permissions/permissions_data_unittest.cc
@@ -125,8 +125,8 @@ void CheckRestrictedUrls(const Extension* extension,
   // We should only allow other schemes for extensions when it's a whitelisted
   // extension.
   error.clear();
-  bool allow_on_other_schemes =
-      PermissionsData::CanExecuteScriptEverywhere(extension);
+  bool allow_on_other_schemes = PermissionsData::CanExecuteScriptEverywhere(
+      extension->id(), extension->location());
   EXPECT_EQ(!allow_on_other_schemes,
             PermissionsData::IsRestrictedUrl(
                 invalid_url, extension, &error)) << name;

diff --git a/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc b/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
@@ -146,11 +146,9 @@ std::unique_ptr<extensions::UserScript> ParseContentScript(
 
   // The default for WebUI is not having special access, but we can change that
   // if needed.
-  bool allowed_everywhere = false;
-  if (extension &&
-      extensions::PermissionsData::CanExecuteScriptEverywhere(extension))
-    allowed_everywhere = true;
-
+  bool allowed_everywhere =
+      extension && extensions::PermissionsData::CanExecuteScriptEverywhere(
+                       extension->id(), extension->location());
   for (const std::string& match : script_value.matches) {
     URLPattern pattern(UserScript::ValidUserScriptSchemes(allowed_everywhere));
     if (pattern.Parse(match) != URLPattern::PARSE_SUCCESS) {

diff --git a/extensions/common/manifest_handlers/content_scripts_handler.cc b/extensions/common/manifest_handlers/content_scripts_handler.cc
@@ -137,6 +137,13 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
         errors::kInvalidMatchCount, base::IntToString(definition_index));
     return nullptr;
   }
+
+  const bool can_execute_script_everywhere =
+      PermissionsData::CanExecuteScriptEverywhere(extension->id(),
+                                                  extension->location());
+  const int valid_schemes =
+      UserScript::ValidUserScriptSchemes(can_execute_script_everywhere);
+
   for (size_t j = 0; j < matches->GetSize(); ++j) {
     std::string match_str;
     if (!matches->GetString(j, &match_str)) {
@@ -146,8 +153,7 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
       return nullptr;
     }
 
-    URLPattern pattern(UserScript::ValidUserScriptSchemes(
-        PermissionsData::CanExecuteScriptEverywhere(extension)));
+    URLPattern pattern(valid_schemes);
 
     URLPattern::ParseResult parse_result = pattern.Parse(match_str);
     if (parse_result != URLPattern::PARSE_SUCCESS) {
@@ -159,7 +165,7 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
     }
 
     // TODO(aboxhall): check for webstore
-    if (!PermissionsData::CanExecuteScriptEverywhere(extension) &&
+    if (!can_execute_script_everywhere &&
         pattern.scheme() != content::kChromeUIScheme) {
       // Exclude SCHEME_CHROMEUI unless it's been explicitly requested.
       // If the --extensions-on-chrome-urls flag has not been passed, requesting
@@ -170,7 +176,7 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
     }
 
     if (pattern.MatchesScheme(url::kFileScheme) &&
-        !PermissionsData::CanExecuteScriptEverywhere(extension)) {
+        !can_execute_script_everywhere) {
       extension->set_wants_file_access(true);
       if (!(extension->creation_flags() & Extension::ALLOW_FILE_ACCESS)) {
         pattern.SetValidSchemes(pattern.valid_schemes() &
@@ -199,8 +205,6 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
         return nullptr;
       }
 
-      int valid_schemes = UserScript::ValidUserScriptSchemes(
-          PermissionsData::CanExecuteScriptEverywhere(extension));
       URLPattern pattern(valid_schemes);
 
       URLPattern::ParseResult parse_result = pattern.Parse(match_str);

diff --git a/extensions/common/manifest_handlers/permissions_parser.cc b/extensions/common/manifest_handlers/permissions_parser.cc
@@ -61,8 +61,10 @@ bool CanSpecifyHostPermission(const Extension* extension,
       return true;
 
     // Component extensions can have access to all of chrome://*.
-    if (PermissionsData::CanExecuteScriptEverywhere(extension))
+    if (PermissionsData::CanExecuteScriptEverywhere(extension->id(),
+                                                    extension->location())) {
       return true;
+    }
 
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kExtensionsOnChromeURLs)) {
@@ -157,11 +159,14 @@ bool ParseHelper(Extension* extension,
     api_permissions->erase(*iter);
   }
 
+  bool can_execute_script_everywhere =
+      PermissionsData::CanExecuteScriptEverywhere(extension->id(),
+                                                  extension->location());
+
   // Parse host pattern permissions.
-  const int kAllowedSchemes =
-      PermissionsData::CanExecuteScriptEverywhere(extension)
-          ? URLPattern::SCHEME_ALL
-          : Extension::kValidHostPermissionSchemes;
+  const int kAllowedSchemes = can_execute_script_everywhere
+                                  ? URLPattern::SCHEME_ALL
+                                  : Extension::kValidHostPermissionSchemes;
 
   for (std::vector<std::string>::const_iterator iter = host_data.begin();
        iter != host_data.end();
@@ -177,16 +182,16 @@ bool ParseHelper(Extension* extension,
       pattern.SetPath("/*");
       int valid_schemes = pattern.valid_schemes();
       if (pattern.MatchesScheme(url::kFileScheme) &&
-          !PermissionsData::CanExecuteScriptEverywhere(extension)) {
+          !can_execute_script_everywhere) {
         extension->set_wants_file_access(true);
         if (!(extension->creation_flags() & Extension::ALLOW_FILE_ACCESS))
           valid_schemes &= ~URLPattern::SCHEME_FILE;
       }
 
       if (pattern.scheme() != content::kChromeUIScheme &&
-          !PermissionsData::CanExecuteScriptEverywhere(extension)) {
+          !can_execute_script_everywhere) {
         // Keep chrome:// in allowed schemes only if it's explicitly requested
-        // or CanExecuteScriptEverywhere is true. If the
+        // or can_execute_script_everywhere is true. If the
         // extensions_on_chrome_urls flag is not set, CanSpecifyHostPermission
         // will fail, so don't check the flag here.
         valid_schemes &= ~URLPattern::SCHEME_CHROMEUI;

diff --git a/extensions/common/permissions/permissions_data.cc b/extensions/common/permissions/permissions_data.cc
@@ -77,14 +77,16 @@ void PermissionsData::SetPolicyDelegate(PolicyDelegate* delegate) {
 }
 
 // static
-bool PermissionsData::CanExecuteScriptEverywhere(const Extension* extension) {
-  if (extension->location() == Manifest::COMPONENT)
+bool PermissionsData::CanExecuteScriptEverywhere(
+    const ExtensionId& extension_id,
+    Manifest::Location location) {
+  if (location == Manifest::COMPONENT)
     return true;
 
   const ExtensionsClient::ScriptingWhitelist& whitelist =
       ExtensionsClient::Get()->GetScriptingWhitelist();
 
-  return base::ContainsValue(whitelist, extension->id());
+  return base::ContainsValue(whitelist, extension_id);
 }
 
 // static
@@ -98,8 +100,10 @@ bool PermissionsData::ShouldSkipPermissionWarnings(
 bool PermissionsData::IsRestrictedUrl(const GURL& document_url,
                                       const Extension* extension,
                                       std::string* error) {
-  if (extension && CanExecuteScriptEverywhere(extension))
+  if (extension &&
+      CanExecuteScriptEverywhere(extension->id(), extension->location())) {
     return false;
+  }
 
   if (g_policy_delegate &&
       g_policy_delegate->IsRestrictedUrl(document_url, error)) {

diff --git a/extensions/common/permissions/permissions_data.h b/extensions/common/permissions/permissions_data.h
@@ -15,6 +15,7 @@
 #include "base/strings/string16.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
+#include "extensions/common/extension_id.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permission_message.h"
@@ -71,7 +72,10 @@ class PermissionsData {
 
   // Returns true if the extension is a COMPONENT extension or is on the
   // whitelist of extensions that can script all pages.
-  static bool CanExecuteScriptEverywhere(const Extension* extension);
+  // NOTE: This is static because it is used during extension initialization,
+  // before the extension has an associated PermissionsData object.
+  static bool CanExecuteScriptEverywhere(const ExtensionId& extension_id,
+                                         Manifest::Location location);
 
   // Returns true if we should skip the permissions warning for the extension
   // with the given |extension_id|.

diff --git a/extensions/renderer/extension_injection_host.cc b/extensions/renderer/extension_injection_host.cc
@@ -55,8 +55,10 @@ PermissionsData::PageAccess ExtensionInjectionHost::CanExecuteOnFrame(
   // Only whitelisted extensions may run scripts on another extension's page.
   if (top_frame_security_origin.Protocol().Utf8() == kExtensionScheme &&
       top_frame_security_origin.Host().Utf8() != extension_->id() &&
-      !PermissionsData::CanExecuteScriptEverywhere(extension_))
+      !PermissionsData::CanExecuteScriptEverywhere(extension_->id(),
+                                                   extension_->location())) {
     return PermissionsData::PageAccess::kDenied;
+  }
 
   // Declarative user scripts use "page access" (from "permissions" section in
   // manifest) whereas non-declarative user scripts use custom

diff --git a/extensions/renderer/user_script_set.cc b/extensions/renderer/user_script_set.cc
@@ -160,8 +160,8 @@ bool UserScriptSet::UpdateUserScripts(base::SharedMemoryHandle shared_memory,
     const Extension* extension =
         RendererExtensionRegistry::Get()->GetByID(script->extension_id());
     if (whitelisted_only &&
-        (!extension ||
-         !PermissionsData::CanExecuteScriptEverywhere(extension))) {
+        (!extension || !PermissionsData::CanExecuteScriptEverywhere(
+                           extension->id(), extension->location()))) {
       continue;
     }