Respect QuicAllowed policy for new streams

chrome/browser/policy/policy_network_browsertest.cc

Index: chrome/browser/policy/policy_network_browsertest.cc
diff --git a/chrome/browser/policy/policy_network_browsertest.cc b/chrome/browser/policy/policy_network_browsertest.cc
index 7aed8c46b6de0cf7310f1f0a67757166036fcf0c..5fc0c2fa9617a4f5244c946f32039a76cf111200 100644
--- a/chrome/browser/policy/policy_network_browsertest.cc
+++ b/chrome/browser/policy/policy_network_browsertest.cc
@@ -9,6 +9,12 @@
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/policy/profile_policy_connector_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/profiles/profiles_state.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/policy/core/browser/browser_policy_connector.h"
@@ -22,30 +28,121 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/chromeos_switches.h"
+#endif
+
 namespace {
 
-void VerifyQuicEnabledStatus(net::URLRequestContextGetter* getter,
-                             bool quic_enabled_expected,
-                             const base::Closure& done_callback) {
-  net::URLRequestContext* context = getter->GetURLRequestContext();
-  bool quic_enabled = context->http_transaction_factory()->GetSession()->
-        params().enable_quic;
-  EXPECT_EQ(quic_enabled_expected, quic_enabled);
+// Checks if QUIC is enabled for new streams in the passed |context|.
+bool IsQuicEnabled(net::URLRequestContextGetter* context) {
+  return context->GetURLRequestContext()
+      ->http_transaction_factory()
+      ->GetSession()
+      ->IsQuicEnabled();
+}
+
+// Verifies QUIC enablement for new streams in the passed |profile_context|.
+// |done_callback| is used to indicate that the browser test may continue.
+void VerifyProfileQuicEnabledOnIOThread(
+    net::URLRequestContextGetter* profile_context,
+    bool quic_enabled_expected,
+    const base::Closure& done_callback) {
+  bool quic_enabled_profile = IsQuicEnabled(profile_context);
+  EXPECT_EQ(quic_enabled_expected, quic_enabled_profile);
 
   content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
                                    done_callback);
 }
 
-void VerifyQuicEnabledStatusInIOThread(bool quic_enabled_expected) {
+// Verifies QUIC enablement for new streams in the passed |profile|.
+void VerifyProfileQuicEnabled(Profile* profile, bool quic_enabled_expected) {

[mmenke, 2017/01/10 18:03:38]

For slightly easier testing, I suggest:

bool IsQuicEnabledOnUIThread(net::URLRequestContextGetter request_context) {
  base::RunLoop run_loop;
  bool is_quic_enabled= false;
  PostTaskAndReply(base::Bind(IsQuicEnabled(context, &is_quic_enabled)),
                   run_loop.QuitClosure());
  run_loop.Run();
  return is_quic_enabled;
}

(And update IsQuicEnabled to take a bool pointer, which it sets, instead of
returning a value).

This allows you to run EXPECTS in the test fixture, which is easier to read and
tells you exactly which EXPECT failed if you have more than one in a test.  Also
requires less code, since you don't need the above method.

(Suggest using URLRequestContextGetter instead of the profile so we can check
the other two contexts, and get rid of the next two methods, and can separate
out checks on the three URLRequestContexts)

[pmarko, 2017/01/10 20:47:05]

Good idea. As discussed offline, I've named them IsQuicEnabled and
IsQuicEnabledOnIOThread for brevity where IsQuicEnabled is used, and added
DCHECK_CURRENTLY_ON calls.

   base::RunLoop run_loop;
+  net::URLRequestContextGetter* profile_context = profile->GetRequestContext();
+
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
-      base::Bind(VerifyQuicEnabledStatus,
-                 base::RetainedRef(g_browser_process->system_request_context()),
+      base::Bind(VerifyProfileQuicEnabledOnIOThread,
+                 base::RetainedRef(profile_context), quic_enabled_expected,
+                 run_loop.QuitClosure()));
+  run_loop.Run();
+}
+
+// Verifies QUIC enablement for new streams in the passed |system_context| and
+// |safe_browsing_context|.
+// |done_callback| is used to indicate that the browser test may continue.
+void VerifyGlobalQuicEnabledOnIOThread(
+    net::URLRequestContextGetter* system_context,
+    scoped_refptr<net::URLRequestContextGetter> safe_browsing_context,
+    bool quic_enabled_expected,
+    const base::Closure& done_callback) {
+  bool quic_enabled_system = IsQuicEnabled(system_context);
+  EXPECT_EQ(quic_enabled_expected, quic_enabled_system);
+
+  bool quic_enabled_safe_browsing = IsQuicEnabled(safe_browsing_context.get());
+  EXPECT_EQ(quic_enabled_expected, quic_enabled_safe_browsing);
+
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                   done_callback);
+}
+
+// Verifies QUIC enablement for globally owned HttpNetworkSessions.
+void VerifyGlobalQuicEnabled(bool quic_enabled_expected) {
+  base::RunLoop run_loop;
+  net::URLRequestContextGetter* system_context =
+      g_browser_process->system_request_context();
+  scoped_refptr<net::URLRequestContextGetter> safe_browsing_context =
+      g_browser_process->safe_browsing_service()->url_request_context();
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(VerifyGlobalQuicEnabledOnIOThread,
+                 base::RetainedRef(system_context), safe_browsing_context,
                  quic_enabled_expected, run_loop.QuitClosure()));
   run_loop.Run();
 }
 
+// This class is used to create an additional Profile during tests. Its main
+// purpose is to wait while Profile creation is in progress, store the Profile
+// instance passed in the OnProfileInitialized callback, and return it.
+class AdditionalProfileCreator {

[mmenke, 2017/01/10 18:03:38]

optional:  Could get rid of this class, and use a stack variable to track the
profile, as I suggested in the quic check case.  Main advantage is that you
don't have the extra class object hanging around.

[pmarko, 2017/01/10 20:47:05]

Done.

+ public:
+  // Create a new Profile. Blocks until Profile creation is done and returns the
+  // newly created Profile.
+  Profile* Create() {
+    ProfileManager* profile_manager = g_browser_process->profile_manager();
+
+    // Create an additional profile.
+    base::FilePath path_profile =
+        profile_manager->GenerateNextProfileDirectoryPath();
+    base::RunLoop run_loop;
+    profile_manager->CreateProfileAsync(
+        path_profile,
+        base::Bind(&AdditionalProfileCreator::OnProfileInitialized,
+                   base::Unretained(this), run_loop.QuitClosure()),
+        base::string16(), std::string(), std::string());
+
+    // Run the message loop to allow profile creation to take place; the loop is
+    // terminated by OnUnblockOnProfileCreation when the profile is created.
+    run_loop.Run();
+
+    return created_profile_;
+  }
+
+ private:
+  void OnProfileInitialized(const base::Closure& closure,
+                            Profile* profile,
+                            Profile::CreateStatus status) {
+    if (status == Profile::CREATE_STATUS_INITIALIZED) {
+      created_profile_ = profile;
+      closure.Run();
+    }
+  }
+
+  // Stores the created profile (passed into OnProfileInitialzed callback).
+  Profile* created_profile_;
+};
+
 }  // namespace
 
 namespace policy {
@@ -55,7 +152,7 @@ namespace policy {
 // when these are being written.
 class QuicAllowedPolicyTestBase: public InProcessBrowserTest {
  public:
-  QuicAllowedPolicyTestBase() : InProcessBrowserTest(){}
+  QuicAllowedPolicyTestBase() : InProcessBrowserTest() {}
 
  protected:
   void SetUpInProcessBrowserTestFixture() override {
@@ -79,7 +176,7 @@ class QuicAllowedPolicyTestBase: public InProcessBrowserTest {
 // Policy QuicAllowed set to false.
 class QuicAllowedPolicyIsFalse: public QuicAllowedPolicyTestBase {
  public:
-  QuicAllowedPolicyIsFalse() : QuicAllowedPolicyTestBase(){}
+  QuicAllowedPolicyIsFalse() : QuicAllowedPolicyTestBase() {}
 
  protected:
   void GetQuicAllowedPolicy(PolicyMap* values) override {
@@ -93,13 +190,14 @@ class QuicAllowedPolicyIsFalse: public QuicAllowedPolicyTestBase {
 };
 
 IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyIsFalse, QuicDisallowed) {
-  VerifyQuicEnabledStatusInIOThread(false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(browser()->profile(), false);
 }
 
 // Policy QuicAllowed set to true.
 class QuicAllowedPolicyIsTrue: public QuicAllowedPolicyTestBase {
  public:
-  QuicAllowedPolicyIsTrue() : QuicAllowedPolicyTestBase(){}
+  QuicAllowedPolicyIsTrue() : QuicAllowedPolicyTestBase() {}
 
  protected:
   void GetQuicAllowedPolicy(PolicyMap* values) override {
@@ -113,13 +211,14 @@ class QuicAllowedPolicyIsTrue: public QuicAllowedPolicyTestBase {
 };
 
 IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyIsTrue, QuicAllowed) {
-  VerifyQuicEnabledStatusInIOThread(true);
+  VerifyGlobalQuicEnabled(true);
+  VerifyProfileQuicEnabled(browser()->profile(), true);
 }
 
 // Policy QuicAllowed is not set.
 class QuicAllowedPolicyIsNotSet: public QuicAllowedPolicyTestBase {
  public:
-  QuicAllowedPolicyIsNotSet() : QuicAllowedPolicyTestBase(){}
+  QuicAllowedPolicyIsNotSet() : QuicAllowedPolicyTestBase() {}
 
  protected:
   void GetQuicAllowedPolicy(PolicyMap* values) override {
@@ -130,7 +229,210 @@ class QuicAllowedPolicyIsNotSet: public QuicAllowedPolicyTestBase {
 };
 
 IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyIsNotSet, NoQuicRegulations) {
-  VerifyQuicEnabledStatusInIOThread(true);
+  VerifyGlobalQuicEnabled(true);
+  VerifyProfileQuicEnabled(browser()->profile(), true);
+}
+
+// Policy QuicAllowed is set dynamically after profile creation.
+// Supports creation of an additional profile.
+class QuicAllowedPolicyDynamicTest : public InProcessBrowserTest {
+ public:
+  QuicAllowedPolicyDynamicTest()
+      : InProcessBrowserTest(), profile_1_(nullptr), profile_2_(nullptr) {}
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+// See TODO.

[mmenke, 2017/01/10 18:03:38]

Also, which TODO?

[mmenke, 2017/01/10 18:03:38]

Indent here is weird.  Maybe move down a line?

[pmarko, 2017/01/10 20:47:05]

My bad, removed.

+#if defined(OS_CHROMEOS)
+    command_line->AppendSwitch(
+        chromeos::switches::kIgnoreUserProfileMappingForTests);
+#endif
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    // Set the overriden policy provider for the first Profile (profile_1_).
+    EXPECT_CALL(policy_for_profile_1_, IsInitializationComplete(testing::_))
+        .WillRepeatedly(testing::Return(true));
+    policy::ProfilePolicyConnectorFactory::GetInstance()
+        ->PushProviderForTesting(&policy_for_profile_1_);
+  }
+
+  void SetUpOnMainThread() override { profile_1_ = browser()->profile(); }
+
+  // Creates a second Profile for testing. The Profile can then be accessed by
+  // profile_2() and its policy by policy_for_profile_2().
+  void CreateSecondProfile() {
+    EXPECT_FALSE(profile_2_);
+
+    EXPECT_CALL(policy_for_profile_2_, IsInitializationComplete(testing::_))
+        .WillRepeatedly(testing::Return(true));
+    policy::ProfilePolicyConnectorFactory::GetInstance()
+        ->PushProviderForTesting(&policy_for_profile_2_);
+
+    AdditionalProfileCreator additional_profile_creator;
+    profile_2_ = additional_profile_creator.Create();
+
+    // Make sure second profile creation does what we think it does.
+    EXPECT_TRUE(profile_1() != profile_2());
+  }
+
+  // Sets the QuicAllowed policy for a Profile.
+  // |provider| is supposed to be the MockConfigurationPolicyProvider for the
+  // Profile, as returned by policy_for_profile_1() / policy_for_profile_2().
+  void SetQuicAllowedPolicy(MockConfigurationPolicyProvider* provider,
+                            bool value) {
+    PolicyMap policy_map;
+    policy_map.Set(key::kQuicAllowed, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                   POLICY_SOURCE_CLOUD,
+                   base::MakeUnique<base::FundamentalValue>(value), nullptr);
+    provider->UpdateChromePolicy(policy_map);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  // Removes all policies for a Profile.
+  // |provider| is supposed to be the MockConfigurationPolicyProvider for the
+  // Profile, as returned by policy_for_profile_1() / policy_for_profile_2().
+  void RemoveAllPolicies(MockConfigurationPolicyProvider* provider) {
+    PolicyMap policy_map;
+    provider->UpdateChromePolicy(policy_map);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  // Returns the first Profile.
+  Profile* profile_1() { return profile_1_; }
+
+  // Returns the second Profile. May only be called after CreateSecondProfile
+  // has been called.
+  Profile* profile_2() {
+    // Only valid after CreateSecondProfile() has been called.
+    EXPECT_TRUE(profile_2_);
+    return profile_2_;
+  }
+
+  // Returns the MockConfigurationPolicyProvider for profile_1.
+  MockConfigurationPolicyProvider* policy_for_profile_1() {
+    return &policy_for_profile_1_;
+  }
+
+  // Returns the MockConfigurationPolicyProvider for profile_2.
+  MockConfigurationPolicyProvider* policy_for_profile_2() {
+    return &policy_for_profile_2_;
+  }
+
+ private:
+  // The first profile.
+  Profile* profile_1_;
+  // The second profile. Only valid after CreateSecondProfile() has been called.
+  Profile* profile_2_;
+
+  // Mock Policy for profile_1_.
+  MockConfigurationPolicyProvider policy_for_profile_1_;
+  // Mock Policy for profile_2_.
+  MockConfigurationPolicyProvider policy_for_profile_2_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuicAllowedPolicyDynamicTest);
+};
+
+// QUIC is disallowed by policy after the profile has been initialized.
+IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyDynamicTest, QuicAllowedFalseThenTrue) {
+  // After browser start, QuicAllowed=false comes in dynamically
+  SetQuicAllowedPolicy(policy_for_profile_1(), false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+
+  // Set the QuicAllowed policy to true again
+  SetQuicAllowedPolicy(policy_for_profile_1(), true);
+  // Effectively, QUIC is sitll disabled because QUIC re-enabling is not

[mmenke, 2017/01/10 18:03:38]

nit:  still

[pmarko, 2017/01/10 20:47:05]

Done.

+  // supported.
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+
+  // Completely remove the QuicAllowed policy
+  RemoveAllPolicies(policy_for_profile_1());
+  // Effectively, QUIC is still disabled because QUIC re-enabling is not
+  // supported.
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+
+  // QuicAllowed=false is set again
+  SetQuicAllowedPolicy(policy_for_profile_1(), false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+}
+
+// QUIC is allowed, then disallowed by policy after the profile has been
+// initialized.
+IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyDynamicTest, QuicAllowedTrueThenFalse) {
+  // After browser start, QuicAllowed=true comes in dynamically
+  SetQuicAllowedPolicy(policy_for_profile_1(), true);
+  VerifyGlobalQuicEnabled(true);
+  VerifyProfileQuicEnabled(profile_1(), true);
+
+  // Completely remove the QuicAllowed policy
+  RemoveAllPolicies(policy_for_profile_1());
+  VerifyGlobalQuicEnabled(true);
+  VerifyProfileQuicEnabled(profile_1(), true);
+
+  // Set the QuicAllowed policy to true again
+  SetQuicAllowedPolicy(policy_for_profile_1(), true);
+  // Effectively, QUIC is sitll disabled because QUIC re-enabling is not
+  // supported.
+  VerifyGlobalQuicEnabled(true);
+  VerifyProfileQuicEnabled(profile_1(), true);
+
+  // Now set QuicAllowed=false
+  SetQuicAllowedPolicy(policy_for_profile_1(), false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+}
+
+// A second Profile is created when QuicAllowed=false policy is in effect for
+// the first profile.
+IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyDynamicTest,
+                       SecondProfileCreatedWhenQuicAllowedFalse) {
+  // If multiprofile mode is not enabled, you can't switch between profiles.
+  if (!profiles::IsMultipleProfilesEnabled())
+    return;
+
+  SetQuicAllowedPolicy(policy_for_profile_1(), false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+
+  CreateSecondProfile();
+
+  // QUIC is disabled in both profiles
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+  VerifyProfileQuicEnabled(profile_2(), false);
+}
+
+// A second Profile is created when no QuicAllowed policy is in effect for the
+// first profile.
+// Then QuicAllowed=false policy is dynamically set for both profiles.
+IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyDynamicTest,
+                       QuicAllowedFalseAfterTwoProfilesCreated) {
+  // If multiprofile mode is not enabled, you can't switch between profiles.
+  if (!profiles::IsMultipleProfilesEnabled())
+    return;
+
+  CreateSecondProfile();
+
+  // QUIC is enabled in both profiles
+  VerifyGlobalQuicEnabled(true);
+  VerifyProfileQuicEnabled(profile_1(), true);
+  VerifyProfileQuicEnabled(profile_2(), true);
+
+  // Disable QUIC in first profile
+  SetQuicAllowedPolicy(policy_for_profile_1(), false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+  VerifyProfileQuicEnabled(profile_2(), true);
+
+  // Disable QUIC in second profile
+  SetQuicAllowedPolicy(policy_for_profile_2(), false);
+  VerifyGlobalQuicEnabled(false);
+  VerifyProfileQuicEnabled(profile_1(), false);
+  VerifyProfileQuicEnabled(profile_2(), false);
 }
 
 }  // namespace policy