[Extensions] Enable the scripts-require-action feature based on all-urls pref

chrome/browser/extensions/extension_util.cc

Index: chrome/browser/extensions/extension_util.cc
diff --git a/chrome/browser/extensions/extension_util.cc b/chrome/browser/extensions/extension_util.cc
index b07ab1ca1aacc30643ac3faecddc0baa1e75c4e3..046b9440b1e74d85eb773956e496d1e11250db9d 100644
--- a/chrome/browser/extensions/extension_util.cc
+++ b/chrome/browser/extensions/extension_util.cc
@@ -70,6 +70,32 @@ std::string ReloadExtensionIfEnabled(const std::string& extension_id,
   return id;
 }
 
+// Sets the preference for scripting on all urls to |allowed|, optionally
+// updating the extension's active permissions (based on |update_permissions|).
+void SetAllowedScriptingOnAllUrlsHelper(
+    content::BrowserContext* context,
+    const std::string& extension_id,
+    bool allowed,
+    bool update_permissions) {
+  ExtensionPrefs::Get(context)->UpdateExtensionPref(
+      extension_id,
+      kExtensionAllowedOnAllUrlsPrefName,
+      new base::FundamentalValue(allowed));

[not at google - send to devlin, 2015/02/06 00:26:55]

So that the prefs delete the value if false, use (allowed ? new
base::FundamentalValue(true) : NULL)

[Devlin, 2015/02/06 18:58:43]

As discussed offline, we actually need this pref to have a value (to indicate
whether the switch was on/off at installation).
Added a TODO to change it to null once the switch is on by default.

+
+  if (update_permissions) {
+    const Extension* extension =
+        ExtensionRegistry::Get(context)->enabled_extensions().GetByID(
+            extension_id);
+    if (extension) {
+      PermissionsUpdater updater(context);
+      if (allowed)
+        updater.GrantWithheldImpliedAllHosts(extension);
+      else
+        updater.WithholdImpliedAllHosts(extension);
+    }
+  }
+}
+
 }  // namespace
 
 bool IsIncognitoEnabled(const std::string& extension_id,
@@ -176,11 +202,19 @@ void SetAllowFileAccess(const std::string& extension_id,
 bool AllowedScriptingOnAllUrls(const std::string& extension_id,
                                content::BrowserContext* context) {
   bool allowed = false;
-  return ExtensionPrefs::Get(context)->ReadPrefAsBoolean(
-             extension_id,
-             kExtensionAllowedOnAllUrlsPrefName,
-             &allowed) &&
-         allowed;
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(context);
+  if (!prefs->ReadPrefAsBoolean(extension_id,
+                                kExtensionAllowedOnAllUrlsPrefName,
+                                &allowed)) {
+    // If there is no value present, we make one, defaulting it to the value of
+    // the 'scripts require action' flag. If the flag is on, then the extension
+    // does not have permission to script on all urls by default.
+    bool default_value = DefaultAllowedScriptingOnAllUrls();

[not at google - send to devlin, 2015/02/06 00:26:55]

sorry I'm in a nit mood. Just assign to |allowed|.

[Devlin, 2015/02/06 18:58:43]

Done.

+    SetAllowedScriptingOnAllUrlsHelper(
+        context, extension_id, default_value, false);
+    allowed = default_value;
+  }
+  return allowed;
 }
 
 void SetAllowedScriptingOnAllUrls(const std::string& extension_id,
@@ -189,33 +223,25 @@ void SetAllowedScriptingOnAllUrls(const std::string& extension_id,
   if (allowed == AllowedScriptingOnAllUrls(extension_id, context))
     return;  // Nothing to do here.
 
-  ExtensionPrefs::Get(context)->UpdateExtensionPref(
-      extension_id,
-      kExtensionAllowedOnAllUrlsPrefName,
-      allowed ? new base::FundamentalValue(true) : NULL);
+  SetAllowedScriptingOnAllUrlsHelper(context, extension_id, allowed, true);

[not at google - send to devlin, 2015/02/06 00:26:55]

now that you have this helper, it would be nice to write this method like

if (allowed != Allowed..)
  SetAllowed...

though I don't know if that check is worth it anyway. It's not like this
function is going to be called all that much, and it's a bit hard to reason
about now that AllowedScriptingOnAllUrls has sideeffects.

[Devlin, 2015/02/06 18:58:43]

Thanks for the catch - I usually cringe at those useless returns, but somehow
missed this one.

Re the check itself, it's definitely worthwhile.  Two big reasons:
- hypothetical: someone is observing the pref for changes/modifications. No
reason to alert them if there is none.
- practical: very shortly, we need to sync these changes. No reason to trigger a
sync if there's no change.

+}
 
-  const Extension* extension =
-      ExtensionRegistry::Get(context)->enabled_extensions().GetByID(
-          extension_id);
-  if (extension) {
-    PermissionsUpdater updater(context);
-    if (allowed)
-      updater.GrantWithheldImpliedAllHosts(extension);
-    else
-      updater.WithholdImpliedAllHosts(extension);
-  }
+bool DefaultAllowedScriptingOnAllUrls() {
+  return !FeatureSwitch::scripts_require_action()->IsEnabled();
 }
 
-bool ScriptsMayRequireActionForExtension(const Extension* extension) {
-  // An extension requires user action to execute scripts iff the switch to do
-  // so is enabled, the extension shows up in chrome:extensions (so the user can
-  // grant withheld permissions), the extension is not part of chrome or
-  // corporate policy, and also not on the scripting whitelist.
-  return FeatureSwitch::scripts_require_action()->IsEnabled() &&
-      extension->ShouldDisplayInExtensionSettings() &&
+bool ScriptsMayRequireActionForExtension(
+    const Extension* extension,
+    const scoped_refptr<const PermissionSet>& permissions) {
+  // An extension may require user action to execute scripts iff the extension
+  // shows up in chrome:extensions (so the user can grant withheld permissions),
+  // is not part of chrome or corporate policy, not on the scripting whitelist,
+  // and requires enough permissions that we should withhold them.
+  return extension->ShouldDisplayInExtensionSettings() &&
       !Manifest::IsPolicyLocation(extension->location()) &&
       !Manifest::IsComponentLocation(extension->location()) &&
-      !PermissionsData::CanExecuteScriptEverywhere(extension);
+      !PermissionsData::CanExecuteScriptEverywhere(extension) &&
+      permissions->ShouldWarnAllHosts();
 }
 
 bool IsAppLaunchable(const std::string& extension_id,