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

chrome/browser/extensions/extension_util.cc

 // Copyright 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 "chrome/browser/extensions/extension_util.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ skipping 52 matching lines @@
   // When we reload the extension the ID may be invalidated if we've passed it
   // by const ref everywhere. Make a copy to be safe. http://crbug.com/103762
   std::string id = extension_id;
   ExtensionService* service =
       ExtensionSystem::Get(context)->extension_service();
   CHECK(service);
   service->ReloadExtension(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,
                         content::BrowserContext* context) {
   const Extension* extension = ExtensionRegistry::Get(context)->
       GetExtensionById(extension_id, ExtensionRegistry::ENABLED);
   if (extension) {
     if (!extension->can_be_incognito_enabled())
       return false;
     // If this is an existing component extension we always allow it to
@@ skipping 86 matching lines @@
     return;
 
   ExtensionPrefs::Get(context)->SetAllowFileAccess(extension_id, allow);
 
   ReloadExtensionIfEnabled(extension_id, context);
 }
 
 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,
                                   content::BrowserContext* context,
                                   bool allowed) {
   if (allowed == AllowedScriptingOnAllUrls(extension_id, context))
     return;  // Nothing to do here.
 
-  ExtensionPrefs::Get(context)->UpdateExtensionPref(
-      extension_id,
-      kExtensionAllowedOnAllUrlsPrefName,
-      allowed ? new base::FundamentalValue(true) : NULL);
-
-  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);
-  }
+  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.

 }
 
-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 DefaultAllowedScriptingOnAllUrls() {
+  return !FeatureSwitch::scripts_require_action()->IsEnabled();
+}
+
+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,
                      content::BrowserContext* context) {
   int reason = ExtensionPrefs::Get(context)->GetDisableReasons(extension_id);
   return !((reason & Extension::DISABLE_UNSUPPORTED_REQUIREMENT) ||
            (reason & Extension::DISABLE_CORRUPTED));
 }
 
 bool IsAppLaunchableWithoutEnabling(const std::string& extension_id,
@@ skipping 120 matching lines @@
       IDR_EXTENSION_DEFAULT_ICON);
 }
 
 bool IsNewBookmarkAppsEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableNewBookmarkApps);
 }
 
 }  // namespace util
 }  // namespace extensions