OOR-CORS: support DefaultPolicyHostRestrictions updates

PermissionsUpdater::SetDefaultPolicyHostRestrictions needs to
update cors access lists as well as other permission updating
methods do.

This patch extends NetworkPermissionsUpdateHelper to support
the default policy host restrictions case in addition to
permissions update.

This makes following tests pass with OOR-CORS.
 - BackgroundXhrWebstoreTest.PolicyContentScriptXHR
 - BackgroundXhrWebstoreTest.PolicyUpdateXHR
 - BackgroundXhrWebstoreTest.PolicyUpdateIndividualXHR
 - BackgroundXhrWebstoreTest.PolicyUpdateDefaultXHR

Bug: 907428
Change-Id: I0ae7777bd87baa1c98dfbefba44b9911eb9c08f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1478661Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637600}

chrome/browser/extensions/background_xhr_browsertest.cc

@@ -9,6 +9,7 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/post_task.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_with_management_policy_apitest.h"
@@ -33,6 +34,7 @@
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/ssl_server_config.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "services/network/public/cpp/features.h"
 #include "url/gurl.h"
 
 namespace extensions {
@@ -41,6 +43,12 @@ namespace {
 
 constexpr const char kWebstoreDomain[] = "cws.com";
 
+enum class TestMode {
+  kWithoutAny,
+  kWithOutOfBlinkCors,
+  kWithOutOfBlinkCorsAndNetworkService,
+};
+
 std::unique_ptr<net::ClientCertStore> CreateNullCertStore() {
   return nullptr;
 }
@@ -113,11 +121,34 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrTest, HttpAuth) {
       "test_http_auth.html", embedded_test_server()->GetURL("/auth-basic")));
 }
 
-class BackgroundXhrWebstoreTest : public ExtensionApiTestWithManagementPolicy {
+class BackgroundXhrWebstoreTest : public ExtensionApiTestWithManagementPolicy,
+                                  public testing::WithParamInterface<TestMode> {
  public:
   BackgroundXhrWebstoreTest() = default;
   ~BackgroundXhrWebstoreTest() override = default;
 
+  void SetUp() override {
+    std::vector<base::Feature> enabled_features;
+    std::vector<base::Feature> disabled_features;
+    switch (GetParam()) {
+      case TestMode::kWithoutAny:
+        disabled_features.push_back(network::features::kOutOfBlinkCors);
+        disabled_features.push_back(network::features::kNetworkService);
+        break;
+      case TestMode::kWithOutOfBlinkCors:
+        enabled_features.push_back(network::features::kOutOfBlinkCors);
+        disabled_features.push_back(network::features::kNetworkService);
+        break;
+      case TestMode::kWithOutOfBlinkCorsAndNetworkService:
+        enabled_features.push_back(network::features::kOutOfBlinkCors);
+        enabled_features.push_back(network::features::kNetworkService);
+        break;
+    }
+    scoped_feature_list_.InitWithFeatures(enabled_features, disabled_features);
+
+    ExtensionApiTestWithManagementPolicy::SetUp();
+  }
+
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ExtensionApiTest::SetUpCommandLine(command_line);
     // TODO(devlin): For some reason, trying to fetch an HTTPS url in this test
@@ -184,11 +215,13 @@ class BackgroundXhrWebstoreTest : public ExtensionApiTestWithManagementPolicy {
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(BackgroundXhrWebstoreTest);
 };
 
 // Extensions should not be able to XHR to the webstore.
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, XHRToWebstore) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, XHRToWebstore) {
   const Extension* extension = LoadXhrExtension("<all_urls>");
 
   GURL webstore_launch_url = extension_urls::GetWebstoreLaunchURL();
@@ -204,7 +237,7 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, XHRToWebstore) {
 }
 
 // Extensions should not be able to XHR to the webstore regardless of policy.
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, XHRToWebstorePolicy) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, XHRToWebstorePolicy) {
   {
     ExtensionManagementPolicyUpdater pref(&policy_provider_);
     pref.AddPolicyAllowedHost(
@@ -227,7 +260,7 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, XHRToWebstorePolicy) {
 
 // Extensions should not be able to bypass same-origin despite declaring
 // <all_urls> for hosts restricted by enterprise policy.
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyBlockedXHR) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, PolicyBlockedXHR) {
   {
     ExtensionManagementPolicyUpdater pref(&policy_provider_);
     pref.AddPolicyBlockedHost("*", "*://*.example.com");
@@ -248,7 +281,7 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyBlockedXHR) {
 }
 
 // Verify that policy blocklists apply to XHRs done from injected scripts.
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyContentScriptXHR) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, PolicyContentScriptXHR) {
   TestExtensionDir test_dir;
   test_dir.WriteManifest(R"(
     {
@@ -303,7 +336,7 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyContentScriptXHR) {
 
 // Make sure the blocklist and allowlist update for both Default and Individual
 // scope policies. Testing with all host permissions granted (<all_urls>).
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyUpdateXHR) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, PolicyUpdateXHR) {
   const Extension* extension = LoadXhrExtension("<all_urls>");
 
   GURL example_url =
@@ -347,7 +380,7 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyUpdateXHR) {
 // Make sure the allowlist entries added due to host permissions are removed
 // when a more generic blocklist policy is updated and contains them.
 // This tests the default policy scope update.
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyUpdateDefaultXHR) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, PolicyUpdateDefaultXHR) {
   const Extension* extension = LoadXhrExtension("*://public.example.com/*");
 
   GURL example_url =
@@ -372,7 +405,7 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyUpdateDefaultXHR) {
 // Make sure the allowlist entries added due to host permissions are removed
 // when a more generic blocklist policy is updated and contains them.
 // This tests an individual policy scope update.
-IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyUpdateIndividualXHR) {
+IN_PROC_BROWSER_TEST_P(BackgroundXhrWebstoreTest, PolicyUpdateIndividualXHR) {
   const Extension* extension = LoadXhrExtension("*://public.example.com/*");
 
   GURL example_url =
@@ -394,4 +427,15 @@ IN_PROC_BROWSER_TEST_F(BackgroundXhrWebstoreTest, PolicyUpdateIndividualXHR) {
   EXPECT_FALSE(CanFetch(extension, public_example_url));
 }
 
+INSTANTIATE_TEST_SUITE_P(WithoutAny,
+                         BackgroundXhrWebstoreTest,
+                         testing::Values(TestMode::kWithoutAny));
+INSTANTIATE_TEST_SUITE_P(WithOutOfBlinkCors,
+                         BackgroundXhrWebstoreTest,
+                         testing::Values(TestMode::kWithOutOfBlinkCors));
+INSTANTIATE_TEST_SUITE_P(
+    WithOutOfBlinkCorsAndNetworkService,
+    BackgroundXhrWebstoreTest,
+    testing::Values(TestMode::kWithOutOfBlinkCorsAndNetworkService));
+
 }  // namespace extensions

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/strings/string_split.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ -308,6 +309,10 @@ class ContentVerifierPolicyTest : public ContentVerifierTest {
 
     policy::BrowserPolicyConnector::SetPolicyProviderForTesting(
         &policy_provider_);
+    // ExtensionManagementPolicyUpdater requires a single-threaded context to
+    // call RunLoop::RunUntilIdle internally, and it isn't ready at this setup
+    // moment.
+    base::test::ScopedTaskEnvironment env;
     ExtensionManagementPolicyUpdater management_policy(&policy_provider_);
     management_policy.SetIndividualExtensionAutoInstalled(
         id_, extension_urls::kChromeWebstoreUpdateURL, true /* forced */);

chrome/browser/extensions/extension_management_test_util.cc

@@ -7,6 +7,7 @@
 #include <string>
 #include <utility>
 
+#include "base/run_loop.h"
 #include "components/crx_file/id_util.h"
 #include "components/policy/core/common/configuration_policy_provider.h"
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
@@ -35,6 +36,9 @@ ExtensionManagementPrefUpdaterBase::ExtensionManagementPrefUpdaterBase() {
 }
 
 ExtensionManagementPrefUpdaterBase::~ExtensionManagementPrefUpdaterBase() {
+  // Make asynchronous calls finished to deliver all preference changes to the
+  // NetworkService and extension processes.
+  base::RunLoop().RunUntilIdle();
 }
 
 // Helper functions for per extension settings ---------------------------------

chrome/browser/extensions/permissions_updater.cc

@@ -6,7 +6,9 @@
 
 #include <set>
 #include <utility>
+#include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/feature_list.h"
@@ -31,6 +33,7 @@
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/event_router_factory.h"
 #include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/cors_util.h"
 #include "extensions/common/extension.h"
@@ -121,20 +124,24 @@ class PermissionsUpdaterShutdownNotifierFactory
 
 }  // namespace
 
-// A helper class to asynchronously dispatch the permissions updated
-// notification once origin access has been updated. This will fire the
+// A helper class to asynchronously dispatch the event to notify policy host
+// restrictions or permissions once they have been updated. This will fire the
 // event if and only if the BrowserContext is still valid.
 // This class manages its own lifetime and deletes itself when either the
 // permissions updated event is fired, or the BrowserContext is shut down
 // (whichever happens first).
 class PermissionsUpdater::NetworkPermissionsUpdateHelper {
  public:
-  static void UpdateNetworkServicePermissions(
+  static void UpdatePermissions(content::BrowserContext* browser_context,
+                                EventType event_type,
+                                scoped_refptr<const Extension> extension,
+                                const PermissionSet& changed,
+                                base::OnceClosure completion_callback);
+
+  static void UpdateDefaultPolicyHostRestrictions(
       content::BrowserContext* browser_context,
-      EventType event_type,
-      scoped_refptr<const Extension> extension,
-      const PermissionSet& changed,
-      base::OnceClosure completion_callback);
+      const URLPatternSet& default_runtime_blocked_hosts,
+      const URLPatternSet& default_runtime_allowed_hosts);
 
  private:
   // This class manages its own lifetime.
@@ -154,15 +161,17 @@ class PermissionsUpdater::NetworkPermissionsUpdateHelper {
 };
 
 // static
-void PermissionsUpdater::NetworkPermissionsUpdateHelper::
-    UpdateNetworkServicePermissions(content::BrowserContext* browser_context,
-                                    EventType event_type,
-                                    scoped_refptr<const Extension> extension,
-                                    const PermissionSet& changed,
-                                    base::OnceClosure completion_callback) {
-  if (changed.effective_hosts().is_empty()) {
-    // If there is no difference in allowlist/blocklist for the extension, we
-    // can synchronously finish it without updating the CORS access list.
+void PermissionsUpdater::NetworkPermissionsUpdateHelper::UpdatePermissions(
+    content::BrowserContext* browser_context,
+    EventType event_type,
+    scoped_refptr<const Extension> extension,
+    const PermissionSet& changed,
+    base::OnceClosure completion_callback) {
+  // If there is no difference in allowlist/blocklist for the extension, we can
+  // synchronously finish it without updating the CORS access list.
+  // We do not apply this optimization for POLICY event_type, since callers do
+  // not pass effective |changed| argument.
+  if (event_type != POLICY && changed.effective_hosts().is_empty()) {
     PermissionsUpdater::NotifyPermissionsUpdated(
         browser_context, event_type, std::move(extension), changed.Clone(),
         std::move(completion_callback));
@@ -193,6 +202,41 @@ void PermissionsUpdater::NetworkPermissionsUpdateHelper::
                      helper->weak_factory_.GetWeakPtr()));
 }
 
+// static
+void PermissionsUpdater::NetworkPermissionsUpdateHelper::
+    UpdateDefaultPolicyHostRestrictions(
+        content::BrowserContext* browser_context,
+        const URLPatternSet& default_runtime_blocked_hosts,
+        const URLPatternSet& default_runtime_allowed_hosts) {
+  NetworkPermissionsUpdateHelper* helper = new NetworkPermissionsUpdateHelper(
+      browser_context,
+      base::BindOnce(
+          &PermissionsUpdater::NotifyDefaultPolicyHostRestrictionsUpdated,
+          browser_context, default_runtime_blocked_hosts.Clone(),
+          default_runtime_allowed_hosts.Clone()));
+
+  const ExtensionSet& extensions =
+      ExtensionRegistry::Get(browser_context)->enabled_extensions();
+  base::RepeatingClosure barrier_closure = base::BarrierClosure(
+      extensions.size(),
+      base::BindOnce(&NetworkPermissionsUpdateHelper::OnOriginAccessUpdated,
+                     helper->weak_factory_.GetWeakPtr()));
+
+  for (const auto& extension : extensions) {
+    std::vector<network::mojom::CorsOriginPatternPtr> allow_list =
+        CreateCorsOriginAccessAllowList(
+            *extension,
+            PermissionsData::EffectiveHostPermissionsMode::kOmitTabSpecific);
+    if (!base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+      ExtensionsClient::Get()->AddOriginAccessPermissions(*extension, true,
+                                                          &allow_list);
+    }
+    browser_context->SetCorsOriginAccessListForOrigin(
+        url::Origin::Create(extension->url()), std::move(allow_list),
+        CreateCorsOriginAccessBlockList(*extension), barrier_closure);
+  }
+}
+
 PermissionsUpdater::NetworkPermissionsUpdateHelper::
     NetworkPermissionsUpdateHelper(content::BrowserContext* browser_context,
                                    base::OnceClosure dispatch_event)
@@ -391,29 +435,32 @@ void PermissionsUpdater::SetPolicyHostRestrictions(
 
   // Update the BrowserContext origin lists, and send notification to the
   // currently running renderers of the runtime block hosts settings.
-  NetworkPermissionsUpdateHelper::UpdateNetworkServicePermissions(
-      browser_context_, POLICY, extension, PermissionSet(),
-      base::DoNothing::Once());
+  NetworkPermissionsUpdateHelper::UpdatePermissions(browser_context_, POLICY,
+                                                    extension, PermissionSet(),
+                                                    base::DoNothing::Once());
 }
 
 void PermissionsUpdater::SetUsesDefaultHostRestrictions(
     const Extension* extension) {
   extension->permissions_data()->SetUsesDefaultHostRestrictions();
-  NetworkPermissionsUpdateHelper::UpdateNetworkServicePermissions(
-      browser_context_, POLICY, extension, PermissionSet(),
-      base::DoNothing::Once());
+  NetworkPermissionsUpdateHelper::UpdatePermissions(browser_context_, POLICY,
+                                                    extension, PermissionSet(),
+                                                    base::DoNothing::Once());
 }
 
 void PermissionsUpdater::SetDefaultPolicyHostRestrictions(
     const URLPatternSet& default_runtime_blocked_hosts,
     const URLPatternSet& default_runtime_allowed_hosts) {
+  DCHECK_EQ(0, init_flag_ & INIT_FLAG_TRANSIENT);
+
   PermissionsData::SetDefaultPolicyHostRestrictions(
       default_runtime_blocked_hosts, default_runtime_allowed_hosts);
 
-  // Send notification to the currently running renderers of the runtime block
-  // hosts settings.
-  NotifyDefaultPolicyHostRestrictionsUpdated(default_runtime_blocked_hosts,
-                                             default_runtime_allowed_hosts);
+  // Update the BrowserContext origin lists, and send notification to the
+  // currently running renderers of the runtime block hosts settings.
+  NetworkPermissionsUpdateHelper::UpdateDefaultPolicyHostRestrictions(
+      browser_context_, default_runtime_blocked_hosts,
+      default_runtime_allowed_hosts);
 }
 
 void PermissionsUpdater::RemovePermissionsUnsafe(
@@ -434,7 +481,7 @@ void PermissionsUpdater::RemovePermissionsUnsafe(
   // permissions would be re-added.
   constexpr bool update_active_prefs = true;
   SetPermissions(extension, std::move(total), update_active_prefs);
-  NetworkPermissionsUpdateHelper::UpdateNetworkServicePermissions(
+  NetworkPermissionsUpdateHelper::UpdatePermissions(
       browser_context_, REMOVED, extension, *successfully_removed,
       base::DoNothing::Once());
 }
@@ -634,14 +681,12 @@ void PermissionsUpdater::NotifyPermissionsUpdated(
   std::move(completion_callback).Run();
 }
 
-// Notify the renderers that extension policy (policy_blocked_hosts) is updated
-// and provide new set of hosts.
+// static
 void PermissionsUpdater::NotifyDefaultPolicyHostRestrictionsUpdated(
-    const URLPatternSet& default_runtime_blocked_hosts,
-    const URLPatternSet& default_runtime_allowed_hosts) {
-  DCHECK_EQ(0, init_flag_ & INIT_FLAG_TRANSIENT);
-
-  Profile* profile = Profile::FromBrowserContext(browser_context_);
+    content::BrowserContext* browser_context,
+    const URLPatternSet default_runtime_blocked_hosts,
+    const URLPatternSet default_runtime_allowed_hosts) {
+  Profile* profile = Profile::FromBrowserContext(browser_context);
 
   ExtensionMsg_UpdateDefaultPolicyHostRestrictions_Params params;
   params.default_policy_blocked_hosts = default_runtime_blocked_hosts.Clone();
@@ -685,7 +730,7 @@ void PermissionsUpdater::AddPermissionsImpl(
                                        prefs_permissions_to_add);
   }
 
-  NetworkPermissionsUpdateHelper::UpdateNetworkServicePermissions(
+  NetworkPermissionsUpdateHelper::UpdatePermissions(
       browser_context_, ADDED, &extension, active_permissions_to_add,
       std::move(completion_callback));
 }
@@ -717,7 +762,7 @@ void PermissionsUpdater::RemovePermissionsImpl(
                                            prefs_permissions_to_remove);
   }
 
-  NetworkPermissionsUpdateHelper::UpdateNetworkServicePermissions(
+  NetworkPermissionsUpdateHelper::UpdatePermissions(
       browser_context_, REMOVED, &extension, active_permissions_to_remove,
       std::move(completion_callback));
 }

chrome/browser/extensions/permissions_updater.h

@@ -188,6 +188,15 @@ class PermissionsUpdater {
       std::unique_ptr<const PermissionSet> changed,
       base::OnceClosure completion_callback);
 
+  // Issues the relevant events, messages and notifications when the default
+  // scope management policy have changed.
+  // Specifically, this sends the ExtensionMsg_UpdateDefaultHostRestrictions
+  // IPC message.
+  static void NotifyDefaultPolicyHostRestrictionsUpdated(
+      content::BrowserContext* browser_context,
+      const URLPatternSet default_runtime_blocked_hosts,
+      const URLPatternSet default_runtime_allowed_hosts);
+
   // Sets the |extension|'s active permissions to |active|, and calculates and
   // sets the |extension|'s new withheld permissions. If |update_prefs| is true,
   // also updates the set of active permissions in the extension preferences.
@@ -195,14 +204,6 @@ class PermissionsUpdater {
                       std::unique_ptr<const PermissionSet> active,
                       bool update_prefs);
 
-  // Issues the relevant events, messages and notifications when the
-  // default scope management policy have changed.
-  // Specifically, this sends the ExtensionMsg_UpdateDefaultHostRestrictions
-  // IPC message.
-  void NotifyDefaultPolicyHostRestrictionsUpdated(
-      const URLPatternSet& default_runtime_blocked_hosts,
-      const URLPatternSet& default_runtime_allowed_hosts);
-
   // Adds the given |active_permissions_to_add| to |extension|'s current
   // active permissions (i.e., the permissions associated with the |extension|
   // object and the extension's process). Updates the preferences according to

chrome/browser/extensions/updater/update_service_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_task_environment.h"
 #include "chrome/browser/extensions/content_verifier_test_utils.h"
 #include "chrome/browser/extensions/extension_management_test_util.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -542,6 +543,10 @@ class PolicyUpdateServiceTest : public ExtensionUpdateClientBaseTest,
 
     policy::BrowserPolicyConnector::SetPolicyProviderForTesting(
         &policy_provider_);
+    // ExtensionManagementPolicyUpdater requires a single-threaded context to
+    // call RunLoop::RunUntilIdle internally, and it isn't ready at this setup
+    // moment.
+    base::test::ScopedTaskEnvironment env;
     ExtensionManagementPolicyUpdater management_policy(&policy_provider_);
     management_policy.SetIndividualExtensionAutoInstalled(
         id_, extension_urls::kChromeWebstoreUpdateURL, true /* forced */);