Fix SecurityStyleChangedObserverNonsecureConnection test

chrome/browser/ui/browser_browsertest.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/devtools/devtools_window_testing.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 65 matching lines @@
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/uninstall_reason.h"
 #include "extensions/common/constants.h"
 #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/cert/x509_certificate.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"
 #include "net/test/url_request/url_request_mock_http_job.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_test_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/page_transition_types.h"
 
 #if defined(OS_MACOSX)
@@ skipping 2967 matching lines @@
   EXPECT_EQ(0u, observer.latest_explanations().broken_explanations.size());
   CheckSecureExplanations(observer.latest_explanations().secure_explanations,
                           VALID_CERTIFICATE, browser());
   EXPECT_TRUE(observer.latest_explanations().scheme_is_cryptographic);
   EXPECT_FALSE(observer.latest_explanations().displayed_insecure_content);
   EXPECT_FALSE(observer.latest_explanations().ran_insecure_content);
 }
 
 namespace {
 
+// After AddNonsecureUrlHandler() 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 URLRequestObsoleteTLSJob : public net::URLRequestMockHTTPJob {
+ public:
+  URLRequestObsoleteTLSJob(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)

[davidben, 2016/04/29 21:22:49]

As of recently, const scoped_refptr<T>& is dead!!! (I hated it so much. So
ugly...)

https://www.chromium.org/developers/smart-pointer-guidelines

Instead, since you're taking a reference, you can write just plain
scoped_refptr<T> worker_pool and then cert_(std::move(cert)) to save the
refcount hop.

[estark, 2016/04/29 22:09:56]

Done.

+      : 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);

[davidben, 2016/04/29 21:22:49]

net::SSLConnectionStatusSetVersion?

[estark, 2016/04/29 22:09:56]

Done.

+    const uint16_t kTlsEcdheRsaWithAes128GcmSha256 = 0xc02f;
+    net::SSLConnectionStatusSetCipherSuite(kTlsEcdheRsaWithAes128GcmSha256,
+                                           &info->ssl_info.connection_status);

[davidben, 2016/04/29 21:22:49]

This is kind of weird. That pair isn't actually possible.
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA maybe?

[estark, 2016/04/29 22:09:56]

Done.

+    info->ssl_info.cert = cert_;
+  }
+
+ protected:
+  ~URLRequestObsoleteTLSJob() override {}
+
+ private:
+  const scoped_refptr<net::X509Certificate> cert_;
+
+  DISALLOW_COPY_AND_ASSIGN(URLRequestObsoleteTLSJob);
+};
+
+// A URLRequestInterceptor that handles requests with
+// URLRequestObsoleteTLSJob jobs.
+class URLRequestNonsecureInterceptor : public net::URLRequestInterceptor {

[davidben, 2016/04/29 21:22:49]

I'm not especially familiar with all the mess around how to make these things so
I assume mmenke's previously pass got it.

[mmenke, 2016/04/29 21:26:49]

Yea, I reviewed this mess.  And certainly agree it's an ugly API.  :(

+ public:
+  URLRequestNonsecureInterceptor(
+      const base::FilePath& base_path,
+      const scoped_refptr<base::SequencedWorkerPool>& worker_pool,

[davidben, 2016/04/29 21:22:49]

Ditto about const-ref of scoped_refptr nightmares.

[estark, 2016/04/29 22:09:56]

Done.

+      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 URLRequestObsoleteTLSJob(
+        request, network_delegate, base_path_, cert_,
+        worker_pool_->GetTaskRunnerWithShutdownBehavior(
+            base::SequencedWorkerPool::SKIP_ON_SHUTDOWN));
+  }
+
+ private:
+  const base::FilePath base_path_;
+  const scoped_refptr<base::SequencedWorkerPool> worker_pool_;
+  const scoped_refptr<net::X509Certificate> cert_;
+
+  DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureInterceptor);
+};
+
+// Installs a handler to serve HTTPS requests to
+// |kMockNonsecureHostname| with connections that have obsolete TLS
+// settings.
+void AddNonsecureUrlHandler(
+    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");

[davidben, 2016/04/29 21:22:49]

Nit: Might be worth an ASSERT_TRUE(cert_) since that function can fail.

[estark, 2016/04/29 22:09:56]

Done.

+  }
+
   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,
+            &AddNonsecureUrlHandler, 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 =
       browser()->tab_strip_model()->GetActiveWebContents();
   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());
+
+  // 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),
               explanation.summary);
   }
 }
 
 namespace {
 class JSBooleanResultGetter {
  public:
@@ skipping 142 matching lines @@
     Browser* browser = new Browser(params);
     gfx::Rect bounds = browser->window()->GetBounds();
 
     // Should be EXPECT_EQ, but this width is inconsistent across platforms.
     // See https://crbug.com/567925.
     EXPECT_GE(bounds.width(), 100);
     EXPECT_EQ(122, bounds.height());
     browser->window()->Close();
   }
 }