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.
+  // 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;
 
   // Request an injection for the given |extension|.
   void RequestInjection(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;
 
@@ skipping 25 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"));

[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(
               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) {

[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 =
       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;
 }
 
@@ skipping 192 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()));
+
+  // 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