Support "always allow" for runtime script execution

chrome/browser/extensions/active_script_controller_unittest.cc

Index: chrome/browser/extensions/active_script_controller_unittest.cc
diff --git a/chrome/browser/extensions/active_script_controller_unittest.cc b/chrome/browser/extensions/active_script_controller_unittest.cc
index 01fe26ba0cf72bcc319482f4dd6684c120a1d75c..e602076a13f02e21865c680c87d9b150998a0879 100644
--- a/chrome/browser/extensions/active_script_controller_unittest.cc
+++ b/chrome/browser/extensions/active_script_controller_unittest.cc
@@ -42,9 +42,17 @@ class ActiveScriptControllerUnitTest : public ChromeRenderViewHostTestHarness {
   ActiveScriptControllerUnitTest();
   virtual ~ActiveScriptControllerUnitTest();
 
-  // Creates an extension with all hosts permission and adds it to the registry.
+  // Adds an extension with a generated id.
   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);

[Devlin, 2014/08/14 01:20:18]

const &

+
+  // Reloads the extension with |id| by removing it from the registry and
+  // readding it.
+  const Extension* ReloadExtension(const std::string id);

[Devlin, 2014/08/14 01:20:18]

const &

+
   // Returns true if the |extension| requires user consent before injecting
   // a script.
   bool RequiresUserConsent(const Extension* extension) const;
@@ -90,7 +98,11 @@ ActiveScriptControllerUnitTest::~ActiveScriptControllerUnitTest() {
 }
 
 const Extension* ActiveScriptControllerUnitTest::AddExtension() {
-  const std::string kId = id_util::GenerateId("all_hosts_extension");
+  return AddExtension(id_util::GenerateId("all_hosts_extension"));

[Devlin, 2014/08/14 01:20:18]

All of these ids will always be the same, so why do we have two methods?

[gpdavis, 2014/08/14 20:05:43]

Kalman suggested making an AddExtension method that takes an ID for reloading an
extension (by removing it and adding a new one with the same ID).  I didn't
realize GenerateId always generated the same ID for the same input string, but
using it to reload an extension without passing an ID seems kinda like a hack to
me (since it only works because the ID generated is always the same).

Thoughts?  We could definitely simplify this by removing the overloaded
AddExtension method, and making ReloadExtension simply remove the extension and
return AddExtension().

[not at google - send to devlin, 2014/08/14 20:08:07]

I prefer it being explicit. One version generates an ID (even though it
doesn't), the other makes it explicit.

+}
+
+const Extension* ActiveScriptControllerUnitTest::AddExtension(
+    const std::string id) {

[Devlin, 2014/08/14 01:20:18]

const &

   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
           .SetManifest(
@@ -102,7 +114,7 @@ const Extension* ActiveScriptControllerUnitTest::AddExtension() {
                   .Set("permissions",
                        ListBuilder().Append(kAllHostsPermission)))
           .SetLocation(Manifest::INTERNAL)
-          .SetID(kId)
+          .SetID(id)
           .Build();
 
   ExtensionRegistry::Get(profile())->AddEnabled(extension);
@@ -110,6 +122,12 @@ const Extension* ActiveScriptControllerUnitTest::AddExtension() {
   return extension;
 }
 
+const Extension* ActiveScriptControllerUnitTest::ReloadExtension(
+    const std::string id) {

[Devlin, 2014/08/14 01:20:18]

const &

[gpdavis, 2014/08/14 20:05:43]

This occurred to me, but since ReloadExtension removes the extension, then adds
a new one with the same ID, this can't be a reference.  I tested it, and it
indeed causes the test to crash.  AddExtension can take a reference, though, so
I changed that one.

[not at google - send to devlin, 2014/08/14 20:08:07]

Good point, though the solution here is for the caller to fix that problem, not
this function. I.e. do make this a const& but store a copy of the ID before
removing the extension.

[gpdavis, 2014/08/14 20:19:20]

Sweet, I like this idea.

+  ExtensionRegistry::Get(profile())->RemoveEnabled(id);
+  return AddExtension(id);
+}
+
 bool ActiveScriptControllerUnitTest::RequiresUserConsent(
     const Extension* extension) const {
   PermissionsData::AccessType access_type =
@@ -322,4 +340,49 @@ TEST_F(ActiveScriptControllerUnitTest, ActiveScriptsCanHaveAllUrlsPref) {
   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()));
+
+  // Simulate clicking "always run" menu item.

[Devlin, 2014/08/14 01:20:18]

Abstract away "clicking the menu item".

[gpdavis, 2014/08/14 20:05:43]

Done.

+  controller()->AlwaysRunOnVisibleHost(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/?gws_rd=ssl"));

[Devlin, 2014/08/14 01:20:18]

We should check on returning to a different site with the same origin.

[gpdavis, 2014/08/14 20:05:43]

Good idea.  Done.

+  EXPECT_FALSE(RequiresUserConsent(extension));
+
+  // Reloading the extension should clear active permissions, but not persisted
+  // permissions.
+  extension = ReloadExtension(extension->id());
+  Reload();
+  EXPECT_FALSE(RequiresUserConsent(extension));
+}
+
 }  // namespace extensions