Support "always allow" for runtime script execution

chrome/browser/extensions/active_script_controller_unittest.cc

 // Copyright 2014 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 <map>
 
 #include "base/values.h"
 #include "chrome/browser/extensions/active_script_controller.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 24 matching lines @@
 // Unittests for the ActiveScriptController mostly test the internal logic
 // of the controller itself (when to allow/deny extension script injection).
 // Testing real injection is allowed/denied as expected (i.e., that the
 // ActiveScriptController correctly interfaces in the system) is done in the
 // ActiveScriptControllerBrowserTests.
 class ActiveScriptControllerUnitTest : public ChromeRenderViewHostTestHarness {
  protected:
   ActiveScriptControllerUnitTest();
   virtual ~ActiveScriptControllerUnitTest();
 
-  // Creates an extension with all hosts permission and adds it to the registry.
+  // Creates an extension with a generated id and adds it to the registry.

[Devlin, 2014/08/14 22:01:11]

// Creates an extension all-hosts permission (and optionally with |id|),
// and adds it to the registry.
const Extension* AddExtension();
const Extension* AddExtension(const std::string& id);

Or:
Just make a scoped_refptr<const Extension> extension_ member field, and have
AddExtension() and ReloadExtension().

[gpdavis, 2014/08/14 23:31:33]

I like the alternative better since we never need more than one extension at a
time.  Done.

   const Extension* AddExtension();
 
+  // Creates an extension with |id| and all host permission and adds it to the
+  // registry.
+  const Extension* AddExtension(const std::string& id);
+
+  // Reloads the extension with |id| by removing it from the registry and
+  // recreating a new extension with the same |id|.
+  const Extension* ReloadExtension(const std::string& id);
+
   // Returns true if the |extension| requires user consent before injecting
   // a script.
   bool RequiresUserConsent(const Extension* extension) const;
 
   // Request an injection for the given |extension|.
   void RequestInjection(const Extension* extension);
 
+  // Simulate clicking "always run" menu item for |extension|.

[Devlin, 2014/08/14 22:01:11]

My issue here is with the comment.  ActiveScriptController does not (and should
not) necessarily know that this has to happen via a click.  What if
developerPrivate has an "alwaysRun()" function?  What if we use this elsewhere
for component or trusted extensions in the future?  This should simply say
something like "Allow the extension to always run on this host."

[gpdavis, 2014/08/14 23:31:33]

Oh, I see-- I was confused by the wording because you said to abstract it away. 
Done.

+  void AlwaysRun(const Extension* extension);
+
   // Returns the number of times a given extension has had a script execute.
   size_t GetExecutionCountForExtension(const std::string& extension_id) const;
 
   ActiveScriptController* controller() const {
     return active_script_controller_;
   }
 
  private:
   // Returns a closure to use as a script execution for a given extension.
   base::Closure GetExecutionCallbackForExtension(
@@ skipping 18 matching lines @@
 ActiveScriptControllerUnitTest::ActiveScriptControllerUnitTest()
     : feature_override_(FeatureSwitch::scripts_require_action(),
                         FeatureSwitch::OVERRIDE_ENABLED),
       active_script_controller_(NULL) {
 }
 
 ActiveScriptControllerUnitTest::~ActiveScriptControllerUnitTest() {
 }
 
 const Extension* ActiveScriptControllerUnitTest::AddExtension() {
-  const std::string kId = id_util::GenerateId("all_hosts_extension");
+  return AddExtension(id_util::GenerateId("all_hosts_extension"));
+}
+
+const Extension* ActiveScriptControllerUnitTest::AddExtension(
+    const std::string& id) {
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
           .SetManifest(
               DictionaryBuilder()
                   .Set("name", "all_hosts_extension")
                   .Set("description", "an extension")
                   .Set("manifest_version", 2)
                   .Set("version", "1.0.0")
                   .Set("permissions",
                        ListBuilder().Append(kAllHostsPermission)))
           .SetLocation(Manifest::INTERNAL)
-          .SetID(kId)
+          .SetID(id)
           .Build();
 
   ExtensionRegistry::Get(profile())->AddEnabled(extension);
   PermissionsUpdater(profile()).InitializePermissions(extension);
   return extension;
 }
 
+const Extension* ActiveScriptControllerUnitTest::ReloadExtension(
+    const std::string& id) {
+  std::string extension_id(id);

[not at google - send to devlin, 2014/08/14 21:03:55]

nit: prefer " = id" here.

[gpdavis, 2014/08/14 21:59:52]

Done.

+  ExtensionRegistry::Get(profile())->RemoveEnabled(extension_id);
+  return AddExtension(extension_id);
+}
+
 bool ActiveScriptControllerUnitTest::RequiresUserConsent(
     const Extension* extension) const {
   PermissionsData::AccessType access_type =
       controller()->RequiresUserConsentForScriptInjectionForTesting(
           extension, UserScript::PROGRAMMATIC_SCRIPT);
   // We should never downright refuse access in these tests.
   DCHECK_NE(PermissionsData::ACCESS_DENIED, access_type);
   return access_type == PermissionsData::ACCESS_WITHHELD;
 }
 
 void ActiveScriptControllerUnitTest::RequestInjection(
     const Extension* extension) {
   controller()->RequestScriptInjectionForTesting(
       extension,
       GetExecutionCallbackForExtension(extension->id()));
 }
 
+void ActiveScriptControllerUnitTest::AlwaysRun(const Extension* extension) {

[Devlin, 2014/08/14 22:01:11]

May as well just type this out in the one test that uses it.

[gpdavis, 2014/08/14 23:31:33]

Done.

+  controller()->AlwaysRunOnVisibleHost(extension);
+}
+
 size_t ActiveScriptControllerUnitTest::GetExecutionCountForExtension(
     const std::string& extension_id) const {
   std::map<std::string, int>::const_iterator iter =
       extension_executions_.find(extension_id);
   if (iter != extension_executions_.end())
     return iter->second;
   return 0u;
 }
 
 base::Closure ActiveScriptControllerUnitTest::GetExecutionCallbackForExtension(
@@ skipping 175 matching lines @@
 
   // Turning off the preference should have instant effect.
   util::SetAllowedScriptingOnAllUrls(extension->id(), profile(), false);
   EXPECT_TRUE(RequiresUserConsent(extension));
 
   // And should also persist across navigations and websites.
   NavigateAndCommit(GURL("http://www.bar.com"));
   EXPECT_TRUE(RequiresUserConsent(extension));
 }
 
+TEST_F(ActiveScriptControllerUnitTest, TestAlwaysRun) {
+  const Extension* extension = AddExtension();
+  ASSERT_TRUE(extension);
+
+  NavigateAndCommit(GURL("https://www.google.com/?gws_rd=ssl"));
+
+  // Ensure that there aren't any executions pending.
+  ASSERT_EQ(0u, GetExecutionCountForExtension(extension->id()));
+  ASSERT_FALSE(controller()->GetActionForExtension(extension));
+
+  // Since the extension requests all_hosts, we should require user consent.
+  EXPECT_TRUE(RequiresUserConsent(extension));
+
+  // Request an injection. There should be an action visible, but no executions.
+  RequestInjection(extension);
+  EXPECT_TRUE(controller()->GetActionForExtension(extension));
+  EXPECT_EQ(0u, GetExecutionCountForExtension(extension->id()));
+
+  AlwaysRun(extension);
+
+  // The extension should execute, and the action should go away.
+  EXPECT_EQ(1u, GetExecutionCountForExtension(extension->id()));
+  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+
+  // Since we already executed on the given page, we shouldn't need permission
+  // for a second time.
+  EXPECT_FALSE(RequiresUserConsent(extension));
+
+  // Navigating to another site that hasn't been granted a persisted permission
+  // should necessitate user consent.
+  NavigateAndCommit(GURL("https://www.foo.com/bar"));
+  EXPECT_TRUE(RequiresUserConsent(extension));
+
+  // We shouldn't need user permission upon returning to the original host.
+  NavigateAndCommit(GURL("https://www.google.com/foo/bar"));
+  EXPECT_FALSE(RequiresUserConsent(extension));
+
+  // Reloading the extension should clear active permissions, but not persisted
+  // permissions.
+  extension = ReloadExtension(extension->id());
+  Reload();
+  EXPECT_FALSE(RequiresUserConsent(extension));

[not at google - send to devlin, 2014/08/14 21:03:55]

Also test navigating to a site that wasn't always-allowed and check that it
hasn't been allowed.

[gpdavis, 2014/08/14 21:59:52]

Done.

+}
+
 }  // namespace extensions