[Extensions Click-to-Script] Remove granted hosts when changing access

When transitioning from "on specific sites" to "on click", Chromium
should remove specific granted sites from the extension. Do so, and
add unittests for the same.

Bug: 844128

Change-Id: Icfa6c1f6449e94e30560a10cf43163f2fcedd996
Reviewed-on: https://chromium-review.googlesource.com/1133775Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574420}

chrome/browser/extensions/api/developer_private/developer_private_api.cc

@@ -873,8 +873,9 @@ DeveloperPrivateUpdateExtensionConfigurationFunction::Run() {
 
     switch (update.host_access) {
       case developer::HOST_ACCESS_ON_CLICK:
-      // TODO(devlin): We should also clear specific granted sites here.
-      // https://crbug.com/844128.
+        modifier.SetWithholdHostPermissions(true);
+        modifier.RemoveAllGrantedHostPermissions();
+        break;
       case developer::HOST_ACCESS_ON_SPECIFIC_SITES:
         modifier.SetWithholdHostPermissions(true);
         break;

chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc

@@ -137,6 +137,11 @@ class DeveloperPrivateApiUnitTest : public ExtensionServiceTestWithInstall {
   void GetProfileConfiguration(
       std::unique_ptr<api::developer_private::ProfileInfo>* profile_info);
 
+  // Runs the API function to update host access for the given |extension| to
+  // |new_access|.
+  void RunUpdateHostAccess(const Extension& extension,
+                           base::StringPiece new_access);
+
   virtual bool ProfileIsSupervised() const { return false; }
 
   Browser* browser() { return browser_.get(); }
@@ -317,6 +322,20 @@ void DeveloperPrivateApiUnitTest::GetProfileConfiguration(
       api::developer_private::ProfileInfo::FromValue(*response_value);
 }
 
+void DeveloperPrivateApiUnitTest::RunUpdateHostAccess(
+    const Extension& extension,
+    base::StringPiece new_access) {
+  SCOPED_TRACE(new_access);
+  ExtensionFunction::ScopedUserGestureForTests scoped_user_gesture;
+  scoped_refptr<UIThreadExtensionFunction> function = base::MakeRefCounted<
+      api::DeveloperPrivateUpdateExtensionConfigurationFunction>();
+  std::string args =
+      base::StringPrintf(R"([{"extensionId": "%s", "hostAccess": "%s"}])",
+                         extension.id().c_str(), new_access.data());
+  EXPECT_TRUE(api_test_utils::RunFunction(function.get(), args, profile()))
+      << function->GetError();
+}
+
 void DeveloperPrivateApiUnitTest::SetUp() {
   ExtensionServiceTestBase::SetUp();
 
@@ -1396,32 +1415,83 @@ TEST_F(DeveloperPrivateApiUnitTest, UpdateHostAccess) {
 
   EXPECT_FALSE(modifier.HasWithheldHostPermissions());
 
-  auto run_update_host_access = [this,
-                                 extension](base::StringPiece new_access) {
-    SCOPED_TRACE(new_access);
-    ExtensionFunction::ScopedUserGestureForTests scoped_user_gesture;
-    scoped_refptr<UIThreadExtensionFunction> function = base::MakeRefCounted<
-        api::DeveloperPrivateUpdateExtensionConfigurationFunction>();
-    std::string args =
-        base::StringPrintf(R"([{"extensionId": "%s", "hostAccess": "%s"}])",
-                           extension->id().c_str(), new_access.data());
-    EXPECT_TRUE(api_test_utils::RunFunction(function.get(), args, profile()))
-        << function->GetError();
-  };
-
-  run_update_host_access("ON_CLICK");
+  RunUpdateHostAccess(*extension, "ON_CLICK");
   EXPECT_TRUE(modifier.HasWithheldHostPermissions());
 
-  run_update_host_access("ON_ALL_SITES");
+  RunUpdateHostAccess(*extension, "ON_ALL_SITES");
   EXPECT_FALSE(modifier.HasWithheldHostPermissions());
 
-  run_update_host_access("ON_SPECIFIC_SITES");
+  RunUpdateHostAccess(*extension, "ON_SPECIFIC_SITES");
   EXPECT_TRUE(modifier.HasWithheldHostPermissions());
+}
 
-  // TODO(devlin): Test behavior of specific granted hosts (e.g. in the case of
-  // `on specific sites` -> `on click`) once we revoke granted hosts in that
-  // transition.
-  // https://crbug.com/844128.
+TEST_F(DeveloperPrivateApiUnitTest,
+       UpdateHostAccess_SpecificSitesRemovedOnTransitionToOnClick) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kRuntimeHostPermissions);
+
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("test").AddPermission("<all_urls>").Build();
+  service()->AddExtension(extension.get());
+  ScriptingPermissionsModifier modifier(profile(), extension.get());
+  modifier.SetWithholdHostPermissions(true);
+
+  RunUpdateHostAccess(*extension, "ON_SPECIFIC_SITES");
+  const GURL example_com("https://example.com");
+  modifier.GrantHostPermission(example_com);
+  EXPECT_TRUE(modifier.HasWithheldHostPermissions());
+  EXPECT_TRUE(modifier.HasGrantedHostPermission(example_com));
+
+  RunUpdateHostAccess(*extension, "ON_CLICK");
+  EXPECT_TRUE(modifier.HasWithheldHostPermissions());
+  EXPECT_FALSE(modifier.HasGrantedHostPermission(example_com));
+
+  // NOTE(devlin): It's a bit unfortunate that by cycling between host access
+  // settings, a user loses any stored state. This would be painful if the user
+  // had set "always run on foo" for a dozen or so sites, and accidentally
+  // changed the setting.
+  // There are ways we could address this, such as introducing a tri-state for
+  // the preference and keeping a stored set of any granted host permissions,
+  // but this then results in a funny edge case:
+  // - User has "on specific sites" set, with access to example.com and
+  //   chromium.org granted.
+  // - User changes to "on click" -> no sites are granted.
+  // - User visits google.com, and says "always run on this site." This changes
+  //   the setting back to "on specific sites", and will implicitly re-grant
+  //   example.com and chromium.org permissions, without any additional
+  //   prompting.
+  // To avoid this, we just clear any granted permissions when the user
+  // transitions between states. Since this is definitely a power-user surface,
+  // this is likely okay.
+  RunUpdateHostAccess(*extension, "ON_SPECIFIC_SITES");
+  EXPECT_TRUE(modifier.HasWithheldHostPermissions());
+  EXPECT_FALSE(modifier.HasGrantedHostPermission(example_com));
+}
+
+TEST_F(DeveloperPrivateApiUnitTest,
+       UpdateHostAccess_SpecificSitesRemovedOnTransitionToAllSites) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kRuntimeHostPermissions);
+
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("test").AddPermission("<all_urls>").Build();
+  service()->AddExtension(extension.get());
+  ScriptingPermissionsModifier modifier(profile(), extension.get());
+  modifier.SetWithholdHostPermissions(true);
+
+  RunUpdateHostAccess(*extension, "ON_SPECIFIC_SITES");
+  const GURL example_com("https://example.com");
+  modifier.GrantHostPermission(example_com);
+  EXPECT_TRUE(modifier.HasWithheldHostPermissions());
+  EXPECT_TRUE(modifier.HasGrantedHostPermission(example_com));
+
+  RunUpdateHostAccess(*extension, "ON_ALL_SITES");
+  EXPECT_FALSE(modifier.HasWithheldHostPermissions());
+  EXPECT_TRUE(modifier.HasGrantedHostPermission(example_com));
+
+  RunUpdateHostAccess(*extension, "ON_SPECIFIC_SITES");
+  EXPECT_TRUE(modifier.HasWithheldHostPermissions());
+  EXPECT_FALSE(modifier.HasGrantedHostPermission(example_com));
 }
 
 class DeveloperPrivateZipInstallerUnitTest