Fix SecurityStyleChangedObserverNonsecureConnection test

chrome/browser/ui/browser_browsertest.cc

Index: chrome/browser/ui/browser_browsertest.cc
diff --git a/chrome/browser/ui/browser_browsertest.cc b/chrome/browser/ui/browser_browsertest.cc
index be237af4bff517cf9a48cef8e4e658a886e14f1c..42ca72d6c9347a18005158215fb3e7400013eb79 100644
--- a/chrome/browser/ui/browser_browsertest.cc
+++ b/chrome/browser/ui/browser_browsertest.cc
@@ -105,7 +105,11 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_data_directory.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/ssl/ssl_cipher_suite_names.h"
+#include "net/ssl/ssl_connection_status_flags.h"
+#include "net/test/cert_test_util.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/request_handler_util.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
@@ -3093,27 +3097,117 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, SecurityStyleChangedObserverGoBack) {
 
 namespace {
 
+// After AddNonsecureUrlHandlers() is called, requests to this hostname
+// will use obsolete TLS settings.
+const char kMockNonsecureHostname[] = "example-nonsecure.test";
+
+// A URLRequestMockHTTPJob that mocks a TLS connection with an obsolete
+// protocol version.
+class URLRequestNonsecureConnection : public net::URLRequestMockHTTPJob {

[mmenke, 2016/04/27 20:19:44]

While you're here, mind renaming this to "URLRequestNonsecureConnectionJob" or
somesuch?  It's not actually a connection - quite a few layers above classes
that represent connections.

[estark, 2016/04/27 22:39:36]

Done.

+ public:
+  URLRequestNonsecureConnection(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate,
+      const base::FilePath& file_path,
+      const scoped_refptr<net::X509Certificate>& cert,
+      const scoped_refptr<base::TaskRunner>& task_runner)
+      : net::URLRequestMockHTTPJob(request,
+                                   network_delegate,
+                                   file_path,
+                                   task_runner),
+        cert_(cert) {}
+
+  void GetResponseInfo(net::HttpResponseInfo* info) override {
+    net::URLRequestMockHTTPJob::GetResponseInfo(info);
+    info->ssl_info.connection_status = (net::SSL_CONNECTION_VERSION_TLS1_1
+                                        << net::SSL_CONNECTION_VERSION_SHIFT);
+    const uint16_t kTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 = 0xc02f;

[mmenke, 2016/04/27 20:19:44]

naming style here is wrong.

As a constant, should be named something like kTlsEdchRsaWithAes128GcmSha256.

[estark, 2016/04/27 22:39:36]

Done.

+    net::SSLConnectionStatusSetCipherSuite(
+        kTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+        &info->ssl_info.connection_status);
+    info->ssl_info.cert = cert_;
+  }
+
+ protected:
+  ~URLRequestNonsecureConnection() override {}
+
+ private:
+  const scoped_refptr<net::X509Certificate> cert_;

[mmenke, 2016/04/27 20:19:44]

include ref_counted.h and net/cert/x509_certificate.h

[estark, 2016/04/27 22:39:35]

Done.

+  DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureConnection);

[mmenke, 2016/04/27 20:19:44]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[estark, 2016/04/27 22:39:35]

Done.

+};
+
+// A URLRequestInterceptor that handles requests with
+// URLRequestNonsecureConnection jobs.
+class URLRequestNonsecureInterceptor : public net::URLRequestInterceptor {
+ public:
+  URLRequestNonsecureInterceptor(
+      const base::FilePath& base_path,
+      const scoped_refptr<base::SequencedWorkerPool>& worker_pool,
+      const scoped_refptr<net::X509Certificate>& cert)
+      : base_path_(base_path), worker_pool_(worker_pool), cert_(cert) {}
+
+  ~URLRequestNonsecureInterceptor() override {}
+
+  // net::URLRequestInterceptor:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new URLRequestNonsecureConnection(
+        request, network_delegate, base_path_, cert_,
+        worker_pool_->GetTaskRunnerWithShutdownBehavior(
+            base::SequencedWorkerPool::SKIP_ON_SHUTDOWN));
+  }
+
+ private:
+  base::FilePath base_path_;

[mmenke, 2016/04/27 20:19:44]

const?

[mmenke, 2016/04/27 20:19:44]

need to include file_path.h

[estark, 2016/04/27 22:39:35]

it's already included

[estark, 2016/04/27 22:39:36]

Done.

+  const scoped_refptr<base::SequencedWorkerPool> worker_pool_;

[mmenke, 2016/04/27 20:19:44]

Should include the SequencedWorkerPool header

[estark, 2016/04/27 22:39:35]

Done.

+  const scoped_refptr<net::X509Certificate> cert_;
+  DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureInterceptor);

[mmenke, 2016/04/27 20:19:44]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[estark, 2016/04/27 22:39:35]

Done.

+};
+
+// Installs a handler to serve HTTPS requests to
+// |kMockNonsecureHostname| with connections that have obsolete TLS
+// settings.
+void AddNonsecureUrlHandlers(

[mmenke, 2016/04/27 20:19:44]

nit:  Handlers -> Handler

[estark, 2016/04/27 22:39:35]

Done.

+    const base::FilePath& base_path,
+    const scoped_refptr<net::X509Certificate>& cert,
+    const scoped_refptr<base::SequencedWorkerPool>& worker_pool) {
+  net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
+  filter->AddHostnameInterceptor(
+      "https", kMockNonsecureHostname,
+      std::unique_ptr<net::URLRequestInterceptor>(
+          new URLRequestNonsecureInterceptor(base_path, worker_pool, cert)));
+}
+
 class BrowserTestNonsecureURLRequest : public BrowserTest {
  public:
-  BrowserTestNonsecureURLRequest() : BrowserTest() {}
+  BrowserTestNonsecureURLRequest() : BrowserTest(), cert_(nullptr) {}
+
+  void SetUpInProcessBrowserTestFixture() override {
+    cert_ =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");

[mmenke, 2016/04/27 20:19:44]

Is there some reason this can't just be done in the constructor?  Would be
simpler if we could just do it in SetUpOnMainThread and get rid of |cert_| all
together, but I assume that runs into thread restrictions.

[estark, 2016/04/27 22:39:35]

Yeah, I did it this way because it complained about reading it on a non-IO
thread.

+  }
+
   void SetUpOnMainThread() override {
-    base::FilePath root_http;
-    PathService::Get(chrome::DIR_TEST_DATA, &root_http);
+    base::FilePath serve_file;
+    PathService::Get(chrome::DIR_TEST_DATA, &serve_file);
+    serve_file = serve_file.Append(FILE_PATH_LITERAL("title1.html"));
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(
-            &net::URLRequestMockHTTPJob::AddUrlHandlers, root_http,
+            &AddNonsecureUrlHandlers, serve_file, cert_,
             make_scoped_refptr(content::BrowserThread::GetBlockingPool())));
   }
 
  private:
+  scoped_refptr<net::X509Certificate> cert_;
   DISALLOW_COPY_AND_ASSIGN(BrowserTestNonsecureURLRequest);
 };
 
 }  // namespace
 
-// Tests that a nonsecure connection does not get a secure connection
-// explanation.
+// Tests that a connection with obsolete TLS settings does not get a
+// secure connection explanation.
 IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
                        SecurityStyleChangedObserverNonsecureConnection) {
   content::WebContents* web_contents =
@@ -3121,7 +3215,16 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
   SecurityStyleTestObserver observer(web_contents);
 
   ui_test_utils::NavigateToURL(
-      browser(), net::URLRequestMockHTTPJob::GetMockHttpsUrl(std::string()));
+      browser(), GURL(std::string("https://") + kMockNonsecureHostname));
+
+  // The security style of the page doesn't get downgraded for obsolete
+  // TLS settings, so it should remain at SECURITY_STYLE_AUTHENTICATED.
+  EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATED,
+            observer.latest_security_style());

[mmenke, 2016/04/27 20:19:44]

I think you really need to have an SSL expert review this.  I can certainly
verify that youre injecting the job correctly (which you are), but I don't have
the background to verify anything else (Whether the job is correctly behaving
like it's running into an error of this type, or this test is checking for the
right behavior).

+
+  // The messages explaining the security style do, however, get
+  // downgraded: SECURE_PROTOCOL_AND_CIPHERSUITE should not show up when
+  // the TLS settings are obsolete.
   for (const auto& explanation :
        observer.latest_explanations().secure_explanations) {
     EXPECT_NE(l10n_util::GetStringUTF8(IDS_SECURE_PROTOCOL_AND_CIPHERSUITE),