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..d7aeffd96ec7ecb6110ff3fd367a3e73c8e30e6c 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.
+  // 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;
@@ -52,6 +60,9 @@ class ActiveScriptControllerUnitTest : public ChromeRenderViewHostTestHarness {
   // 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;
 
@@ -90,7 +101,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"));
+}
+
+const Extension* ActiveScriptControllerUnitTest::AddExtension(
+    const std::string& id) {
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
           .SetManifest(
@@ -102,7 +117,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 +125,13 @@ const Extension* ActiveScriptControllerUnitTest::AddExtension() {
   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 =
@@ -127,6 +149,10 @@ void ActiveScriptControllerUnitTest::RequestInjection(
       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 =
@@ -322,4 +348,48 @@ 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()));
+
+  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