Update SSL error handling code to account for Subject CN deprecation

chrome/browser/ssl/ssl_error_handler_unittest.cc

Index: chrome/browser/ssl/ssl_error_handler_unittest.cc
diff --git a/chrome/browser/ssl/ssl_error_handler_unittest.cc b/chrome/browser/ssl/ssl_error_handler_unittest.cc
index 19471ae78d789477045d3911fed819458300186c..520f820f6cfe178901b429745e43cf7e22f9558a 100644
--- a/chrome/browser/ssl/ssl_error_handler_unittest.cc
+++ b/chrome/browser/ssl/ssl_error_handler_unittest.cc
@@ -201,6 +201,54 @@ class SSLErrorHandlerNameMismatchTest : public ChromeRenderViewHostTestHarness {
     ChromeRenderViewHostTestHarness::SetUp();
     SSLErrorHandler::ResetConfigForTesting();
     SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
+    ssl_info_.cert = net::ImportCertFromFile(
+        net::GetTestCertsDirectory(), "subjectAltName_www_example_com.pem");
+    ssl_info_.cert_status = net::CERT_STATUS_COMMON_NAME_INVALID;
+    ssl_info_.public_key_hashes.push_back(
+        net::HashValue(kCertPublicKeyHashValue));
+
+    delegate_ =
+        new TestSSLErrorHandlerDelegate(profile(), web_contents(), ssl_info_);
+    error_handler_.reset(new TestSSLErrorHandler(
+        std::unique_ptr<SSLErrorHandler::Delegate>(delegate_), web_contents(),
+        profile(), net::MapCertStatusToNetError(ssl_info_.cert_status),
+        ssl_info_,
+        GURL(),  // request_url
+        base::Callback<void(content::CertificateRequestResultType)>()));
+  }
+
+  void TearDown() override {
+    EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+    error_handler_.reset(nullptr);
+    SSLErrorHandler::ResetConfigForTesting();
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  TestSSLErrorHandler* error_handler() { return error_handler_.get(); }
+  TestSSLErrorHandlerDelegate* delegate() { return delegate_; }
+
+  const net::SSLInfo& ssl_info() { return ssl_info_; }
+
+ private:
+  net::SSLInfo ssl_info_;
+  std::unique_ptr<TestSSLErrorHandler> error_handler_;
+  TestSSLErrorHandlerDelegate* delegate_;
+  base::FieldTrialList field_trial_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerNameMismatchTest);
+};
+
+// A class to test name mismatch errors, where the certificate lacks a
+// SubjectAltName. Creates an error handler with a name mismatch error.
+class SSLErrorHandlerNameMismatchNoSANTest

[estark, 2017/04/03 02:01:42]

This test fixture is the same as the one above except for the certificate,
right? If so, perhaps you could stick with one test fixture, and pull out
SetUp() lines 242-265 into a SetUpErrorHandler() method that takes a certificate
as an argument.

[elawrence, 2017/04/04 15:52:28]

I didn't know how to do that in a straightforward way because I don't control
the GoogleTest calls to SetUp(). So instead I've created a simple derived
fixture, a pattern I've seen elsewhere. Reasonable?

[estark, 2017/04/04 17:22:07]

Oh, I was thinking you could just call SetUpErrorHandler() in the test itself,
but I like what you did better.

+    : public ChromeRenderViewHostTestHarness {
+ public:
+  SSLErrorHandlerNameMismatchNoSANTest() : field_trial_list_(nullptr) {}
+
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    SSLErrorHandler::ResetConfigForTesting();
+    SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
     ssl_info_.cert =
         net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
     ssl_info_.cert_status = net::CERT_STATUS_COMMON_NAME_INVALID;
@@ -235,7 +283,7 @@ class SSLErrorHandlerNameMismatchTest : public ChromeRenderViewHostTestHarness {
   TestSSLErrorHandlerDelegate* delegate_;
   base::FieldTrialList field_trial_list_;
 
-  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerNameMismatchTest);
+  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerNameMismatchNoSANTest);
 };
 
 // A class to test the captive portal certificate list feature. Creates an error
@@ -686,6 +734,30 @@ TEST_F(SSLErrorHandlerNameMismatchTest,
                                SSLErrorHandler::WWW_MISMATCH_URL_AVAILABLE, 1);
 }
 
+// No suggestions should be requested if certificate lacks a SubjectAltName.
+TEST_F(SSLErrorHandlerNameMismatchNoSANTest,
+       SSLCommonNameMismatchHandlingRequiresSubjectAltName) {
+  base::HistogramTester histograms;
+  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+  delegate()->set_suggested_url_exists();
+  error_handler()->StartHandlingError();
+
+  EXPECT_FALSE(delegate()->suggested_url_checked());
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(delegate()->ssl_interstitial_shown());
+  EXPECT_FALSE(delegate()->redirected_to_suggested_url());
+
+  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 2);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::HANDLE_ALL, 1);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::WWW_MISMATCH_FOUND, 0);
+  histograms.ExpectBucketCount(
+      SSLErrorHandler::GetHistogramNameForTesting(),
+      SSLErrorHandler::SHOW_SSL_INTERSTITIAL_OVERRIDABLE, 1);
+}
+
 TEST_F(SSLErrorHandlerNameMismatchTest,
        ShouldShowSSLInterstitialOnInvalidUrlCheckResult) {
   base::HistogramTester histograms;