Make ActiveScriptController use Active Tab-style permissions

extensions/common/permissions/permissions_data.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/common/permissions/permissions_data.h"
 
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 324 matching lines @@
 
 // static
 void PermissionsData::ClearTabSpecificPermissions(
     const Extension* extension,
     int tab_id) {
   CHECK_GE(tab_id, 0);
   extension->permissions_data()->tab_specific_permissions_.erase(tab_id);
 }
 
 // static
+bool PermissionsData::HasTabSpecificPermission(const Extension* extension,
+                                               int tab_id,
+                                               const GURL& url) {
+  if (tab_id >= 0) {
+    scoped_refptr<const PermissionSet> tab_permissions =
+        GetTabSpecificPermissions(extension, tab_id);
+    if (tab_permissions.get() &&
+        tab_permissions->explicit_hosts().MatchesSecurityOrigin(url)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+// static
 bool PermissionsData::HasAPIPermission(const Extension* extension,
                                        APIPermission::ID permission) {
   base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
   return GetActivePermissions(extension)->HasAPIPermission(permission);
 }
 
 // static
 bool PermissionsData::HasAPIPermission(
     const Extension* extension,
     const std::string& permission_name) {
@@ skipping 52 matching lines @@
   return GetActivePermissions(extension)->HasExplicitAccessToOrigin(url);
 }
 
 // static
 bool PermissionsData::HasEffectiveAccessToAllHosts(const Extension* extension) {
   base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
   return GetActivePermissions(extension)->HasEffectiveAccessToAllHosts();
 }
 
 // static
+bool PermissionsData::HasAccessToMostHosts(const Extension* extension) {
+  base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
+  return GetActivePermissions(extension)->HasAccessToMostHosts();
+}
+
+// static
 PermissionMessages PermissionsData::GetPermissionMessages(
     const Extension* extension) {
   base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
   if (ShouldSkipPermissionWarnings(extension)) {
     return PermissionMessages();
   } else {
     return PermissionMessageProvider::Get()->GetPermissionMessages(
         GetActivePermissions(extension), extension->GetType());
   }
 }
@@ skipping 56 matching lines @@
 
   if (top_frame_url.SchemeIs(extensions::kExtensionScheme) &&
       top_frame_url.GetOrigin() !=
           Extension::GetBaseURLFromExtensionId(extension->id()).GetOrigin() &&
       !can_execute_everywhere) {
     if (error)
       *error = errors::kCannotAccessExtensionUrl;
     return false;
   }
 
-  // If a tab ID is specified, try the tab-specific permissions.
-  if (tab_id >= 0) {
-    scoped_refptr<const PermissionSet> tab_permissions =
-        GetTabSpecificPermissions(extension, tab_id);
-    if (tab_permissions.get() &&
-        tab_permissions->explicit_hosts().MatchesSecurityOrigin(document_url)) {
-      return true;
-    }
-  }
+  if (HasTabSpecificPermission(extension, tab_id, document_url))

[not at google - send to devlin, 2014/05/21 20:10:20]

and "HasTabSpecificPermission" doesn't imply to me what that permission actually
is. more accurate would be CanExecuteScriptOnPageForTab or similar.

[Devlin, 2014/05/21 23:16:07]

My worry is making it clear that this is only asking if there are active tab
permissions for that page/tab, not whether or not it could generically execute
(there are times when this would return false, but it can execute script).  How
about... HasTabSpecificScriptPermission()?  Or, if it needs the extra word,
HasTabSpecificScriptExecutionPermission()?

[not at google - send to devlin, 2014/05/21 23:33:45]

or HasTabSpecificPermissionToExecuteScript

times like these you wonder if it's the wrong abstraction.

[Devlin, 2014/05/22 15:52:14]

Done.

+    return true;
 
   bool can_access = false;
 
   if (script) {
     // If a script is specified, use its matches.
     can_access = script->MatchesURL(document_url);
   } else {
     // Otherwise, see if this extension has permission to execute script
     // programmatically on pages.
     can_access = GetActivePermissions(extension)->
@@ skipping 43 matching lines @@
     return false;
   }
 
   if (error)
     *error = errors::kAllURLOrActiveTabNeeded;
   return false;
 }
 
 // static
 bool PermissionsData::RequiresActionForScriptExecution(
-    const Extension* extension) {
+    const Extension* extension,
+    int tab_id,
+    const GURL& document_url) {

[not at google - send to devlin, 2014/05/21 20:10:20]

"document URL" isn't right, it's actually top level URL that this function cares
about.

but I'm not sure it should really care either way, so perhaps "url".

[Devlin, 2014/05/21 23:16:07]

Done.

   // For now, the user should be notified when an extension with all hosts
-  // permission tries to execute a script on a page. Exceptions for policy-
-  // enabled and component extensions.
-  return extension->ShouldDisplayInExtensionSettings() &&
-         !Manifest::IsPolicyLocation(extension->location()) &&
-         !Manifest::IsComponentLocation(extension->location()) &&
-         HasEffectiveAccessToAllHosts(extension);
+  // permission tries to execute a script on a page, with exceptions for policy-
+  // enabled and component extensions. If this doesn't meet those criteria,
+  // return immediately.
+  if (!extension->ShouldDisplayInExtensionSettings() ||
+      Manifest::IsPolicyLocation(extension->location()) ||
+      Manifest::IsComponentLocation(extension->location()) ||

[not at google - send to devlin, 2014/05/21 20:10:20]

the ComponentLocation thing is actually unnecessary,
SholudDisplayInExtensionSettings does it.

[Devlin, 2014/05/21 23:16:07]

Yes, but that's using internal logic from ShouldDisplayInExtensionSettings(),
and I think they're two different reasons to allow.  What if we someday show
component extensions in chrome:extensions?  (I think there's already discussion
for this as-is).  I think I'd rather keep the condition, given it's all of a one
byte comparison.  But if you feel strongly, I can take it out.

[not at google - send to devlin, 2014/05/21 23:33:45]

Leaving in is fine.

I think we're going to want 2 separate functions here eventually, one which
takes the tab ID / document URL and one which is just the old version, to know
when to show the [*] all URLs checkbox.

+      !HasAccessToMostHosts(extension)) {
+    return false;
+  }
+
+  // If the extension has explicit permission to run on the given tab, then
+  // we don't need to alert the user.
+  if (HasTabSpecificPermission(extension, tab_id, document_url))
+    return false;
+
+  return true;
 }
 
 bool PermissionsData::ParsePermissions(Extension* extension,
                                        base::string16* error) {
   initial_required_permissions_.reset(new InitialPermissions);
   if (!ParseHelper(extension,
                    keys::kPermissions,
                    &initial_required_permissions_->api_permissions,
                    &initial_required_permissions_->host_permissions,
                    error)) {
@@ skipping 34 matching lines @@
       initial_optional_permissions_->api_permissions,
       initial_optional_permissions_->manifest_permissions,
       initial_optional_permissions_->host_permissions,
       URLPatternSet());
 
   initial_required_permissions_.reset();
   initial_optional_permissions_.reset();
 }
 
 }  // namespace extensions