Add back prefs::kSSL3Enabled and prefs::kTLS1Enabled, but control

chrome/browser/net/ssl_config_service_manager_pref_unittest.cc

Index: chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
diff --git a/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc b/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
index fe744e728e7db8c97b4e244ec1b3d8ed80933479..4a11ae51c643049621f2521e38c8d088bd3faf1f 100644
--- a/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
+++ b/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
@@ -4,9 +4,13 @@
 
 #include "chrome/browser/net/pref_proxy_config_service.h"
 
+#include "base/command_line.h"
 #include "base/message_loop.h"
 #include "base/values.h"
 #include "chrome/browser/net/ssl_config_service_manager.h"
+#include "chrome/browser/prefs/pref_service_mock_builder.h"
+#include "chrome/browser/prefs/testing_pref_store.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_pref_service.h"
 #include "content/browser/browser_thread.h"
@@ -20,37 +24,24 @@ using net::SSLConfigService;
 
 class SSLConfigServiceManagerPrefTest : public testing::Test {
  public:
-  SSLConfigServiceManagerPrefTest() {}
-
-  virtual void SetUp() {
-    message_loop_.reset(new MessageLoop());
-    ui_thread_.reset(
-        new BrowserThread(BrowserThread::UI, message_loop_.get()));
-    io_thread_.reset(
-        new BrowserThread(BrowserThread::IO, message_loop_.get()));
-    pref_service_.reset(new TestingPrefService());
-    SSLConfigServiceManager::RegisterPrefs(pref_service_.get());

[wtc, 2011/10/31 19:48:24]

Good cleanup!

Why doesn't this class need the pref_service_ member?

[Ryan Sleevi, 2011/11/01 03:01:19]

It's only used by two of the tests as a member (GoodDisabled*/BadDisabled*), so
I moved it to be a stack variable for those.

The two remaining tests (IgnoreLegacy, CommandLineOverrides) use the builder,
and thus don't use a TestingPrefService

-  }
-
-  virtual void TearDown() {
-    pref_service_.reset();
-    io_thread_.reset();
-    ui_thread_.reset();
-    message_loop_.reset();
-  }
+  SSLConfigServiceManagerPrefTest()
+      : ui_thread_(BrowserThread::UI, &message_loop_),
+        io_thread_(BrowserThread::IO, &message_loop_) {}
 
  protected:
-  scoped_ptr<MessageLoop> message_loop_;
-  scoped_ptr<BrowserThread> ui_thread_;
-  scoped_ptr<BrowserThread> io_thread_;
-  scoped_ptr<TestingPrefService> pref_service_;
+  MessageLoop message_loop_;
+  BrowserThread ui_thread_;
+  BrowserThread io_thread_;
 };
 
 // Test that cipher suites can be disabled. "Good" refers to the fact that
 // every value is expected to be successfully parsed into a cipher suite.
 TEST_F(SSLConfigServiceManagerPrefTest, GoodDisabledCipherSuites) {
+  TestingPrefService pref_service;
+  SSLConfigServiceManager::RegisterPrefs(&pref_service);
+
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(pref_service_.get()));
+      SSLConfigServiceManager::CreateDefaultManager(&pref_service));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -62,11 +53,11 @@ TEST_F(SSLConfigServiceManagerPrefTest, GoodDisabledCipherSuites) {
   ListValue* list_value = new ListValue();
   list_value->Append(Value::CreateStringValue("0x0004"));
   list_value->Append(Value::CreateStringValue("0x0005"));
-  pref_service_->SetUserPref(prefs::kCipherSuiteBlacklist, list_value);
+  pref_service.SetUserPref(prefs::kCipherSuiteBlacklist, list_value);
 
   // Pump the message loop to notify the SSLConfigServiceManagerPref that the
   // preferences changed.
-  message_loop_->RunAllPending();
+  message_loop_.RunAllPending();
 
   SSLConfig config;
   config_service->GetSSLConfig(&config);
@@ -81,8 +72,11 @@ TEST_F(SSLConfigServiceManagerPrefTest, GoodDisabledCipherSuites) {
 // there are one or more non-cipher suite strings in the preference. They
 // should be ignored.
 TEST_F(SSLConfigServiceManagerPrefTest, BadDisabledCipherSuites) {
+  TestingPrefService pref_service;
+  SSLConfigServiceManager::RegisterPrefs(&pref_service);
+
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(pref_service_.get()));
+      SSLConfigServiceManager::CreateDefaultManager(&pref_service));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -96,11 +90,11 @@ TEST_F(SSLConfigServiceManagerPrefTest, BadDisabledCipherSuites) {
   list_value->Append(Value::CreateStringValue("TLS_NOT_WITH_A_CIPHER_SUITE"));
   list_value->Append(Value::CreateStringValue("0x0005"));
   list_value->Append(Value::CreateStringValue("0xBEEFY"));
-  pref_service_->SetUserPref(prefs::kCipherSuiteBlacklist, list_value);
+  pref_service.SetUserPref(prefs::kCipherSuiteBlacklist, list_value);
 
   // Pump the message loop to notify the SSLConfigServiceManagerPref that the
   // preferences changed.
-  message_loop_->RunAllPending();
+  message_loop_.RunAllPending();
 
   SSLConfig config;
   config_service->GetSSLConfig(&config);
@@ -110,3 +104,110 @@ TEST_F(SSLConfigServiceManagerPrefTest, BadDisabledCipherSuites) {
   EXPECT_EQ(0x0004, config.disabled_cipher_suites[0]);
   EXPECT_EQ(0x0005, config.disabled_cipher_suites[1]);
 }
+
+// Test that existing user settings for TLS1.0/SSL3.0 are both ignored and
+// cleared from user preferences.
+TEST_F(SSLConfigServiceManagerPrefTest, IgnoreLegacySSLSettings) {
+  TestingPrefStore* user_prefs = new TestingPrefStore;

[wtc, 2011/10/31 19:48:24]

Is user_prefs heap-allocated because the builder.WithUserPrefs()
call below will take ownership of it?

[Ryan Sleevi, 2011/11/01 03:01:19]

Right. |user_prefs| is RefCounted, and WithUserPrefs will AddRef (which in this
case, assumes ownership). The only other tests using the Builder used this
pattern as well.

However, to be explicit, I've made them both scoped_refptr<>'s, so it behaves
fine and won't leak on any test failures (though they're all EXPECT()s before
the ownership transfer)

+
+  // SSL3.0 and TLS1.0 used to be user-definable prefs. They are now used as
+  // command-line options. Ensure any existing user prefs are ignored in
+  // favour of the command-line flags.
+  user_prefs->SetBoolean(prefs::kSSL3Enabled, false);
+  user_prefs->SetBoolean(prefs::kTLS1Enabled, false);
+
+  // Ensure the preferences exist initially.
+  bool is_ssl3_enabled = false;
+  EXPECT_TRUE(user_prefs->GetBoolean(prefs::kSSL3Enabled, &is_ssl3_enabled));
+  EXPECT_FALSE(is_ssl3_enabled);
+
+  bool is_tls1_enabled = false;

[wtc, 2011/10/31 19:48:24]

Nit: perhaps initialize is_ssl3_enabled and is_tls1_enabled
to true to strengthen these tests?

[Ryan Sleevi, 2011/11/01 03:01:19]

Doh, I even had that originally. Done.

+  EXPECT_TRUE(user_prefs->GetBoolean(prefs::kTLS1Enabled, &is_tls1_enabled));
+  EXPECT_FALSE(is_tls1_enabled);
+
+  PrefServiceMockBuilder builder;
+  builder.WithUserPrefs(user_prefs);
+  scoped_ptr<PrefService> pref_service(builder.Create());
+
+  SSLConfigServiceManager::RegisterPrefs(pref_service.get());
+
+  scoped_ptr<SSLConfigServiceManager> config_manager(
+      SSLConfigServiceManager::CreateDefaultManager(pref_service.get()));
+  ASSERT_TRUE(config_manager.get());
+  scoped_refptr<SSLConfigService> config_service(config_manager->Get());
+  ASSERT_TRUE(config_service.get());
+
+  SSLConfig ssl_config;
+  config_service->GetSSLConfig(&ssl_config);
+  // The default value in the absence of command-line options is that both
+  // protocols are enabled.
+  EXPECT_TRUE(ssl_config.ssl3_enabled);
+  EXPECT_TRUE(ssl_config.tls1_enabled);
+
+  // The existing user settings should be removed from the pref_service.
+  EXPECT_FALSE(pref_service->HasPrefPath(prefs::kSSL3Enabled));
+  EXPECT_FALSE(pref_service->HasPrefPath(prefs::kTLS1Enabled));
+
+  // Explicitly double-check the settings are not in the user preference
+  // store.
+  EXPECT_FALSE(user_prefs->GetBoolean(prefs::kSSL3Enabled, &is_ssl3_enabled));
+  EXPECT_FALSE(user_prefs->GetBoolean(prefs::kTLS1Enabled, &is_tls1_enabled));
+}
+
+// Test that command-line settings for TLS1.0/SSL3.0 are respected, that they
+// disregard any existing user preferences, and that they do not persist to
+// the user preferences files.
+TEST_F(SSLConfigServiceManagerPrefTest, CommandLineOverridesUserPrefs) {
+  TestingPrefStore* user_prefs = new TestingPrefStore;
+
+  // Explicitly enable SSL3.0/TLS1.0 in the user preferences, to mirror the
+  // more common legacy file.
+  user_prefs->SetBoolean(prefs::kSSL3Enabled, true);
+  user_prefs->SetBoolean(prefs::kTLS1Enabled, true);
+
+  // Ensure the preferences exist initially.
+  bool is_ssl3_enabled = false;
+  EXPECT_TRUE(user_prefs->GetBoolean(prefs::kSSL3Enabled, &is_ssl3_enabled));
+  EXPECT_TRUE(is_ssl3_enabled);
+
+  bool is_tls1_enabled = false;
+  EXPECT_TRUE(user_prefs->GetBoolean(prefs::kTLS1Enabled, &is_tls1_enabled));
+  EXPECT_TRUE(is_tls1_enabled);
+
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  command_line.AppendSwitch(switches::kDisableSSL3);
+  command_line.AppendSwitch(switches::kDisableTLS1);
+
+  PrefServiceMockBuilder builder;
+  builder.WithUserPrefs(user_prefs);
+  builder.WithCommandLine(&command_line);
+  scoped_ptr<PrefService> pref_service(builder.Create());
+
+  SSLConfigServiceManager::RegisterPrefs(pref_service.get());
+
+  scoped_ptr<SSLConfigServiceManager> config_manager(
+      SSLConfigServiceManager::CreateDefaultManager(pref_service.get()));
+  ASSERT_TRUE(config_manager.get());
+  scoped_refptr<SSLConfigService> config_service(config_manager->Get());
+  ASSERT_TRUE(config_service.get());
+
+  SSLConfig ssl_config;
+  config_service->GetSSLConfig(&ssl_config);
+  // Command-line flags to disable should override the user preferences to
+  // enable.
+  EXPECT_FALSE(ssl_config.ssl3_enabled);
+  EXPECT_FALSE(ssl_config.tls1_enabled);
+
+  // Explicitly double-check the settings are not in the user preference
+  // store.
+  const PrefService::Preference* ssl3_enabled_pref =
+      pref_service->FindPreference(prefs::kSSL3Enabled);
+  EXPECT_FALSE(ssl3_enabled_pref->IsUserModifiable());
+
+  const PrefService::Preference* tls1_enabled_pref =
+      pref_service->FindPreference(prefs::kTLS1Enabled);
+  EXPECT_FALSE(tls1_enabled_pref->IsUserModifiable());
+
+  EXPECT_FALSE(user_prefs->GetBoolean(prefs::kSSL3Enabled, &is_ssl3_enabled));
+  EXPECT_FALSE(user_prefs->GetBoolean(prefs::kTLS1Enabled, &is_tls1_enabled));
+}