[Extensions Click-to-Script] Change default to allowing host permissions

Change the default behavior of the kRuntimeHostPermissions feature to
allow extensions to have origin access, rather than defaulting to
withholding permission. In the first stage of click-to-script, we don't
want to change any defaults.

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

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

@@ -366,6 +366,9 @@ TEST_F(DeveloperPrivateApiUnitTest,
   const Extension* extension = LoadUnpackedExtension();
   const std::string& id = extension->id();
 
+  ScriptingPermissionsModifier(profile(), base::WrapRefCounted(extension))
+      .SetAllowedOnAllUrls(false);
+
   TestExtensionPrefSetting(
       base::Bind(&HasPrefsPermission, &util::IsIncognitoEnabled, profile(), id),
       "incognitoAccess", id);

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

@@ -541,7 +541,8 @@ void ExtensionInfoGenerator::CreateExtensionInfoHelper(
        permissions_modifier.CanAffectExtension(
            extension.permissions_data()->active_permissions())) ||
       permissions_modifier.HasAffectedExtension();
-  info->run_on_all_urls.is_active = permissions_modifier.IsAllowedOnAllUrls();
+  info->run_on_all_urls.is_active = info->run_on_all_urls.is_enabled &&
+                                    permissions_modifier.IsAllowedOnAllUrls();
 
   // Runtime warnings.
   std::vector<std::string> warnings =

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

@@ -405,28 +405,25 @@ TEST_F(ExtensionInfoGeneratorUnitTest, ExtensionInfoRunOnAllUrls) {
   std::unique_ptr<developer::ExtensionInfo> info =
       GenerateExtensionInfo(all_urls_extension->id());
 
-  // The extension should want all urls, but not currently have it.
+  // The extension should want all urls, and have it currently granted.
   EXPECT_TRUE(info->run_on_all_urls.is_enabled);
-  EXPECT_FALSE(info->run_on_all_urls.is_active);
+  EXPECT_TRUE(info->run_on_all_urls.is_active);
 
-  // Give the extension all urls.
+  // Revoke the all urls permission.
   ScriptingPermissionsModifier permissions_modifier(profile(),
                                                     all_urls_extension);
-  permissions_modifier.SetAllowedOnAllUrls(true);
+  permissions_modifier.SetAllowedOnAllUrls(false);
 
-  // Now the extension should both want and have all urls.
+  // Now the extension want all urls, but not have it granted.
   info = GenerateExtensionInfo(all_urls_extension->id());
   EXPECT_TRUE(info->run_on_all_urls.is_enabled);
-  EXPECT_TRUE(info->run_on_all_urls.is_active);
+  EXPECT_FALSE(info->run_on_all_urls.is_active);
 
   // The other extension should neither want nor have all urls.
   info = GenerateExtensionInfo(no_urls_extension->id());
   EXPECT_FALSE(info->run_on_all_urls.is_enabled);
   EXPECT_FALSE(info->run_on_all_urls.is_active);
 
-  // Revoke the first extension's permissions.
-  permissions_modifier.SetAllowedOnAllUrls(false);
-
   // Turn off the switch and load another extension (so permissions are
   // re-initialized).
   scoped_feature_list.reset();
@@ -453,7 +450,7 @@ TEST_F(ExtensionInfoGeneratorUnitTest, ExtensionInfoRunOnAllUrls) {
   // show up without the switch.
   info = GenerateExtensionInfo(all_urls_extension->id());
   EXPECT_FALSE(info->run_on_all_urls.is_enabled);
-  EXPECT_TRUE(info->run_on_all_urls.is_active);
+  EXPECT_FALSE(info->run_on_all_urls.is_active);
 }
 
 // Test that file:// access checkbox does not show up when the user can't

chrome/browser/extensions/api/web_request/web_request_apitest.cc

@@ -24,6 +24,7 @@
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_with_management_policy_apitest.h"
+#include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/net/profile_network_context_service.h"
 #include "chrome/browser/net/profile_network_context_service_factory.h"
@@ -835,6 +836,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   const Extension* extension =
       LoadExtension(test_data_dir_.AppendASCII("webrequest_activetab"));
   ASSERT_TRUE(extension) << message_;
+  ScriptingPermissionsModifier(profile(), base::WrapRefCounted(extension))
+      .SetAllowedOnAllUrls(false);
   EXPECT_TRUE(listener.WaitUntilSatisfied());
 
   // Navigate the browser to a page in a new tab.

chrome/browser/extensions/extension_action_runner_browsertest.cc

@@ -58,6 +58,8 @@ enum HostType { ALL_HOSTS, EXPLICIT_HOSTS };
 
 enum RequiresConsent { REQUIRES_CONSENT, DOES_NOT_REQUIRE_CONSENT };
 
+enum WithholdPermissions { WITHHOLD_PERMISSIONS, DONT_WITHHOLD_PERMISSIONS };
+
 // Runs all pending tasks in the renderer associated with |web_contents|.
 // Returns true on success.
 bool RunAllPendingInRenderer(content::WebContents* web_contents) {
@@ -97,7 +99,8 @@ class ExtensionActionRunnerBrowserTest : public ExtensionBrowserTest {
   // one will be created.
   // This could potentially return NULL if LoadExtension() fails.
   const Extension* CreateExtension(HostType host_type,
-                                   InjectionType injection_type);
+                                   InjectionType injection_type,
+                                   WithholdPermissions withhold_permissions);
 
  private:
   std::vector<std::unique_ptr<TestExtensionDir>> test_extension_dirs_;
@@ -117,7 +120,8 @@ void ExtensionActionRunnerBrowserTest::TearDownOnMainThread() {
 
 const Extension* ExtensionActionRunnerBrowserTest::CreateExtension(
     HostType host_type,
-    InjectionType injection_type) {
+    InjectionType injection_type,
+    WithholdPermissions withhold_permissions) {
   std::string name = base::StringPrintf(
       "%s %s",
       injection_type == CONTENT_SCRIPT ? "content_script" : "execute_script",
@@ -165,6 +169,13 @@ const Extension* ExtensionActionRunnerBrowserTest::CreateExtension(
   if (extension) {
     test_extension_dirs_.push_back(std::move(dir));
     extensions_.push_back(extension);
+
+    ScriptingPermissionsModifier modifier(profile(), extension);
+    if (withhold_permissions == WITHHOLD_PERMISSIONS &&
+        modifier.CanAffectExtension(
+            extension->permissions_data()->active_permissions())) {
+      modifier.SetAllowedOnAllUrls(false);
+    }
   }
 
   // If extension is NULL here, it will be caught later in the test.
@@ -317,19 +328,21 @@ IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
   // that request all hosts require user consent.
   std::vector<std::unique_ptr<ActiveScriptTester>> testers;
   testers.push_back(std::make_unique<ActiveScriptTester>(
-      "inject_scripts_all_hosts", CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT),
+      "inject_scripts_all_hosts",
+      CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT, WITHHOLD_PERMISSIONS),
       browser(), REQUIRES_CONSENT, EXECUTE_SCRIPT));
   testers.push_back(std::make_unique<ActiveScriptTester>(
       "inject_scripts_explicit_hosts",
-      CreateExtension(EXPLICIT_HOSTS, EXECUTE_SCRIPT), browser(),
-      DOES_NOT_REQUIRE_CONSENT, EXECUTE_SCRIPT));
+      CreateExtension(EXPLICIT_HOSTS, EXECUTE_SCRIPT, WITHHOLD_PERMISSIONS),
+      browser(), DOES_NOT_REQUIRE_CONSENT, EXECUTE_SCRIPT));
   testers.push_back(std::make_unique<ActiveScriptTester>(
-      "content_scripts_all_hosts", CreateExtension(ALL_HOSTS, CONTENT_SCRIPT),
+      "content_scripts_all_hosts",
+      CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS),
       browser(), REQUIRES_CONSENT, CONTENT_SCRIPT));
   testers.push_back(std::make_unique<ActiveScriptTester>(
       "content_scripts_explicit_hosts",
-      CreateExtension(EXPLICIT_HOSTS, CONTENT_SCRIPT), browser(),
-      DOES_NOT_REQUIRE_CONSENT, CONTENT_SCRIPT));
+      CreateExtension(EXPLICIT_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS),
+      browser(), DOES_NOT_REQUIRE_CONSENT, CONTENT_SCRIPT));
 
   // Navigate to an URL (which matches the explicit host specified in the
   // extension content_scripts_explicit_hosts). All four extensions should
@@ -348,9 +361,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
 IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
                        RemoveExtensionWithPendingInjections) {
   // Load up two extensions, each with content scripts.
-  const Extension* extension1 = CreateExtension(ALL_HOSTS, CONTENT_SCRIPT);
+  const Extension* extension1 =
+      CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS);
   ASSERT_TRUE(extension1);
-  const Extension* extension2 = CreateExtension(ALL_HOSTS, CONTENT_SCRIPT);
+  const Extension* extension2 =
+      CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS);
   ASSERT_TRUE(extension2);
 
   ASSERT_NE(extension1->id(), extension2->id());
@@ -394,7 +409,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
 IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
                        GrantExtensionAllUrlsPermission) {
   // Loadup an extension and navigate.
-  const Extension* extension = CreateExtension(ALL_HOSTS, CONTENT_SCRIPT);
+  const Extension* extension =
+      CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS);
   ASSERT_TRUE(extension);
 
   content::WebContents* web_contents =
@@ -451,6 +467,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
   const Extension* extension = LoadExtension(
       test_data_dir_.AppendASCII("blocked_actions/content_scripts"));
   ASSERT_TRUE(extension);
+  ScriptingPermissionsModifier(profile(), extension).SetAllowedOnAllUrls(false);
+
   ui_test_utils::NavigateToURL(browser(), url);
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
@@ -529,6 +547,27 @@ IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
       web_contents->GetController().GetLastCommittedEntry()->GetUniqueID());
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
+                       ScriptsExecuteWhenNoPermissionsWithheld) {
+  // If we don't withhold permissions, extensions should execute normally.
+  std::vector<std::unique_ptr<ActiveScriptTester>> testers;
+  testers.push_back(std::make_unique<ActiveScriptTester>(
+      "content_scripts_all_hosts",
+      CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
+      browser(), DOES_NOT_REQUIRE_CONSENT, CONTENT_SCRIPT));
+  testers.push_back(std::make_unique<ActiveScriptTester>(
+      "inject_scripts_all_hosts",
+      CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
+      browser(), DOES_NOT_REQUIRE_CONSENT, EXECUTE_SCRIPT));
+
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
+
+  for (const auto& tester : testers)
+    EXPECT_TRUE(tester->Verify()) << tester->name();
+}
+
 // A version of the test with the flag off, in order to test that everything
 // still works as expected.
 class FlagOffExtensionActionRunnerBrowserTest
@@ -544,10 +583,12 @@ IN_PROC_BROWSER_TEST_F(FlagOffExtensionActionRunnerBrowserTest,
                        ScriptsExecuteWhenFlagAbsent) {
   std::vector<std::unique_ptr<ActiveScriptTester>> testers;
   testers.push_back(std::make_unique<ActiveScriptTester>(
-      "content_scripts_all_hosts", CreateExtension(ALL_HOSTS, CONTENT_SCRIPT),
+      "content_scripts_all_hosts",
+      CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
       browser(), DOES_NOT_REQUIRE_CONSENT, CONTENT_SCRIPT));
   testers.push_back(std::make_unique<ActiveScriptTester>(
-      "inject_scripts_all_hosts", CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT),
+      "inject_scripts_all_hosts",
+      CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
       browser(), DOES_NOT_REQUIRE_CONSENT, EXECUTE_SCRIPT));
 
   ASSERT_TRUE(embedded_test_server()->Start());

chrome/browser/extensions/extension_action_runner_unittest.cc

@@ -116,6 +116,9 @@ const Extension* ExtensionActionRunnerUnitTest::AddExtension() {
 
   ExtensionRegistry::Get(profile())->AddEnabled(extension_);
   PermissionsUpdater(profile()).InitializePermissions(extension_.get());
+
+  ScriptingPermissionsModifier(profile(), extension_.get())
+      .SetAllowedOnAllUrls(false);
   return extension_.get();
 }
 

chrome/browser/extensions/extension_context_menu_model_unittest.cc

@@ -231,6 +231,7 @@ const Extension* ExtensionContextMenuModelTest::AddExtensionWithHostPermission(
           .Build();
   if (!extension.get())
     ADD_FAILURE();
+  service()->GrantPermissions(extension.get());
   service()->AddExtension(extension.get());
   return extension.get();
 }
@@ -618,10 +619,12 @@ TEST_F(ExtensionContextMenuModelTest, TestPageAccessSubmenu) {
   scoped_feature_list->InitAndEnableFeature(features::kRuntimeHostPermissions);
   InitializeEmptyExtensionService();
 
-  // Add an extension with all urls.
+  // Add an extension with all urls, and withhold permission.
   const Extension* extension =
       AddExtensionWithHostPermission("extension", manifest_keys::kBrowserAction,
                                      Manifest::INTERNAL, "*://*/*");
+  ScriptingPermissionsModifier(profile(), extension).SetAllowedOnAllUrls(false);
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
 
   const GURL kActiveUrl("http://www.example.com/");
   const GURL kOtherUrl("http://www.google.com/");

chrome/browser/extensions/extension_install_prompt_unittest.cc

@@ -129,42 +129,6 @@ TEST_F(ExtensionInstallPromptUnitTest, PromptShowsPermissionWarnings) {
   run_loop.Run();
 }
 
-TEST_F(ExtensionInstallPromptUnitTest, PromptShowsWithheldPermissions) {
-  // Enable features::kRuntimeHostPermissions so that <all_hosts> permissions
-  // get withheld.
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kRuntimeHostPermissions);
-
-  scoped_refptr<const Extension> extension =
-      ExtensionBuilder()
-          .SetManifest(
-              DictionaryBuilder()
-                  .Set("name", "foo")
-                  .Set("version", "1.0")
-                  .Set("manifest_version", 2)
-                  .Set("description", "Random Ext")
-                  .Set("permissions", ListBuilder()
-                                          .Append("http://*/*")
-                                          .Append("http://www.google.com/")
-                                          .Append("tabs")
-                                          .Build())
-                  .Build())
-          .Build();
-
-  content::TestWebContentsFactory factory;
-  ExtensionInstallPrompt prompt(factory.CreateWebContents(profile()));
-  base::RunLoop run_loop;
-
-  // We expect <all_hosts> to be withheld, but http://www.google.com/ and tabs
-  // permissions should be granted as regular permissions.
-  prompt.ShowDialog(
-      ExtensionInstallPrompt::DoneCallback(), extension.get(), nullptr,
-      base::Bind(&VerifyPromptPermissionsCallback, run_loop.QuitClosure(),
-                 2u,    // |regular_permissions_count|.
-                 1u));  // |withheld_permissions_count|.
-  run_loop.Run();
-}
-
 TEST_F(ExtensionInstallPromptUnitTest,
        DelegatedPromptShowsOptionalPermissions) {
   scoped_refptr<const Extension> extension =

chrome/browser/extensions/permissions_updater.cc

@@ -197,17 +197,25 @@ void PermissionsUpdater::RemovePermissionsUnsafe(
 
 std::unique_ptr<const PermissionSet>
 PermissionsUpdater::GetRevokablePermissions(const Extension* extension) const {
-  // The user can revoke any permissions they granted. In other words, any
-  // permissions the extension didn't start with can be revoked.
+  // Any permissions not required by the extension are revokable.
   const PermissionSet& required =
       PermissionsParser::GetRequiredPermissions(extension);
-  std::unique_ptr<const PermissionSet> granted;
-  std::unique_ptr<const PermissionSet> withheld;
-  ScriptingPermissionsModifier(browser_context_,
-                               base::WrapRefCounted(extension))
-      .WithholdPermissions(required, &granted, &withheld, true);
-  return PermissionSet::CreateDifference(
-      extension->permissions_data()->active_permissions(), *granted);
+  std::unique_ptr<const PermissionSet> revokable_permissions =
+      PermissionSet::CreateDifference(
+          extension->permissions_data()->active_permissions(), required);
+
+  // Additionally, some required permissions may be revokable if they can be
+  // withheld by the ScriptingPermissionsModifier.
+  std::unique_ptr<const PermissionSet> revokable_scripting_permissions =
+      ScriptingPermissionsModifier(browser_context_,
+                                   base::WrapRefCounted(extension))
+          .GetRevokablePermissions();
+
+  if (revokable_scripting_permissions) {
+    revokable_permissions = PermissionSet::CreateUnion(
+        *revokable_permissions, *revokable_scripting_permissions);
+  }
+  return revokable_permissions;
 }
 
 void PermissionsUpdater::GrantActivePermissions(const Extension* extension) {
@@ -239,8 +247,7 @@ void PermissionsUpdater::InitializePermissions(const Extension* extension) {
   ScriptingPermissionsModifier(browser_context_,
                                base::WrapRefCounted(extension))
       .WithholdPermissions(*bounded_active, &granted_permissions,
-                           &withheld_permissions,
-                           (init_flag_ & INIT_FLAG_TRANSIENT) != 0);
+                           &withheld_permissions);
 
   if (g_delegate)
     g_delegate->InitializePermissions(extension, &granted_permissions);

chrome/browser/extensions/permissions_updater_unittest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_test_util.h"
 #include "chrome/test/base/testing_profile.h"
@@ -356,8 +357,11 @@ TEST_F(PermissionsUpdaterTest, RevokingPermissions) {
     PermissionsUpdater updater(profile());
     updater.InitializePermissions(extension.get());
 
-    // By default, all-hosts was withheld, so the extension shouldn't have
-    // access to any site (like foo.com).
+    ScriptingPermissionsModifier(profile(), extension)
+        .SetAllowedOnAllUrls(false);
+
+    // All-hosts was withheld, so the extension shouldn't have access to any
+    // site (like foo.com).
     const GURL kOrigin("http://foo.com");
 
     EXPECT_FALSE(extension->permissions_data()

chrome/browser/extensions/scripting_permissions_modifier.cc

@@ -66,6 +66,42 @@ void SetAllowedOnAllUrlsPref(bool by_user,
   }
 }
 
+// Partitions |permissions| into two sets of permissions, placing any
+// all-hosts-like permissions into |withheld_permissions_out| and the rest
+// into |granted_permissions_out|.
+void PartitionPermissions(
+    const PermissionSet& permissions,
+    std::unique_ptr<const PermissionSet>* granted_permissions_out,
+    std::unique_ptr<const PermissionSet>* withheld_permissions_out) {
+  auto segregate_url_permissions = [](const URLPatternSet& patterns,
+                                      URLPatternSet* granted,
+                                      URLPatternSet* withheld) {
+    for (const URLPattern& pattern : patterns) {
+      if (pattern.MatchesEffectiveTld())
+        withheld->AddPattern(pattern);
+      else
+        granted->AddPattern(pattern);
+    }
+  };
+
+  URLPatternSet granted_explicit_hosts;
+  URLPatternSet withheld_explicit_hosts;
+  URLPatternSet granted_scriptable_hosts;
+  URLPatternSet withheld_scriptable_hosts;
+  segregate_url_permissions(permissions.explicit_hosts(),
+                            &granted_explicit_hosts, &withheld_explicit_hosts);
+  segregate_url_permissions(permissions.scriptable_hosts(),
+                            &granted_scriptable_hosts,
+                            &withheld_scriptable_hosts);
+
+  granted_permissions_out->reset(
+      new PermissionSet(permissions.apis(), permissions.manifest_permissions(),
+                        granted_explicit_hosts, granted_scriptable_hosts));
+  withheld_permissions_out->reset(
+      new PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
+                        withheld_explicit_hosts, withheld_scriptable_hosts));
+}
+
 }  // namespace
 
 ScriptingPermissionsModifier::ScriptingPermissionsModifier(
@@ -98,11 +134,6 @@ void ScriptingPermissionsModifier::SetAllowedOnAllUrlsForSync(
   SetAllowedOnAllUrlsPref(true, allowed, id, ExtensionPrefs::Get(context));
 }
 
-// static
-bool ScriptingPermissionsModifier::DefaultAllowedOnAllUrls() {
-  return !base::FeatureList::IsEnabled(features::kRuntimeHostPermissions);
-}
-
 void ScriptingPermissionsModifier::SetAllowedOnAllUrls(bool allowed) {
   if (ExtensionMustBeAllowedOnAllUrls(*extension_)) {
     CleanUpPrefsIfNecessary();
@@ -132,11 +163,9 @@ bool ScriptingPermissionsModifier::IsAllowedOnAllUrls() {
   bool allowed = false;
   if (!extension_prefs_->ReadPrefAsBoolean(
           extension_->id(), kExtensionAllowedOnAllUrlsPrefName, &allowed)) {
-    // If there is no value present, we make one, defaulting it to the value of
-    // the features::kRuntimeHostPermissions feature. If the feature is
-    // enabled, then the extension does not have permission to access all
-    // urls by default.
-    allowed = DefaultAllowedOnAllUrls();
+    // If there is no value present, we make one, defaulting it to true -
+    // extensions are granted permissions until revoked.
+    allowed = true;
     SetAllowedOnAllUrlsPref(false, allowed, extension_->id(), extension_prefs_);
   }
   return allowed;
@@ -185,20 +214,30 @@ void ScriptingPermissionsModifier::GrantHostPermission(const GURL& url) {
 
 bool ScriptingPermissionsModifier::HasGrantedHostPermission(const GURL& url) {
   GURL origin = url.GetOrigin();
-  const PermissionSet& required_permissions =
-      PermissionsParser::GetRequiredPermissions(extension_.get());
+  // If the extension doesn't have access to the host, it clearly hasn't been
+  // granted permission.
   if (!extension_->permissions_data()
            ->active_permissions()
            .effective_hosts()
-           .MatchesURL(origin))
+           .MatchesURL(origin)) {
     return false;
+  }
+
+  // Check which permissions would have been withheld. If access to the host
+  // would have otherwise been withheld, then we know that access has been
+  // explicitly granted.
+  // TODO(devlin): This seems wrong, and won't work with trying to grant or
+  // withhold e.g. optional permissions. It's also overly expensive.
+  const PermissionSet& required_permissions =
+      PermissionsParser::GetRequiredPermissions(extension_.get());
   std::unique_ptr<const PermissionSet> granted_permissions;
   std::unique_ptr<const PermissionSet> withheld_permissions;
-  WithholdPermissions(required_permissions, &granted_permissions,
-                      &withheld_permissions, true);
+  PartitionPermissions(required_permissions, &granted_permissions,
+                       &withheld_permissions);
   if (!granted_permissions->effective_hosts().MatchesURL(origin) &&
-      withheld_permissions->effective_hosts().MatchesURL(origin))
+      withheld_permissions->effective_hosts().MatchesURL(origin)) {
     return true;
+  }
 
   return false;
 }
@@ -212,6 +251,10 @@ void ScriptingPermissionsModifier::RemoveGrantedHostPermission(
   URLPatternSet scriptable_hosts;
   const PermissionSet& active_permissions =
       extension_->permissions_data()->active_permissions();
+
+  // We know the host permission was granted, but it may only be requested in
+  // either explicit or scriptable hosts. Only remove it if it is already
+  // present.
   if (active_permissions.explicit_hosts().MatchesURL(url))
     explicit_hosts.AddOrigin(UserScript::ValidUserScriptSchemes(), origin);
   if (active_permissions.scriptable_hosts().MatchesURL(url))
@@ -228,20 +271,9 @@ void ScriptingPermissionsModifier::RemoveGrantedHostPermission(
 void ScriptingPermissionsModifier::WithholdPermissions(
     const PermissionSet& permissions,
     std::unique_ptr<const PermissionSet>* granted_permissions_out,
-    std::unique_ptr<const PermissionSet>* withheld_permissions_out,
-    bool use_initial_state) {
-  bool should_withhold = false;
-  if (CanAffectExtension(permissions)) {
-    if (use_initial_state) {
-      // If the user ever set the extension's "all-urls" preference, then the
-      // initial state was withheld. This is important, since the all-urls
-      // permission should be shown as revokable. Otherwise, default to whatever
-      // the system setting is.
-      should_withhold = HasSetAllowedOnAllUrls() || !DefaultAllowedOnAllUrls();
-    } else {
-      should_withhold = !IsAllowedOnAllUrls();
-    }
-  }
+    std::unique_ptr<const PermissionSet>* withheld_permissions_out) {
+  bool should_withhold =
+      CanAffectExtension(permissions) && !IsAllowedOnAllUrls();
 
   if (!should_withhold) {
     *granted_permissions_out = permissions.Clone();
@@ -249,33 +281,32 @@ void ScriptingPermissionsModifier::WithholdPermissions(
     return;
   }
 
-  auto segregate_url_permissions = [](const URLPatternSet& patterns,
-                                      URLPatternSet* granted,
-                                      URLPatternSet* withheld) {
-    for (const URLPattern& pattern : patterns) {
-      if (pattern.MatchesEffectiveTld())
-        withheld->AddPattern(pattern);
-      else
-        granted->AddPattern(pattern);
-    }
-  };
+  PartitionPermissions(permissions, granted_permissions_out,
+                       withheld_permissions_out);
+}
 
-  URLPatternSet granted_explicit_hosts;
-  URLPatternSet withheld_explicit_hosts;
-  URLPatternSet granted_scriptable_hosts;
-  URLPatternSet withheld_scriptable_hosts;
-  segregate_url_permissions(permissions.explicit_hosts(),
-                            &granted_explicit_hosts, &withheld_explicit_hosts);
-  segregate_url_permissions(permissions.scriptable_hosts(),
-                            &granted_scriptable_hosts,
-                            &withheld_scriptable_hosts);
+std::unique_ptr<const PermissionSet>
+ScriptingPermissionsModifier::GetRevokablePermissions() {
+  // No additional revokable permissions for extensions that must be allowed
+  // to run everywhere.
+  if (ExtensionMustBeAllowedOnAllUrls(*extension_))
+    return nullptr;
+
+  bool experiment_is_enabled =
+      base::FeatureList::IsEnabled(features::kRuntimeHostPermissions);
+  // Check for revokable permissions if either the experiment is enabled or if
+  // the extension has been affected in the past.
+  // TODO(devlin): This should only rely on the experiment state.
+  // https://crbug.com/841465
+  if (!experiment_is_enabled && !HasAffectedExtension())
+    return nullptr;
 
-  granted_permissions_out->reset(
-      new PermissionSet(permissions.apis(), permissions.manifest_permissions(),
-                        granted_explicit_hosts, granted_scriptable_hosts));
-  withheld_permissions_out->reset(
-      new PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
-                        withheld_explicit_hosts, withheld_scriptable_hosts));
+  std::unique_ptr<const PermissionSet> granted_permissions;
+  std::unique_ptr<const PermissionSet> withheld_permissions;
+  PartitionPermissions(extension_->permissions_data()->active_permissions(),
+                       &granted_permissions, &withheld_permissions);
+
+  return withheld_permissions;
 }
 
 void ScriptingPermissionsModifier::GrantWithheldImpliedAllHosts() {

chrome/browser/extensions/scripting_permissions_modifier.h

@@ -39,9 +39,6 @@ class ScriptingPermissionsModifier {
                                          content::BrowserContext* context,
                                          const std::string& id);
 
-  // Returns the default value for being allowed to script on all urls.
-  static bool DefaultAllowedOnAllUrls();
-
   // Sets whether the extension should be allowed to execute on all urls without
   // explicit user consent. Used when the features::kRuntimeHostPermissions
   // feature is enabled.
@@ -80,13 +77,13 @@ class ScriptingPermissionsModifier {
   // be granted, populating |granted_permissions_out| with the set of all
   // permissions that can be granted, and |withheld_permissions_out| with the
   // set of all withheld permissions.
-  // If |use_initial_state| is true, this will treat the extension as though it
-  // was just installed, not taking into account extra granted preferences.
   void WithholdPermissions(
       const PermissionSet& permissions,
       std::unique_ptr<const PermissionSet>* granted_permissions_out,
-      std::unique_ptr<const PermissionSet>* withheld_permissions_out,
-      bool use_initial_state);
+      std::unique_ptr<const PermissionSet>* withheld_permissions_out);
+
+  // Returns the subset of active permissions which can be withheld.
+  std::unique_ptr<const PermissionSet> GetRevokablePermissions();
 
  private:
   // Grants any withheld all-hosts (or all-hosts-like) permissions.

chrome/browser/ui/extensions/blocked_action_bubble_browsertest.cc

@@ -5,6 +5,7 @@
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/extensions/browser_action_test_util.h"
@@ -77,6 +78,9 @@ void ExtensionBlockedActionsBubbleTest::ShowUi(const std::string& name) {
   const extensions::Extension* extension =
       LoadExtension(test_dir.UnpackedPath());
   ASSERT_TRUE(extension);
+  extensions::ScriptingPermissionsModifier(profile(),
+                                           base::WrapRefCounted(extension))
+      .SetAllowedOnAllUrls(false);
 
   content::WebContents* tab =
       browser()->tab_strip_model()->GetActiveWebContents();

chrome/test/data/extensions/api_test/developer/generated_output/behllobkkfkfnphdnhnkndlbkcpglgmj.json

@@ -30,7 +30,7 @@
    "name": "My extension 1",
    "offlineEnabled": false,
    "runOnAllUrls": {
-      "isActive": true,
+      "isActive": false,
       "isEnabled": false
    },
    "runtimeErrors": [  ],

chrome/test/data/extensions/api_test/developer/generated_output/bjafgdebaacbbbecmhlhpofkepfkgcpa.json

@@ -30,7 +30,7 @@
    "name": "My extension 3",
    "offlineEnabled": false,
    "runOnAllUrls": {
-      "isActive": true,
+      "isActive": false,
       "isEnabled": false
    },
    "runtimeErrors": [  ],

chrome/test/data/extensions/api_test/developer/generated_output/hpiknbiabeeppbpihjehijgoemciehgk.json

@@ -30,7 +30,7 @@
    "name": "My extension 2",
    "offlineEnabled": false,
    "runOnAllUrls": {
-      "isActive": true,
+      "isActive": false,
       "isEnabled": false
    },
    "runtimeErrors": [  ],