Create withheld permissions

chrome/browser/extensions/active_script_controller.cc

Index: chrome/browser/extensions/active_script_controller.cc
diff --git a/chrome/browser/extensions/active_script_controller.cc b/chrome/browser/extensions/active_script_controller.cc
index 906da8ec1d37f32ccda26e75a42be9f4903a5eb9..0f4e10fd07fe9224a4a4d013f87796bba320d9e3 100644
--- a/chrome/browser/extensions/active_script_controller.cc
+++ b/chrome/browser/extensions/active_script_controller.cc
@@ -25,11 +25,31 @@
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/feature_switch.h"
+#include "extensions/common/manifest.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "ipc/ipc_message_macros.h"
 
 namespace extensions {
 
+namespace {
+
+// Returns true if the extension should be regarded as a "permitted" extension
+// for the case of metrics. We need this because we only actually withhold
+// permissions if the switch is enabled, but want to record metrics in all
+// cases.
+// "ExtensionWouldHaveHadHostPermissionsWithheldIfSwitchWasOn()" would be
+// more accurate, but too long.
+bool ShouldRecordExtension(const Extension* extension) {
+  return extension->ShouldDisplayInExtensionSettings() &&
+         !Manifest::IsPolicyLocation(extension->location()) &&
+         !Manifest::IsComponentLocation(extension->location()) &&
+         !PermissionsData::CanExecuteScriptEverywhere(extension) &&
+         extension->permissions_data()->active_permissions()
+             ->ShouldWarnAllHosts();

[not at google - send to devlin, 2014/06/27 23:24:33]

don't you need to check withheld permissions as well?

[Devlin, 2014/06/30 17:06:09]

No.

This method is the yak shave for being able to get good metrics while having
withheld permissions only actually withheld when the switch is on - it's only
called when we're simply notifying the browser of a script injection, vs asking
permission.  As such, we can know that there are no withheld permissions (at
least, no withheld all hosts).

+}
+
+} // namespace
+
 ActiveScriptController::PendingRequest::PendingRequest() :
     page_id(-1) {
 }
@@ -70,40 +90,6 @@ ActiveScriptController* ActiveScriptController::GetForWebContents(
   return location_bar_controller->active_script_controller();
 }
 
-bool ActiveScriptController::RequiresUserConsentForScriptInjection(
-    const Extension* extension) {
-  CHECK(extension);
-  if (!extension->permissions_data()->RequiresActionForScriptExecution(
-          extension,
-          SessionID::IdForTab(web_contents()),
-          web_contents()->GetVisibleURL()) ||
-      util::AllowedScriptingOnAllUrls(extension->id(),
-                                      web_contents()->GetBrowserContext())) {
-    return false;
-  }
-
-  // If the feature is not enabled, we automatically allow all extensions to
-  // run scripts.
-  if (!enabled_)
-    permitted_extensions_.insert(extension->id());
-
-  return permitted_extensions_.count(extension->id()) == 0;
-}
-
-void ActiveScriptController::RequestScriptInjection(
-    const Extension* extension,
-    int page_id,
-    const base::Closure& callback) {
-  CHECK(extension);
-  PendingRequestList& list = pending_requests_[extension->id()];
-  list.push_back(PendingRequest(callback, page_id));
-
-  // If this was the first entry, notify the location bar that there's a new
-  // icon.
-  if (list.size() == 1u)
-    LocationBarController::NotifyChange(web_contents());
-}
-
 void ActiveScriptController::OnActiveTabPermissionGranted(
     const Extension* extension) {
   RunPendingForExtension(extension);
@@ -174,13 +160,55 @@ void ActiveScriptController::OnExtensionUnloaded(const Extension* extension) {
     pending_requests_.erase(iter);
 }
 
+PermissionsData::AccessType
+ActiveScriptController::RequiresUserConsentForScriptInjection(
+    const Extension* extension,
+    extension_misc::InjectedScriptType type) {
+  CHECK(extension);
+
+  // If the feature is not enabled, we automatically allow all extensions to
+  // run scripts.
+  if (!enabled_)
+    permitted_extensions_.insert(extension->id());
+
+  // Allow the extension if it's been explicitly granted permission.
+  if (permitted_extensions_.count(extension->id()) > 0)
+    return PermissionsData::ALLOW_ACCESS;
+
+  PermissionsData::AccessType access = PermissionsData::DENY_ACCESS;
+  GURL url = web_contents()->GetVisibleURL();
+  int tab_id = SessionID::IdForTab(web_contents());
+  switch (type) {
+    case extension_misc::CONTENT_SCRIPT:
+      access = extension->permissions_data()
+                   ->CanRunContentScriptOnPageWithUserConsent(
+                       extension, url, url, tab_id, -1, NULL);
+      break;
+    case extension_misc::PROGRAMMATIC_SCRIPT:
+      access = extension->permissions_data()->CanAccessPageWithUserConsent(
+          extension, url, url, tab_id, -1, NULL);
+  }

[not at google - send to devlin, 2014/06/27 23:24:33]

nit: I'd prefer to phrase this more succinctly without declaring |type| and by
doing "return extension->permissions_data()-> ... " rather than break.

then a NOTREACED() at the end with a return DENY_ACCESS.

[not at google - send to devlin, 2014/06/30 14:37:03]

I mean "without declaring |access|"

[Devlin, 2014/06/30 17:06:09]

Sure, why not.

+
+  return access;
+}
+
+void ActiveScriptController::RequestScriptInjection(
+    const Extension* extension,
+    int page_id,
+    const base::Closure& callback) {
+  CHECK(extension);
+  PendingRequestList& list = pending_requests_[extension->id()];
+  list.push_back(PendingRequest(callback, page_id));
+
+  // If this was the first entry, notify the location bar that there's a new
+  // icon.
+  if (list.size() == 1u)
+    LocationBarController::NotifyChange(web_contents());
+}
+
 void ActiveScriptController::RunPendingForExtension(
     const Extension* extension) {
   DCHECK(extension);
-  PendingRequestMap::iterator iter =
-      pending_requests_.find(extension->id());
-  if (iter == pending_requests_.end())
-    return;
 
   content::NavigationEntry* visible_entry =
       web_contents()->GetController().GetVisibleEntry();
@@ -195,6 +223,12 @@ void ActiveScriptController::RunPendingForExtension(
   // *before* running them to guard against the crazy case where running the
   // callbacks adds more entries.
   permitted_extensions_.insert(extension->id());
+
+  PendingRequestMap::iterator iter =
+      pending_requests_.find(extension->id());
+  if (iter == pending_requests_.end())
+    return;
+
   PendingRequestList requests;
   iter->second.swap(requests);
   pending_requests_.erase(extension->id());
@@ -221,6 +255,7 @@ void ActiveScriptController::RunPendingForExtension(
 
 void ActiveScriptController::OnRequestScriptInjectionPermission(
     const std::string& extension_id,
+    extension_misc::InjectedScriptType script_type,
     int page_id,
     int request_id) {
   if (!Extension::IdIsValid(extension_id)) {
@@ -240,22 +275,32 @@ void ActiveScriptController::OnRequestScriptInjectionPermission(
   // ran (because this feature is not enabled). Add the extension to the list of
   // permitted extensions (for metrics), and return immediately.
   if (request_id == -1) {
-    DCHECK(!enabled_);
-    permitted_extensions_.insert(extension->id());
+    if (ShouldRecordExtension(extension)) {
+      DCHECK(!enabled_);
+      permitted_extensions_.insert(extension->id());
+    }
     return;
   }
 
-  if (RequiresUserConsentForScriptInjection(extension)) {
-    // This base::Unretained() is safe, because the callback is only invoked by
-    // this object.
-    RequestScriptInjection(
-        extension,
-        page_id,
-        base::Bind(&ActiveScriptController::PermitScriptInjection,
-                   base::Unretained(this),
-                   request_id));
-  } else {
-    PermitScriptInjection(request_id);
+  switch (RequiresUserConsentForScriptInjection(extension, script_type)) {
+    case PermissionsData::ALLOW_ACCESS:
+      PermitScriptInjection(request_id);
+      break;
+    case PermissionsData::REQUEST_ACCESS:
+      // This base::Unretained() is safe, because the callback is only invoked
+      // by this object.
+      RequestScriptInjection(
+          extension,
+          page_id,
+          base::Bind(&ActiveScriptController::PermitScriptInjection,
+                     base::Unretained(this),
+                     request_id));
+      break;
+    case PermissionsData::DENY_ACCESS:
+      // We should usually only get a "deny access" if the page changed (as the
+      // renderer wouldn't have requested permission if the answer was always
+      // "no"). Just let the request fizzle and die.
+      break;
   }
 }