Attempt an on-demand time fetch when encountering a date invalid error

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 0dfb7d1b918dbc6922cb0ac2d9528825ccbaf2df..34310ee6e4fa1bbcf4341b5af4708537a04d44f7 100644
--- a/chrome/browser/ssl/ssl_error_handler_unittest.cc
+++ b/chrome/browser/ssl/ssl_error_handler_unittest.cc
@@ -8,6 +8,8 @@
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
+#include "base/test/simple_test_clock.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "base/time/time.h"
 #include "chrome/browser/captive_portal/captive_portal_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -16,13 +18,22 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/captive_portal/captive_portal_testing_utils.h"
+#include "components/network_time/network_time_test_utils.h"
+#include "components/network_time/network_time_tracker.h"
+#include "components/prefs/testing_pref_service.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "net/base/net_errors.h"
+#include "net/cert/cert_status_flags.h"
 #include "net/cert/x509_certificate.h"
+#include "net/http/http_response_headers.h"
 #include "net/ssl/ssl_info.h"
 #include "net/test/cert_test_util.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "net/test/test_certificate_data.h"
 #include "net/test/test_data_directory.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class SSLErrorHandlerForTest : public SSLErrorHandler {
@@ -32,7 +43,7 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
                          const net::SSLInfo& ssl_info)
       : SSLErrorHandler(
             web_contents,
-            net::ERR_CERT_COMMON_NAME_INVALID,
+            net::MapCertStatusToNetError(ssl_info.cert_status),
             ssl_info,
             GURL(),
             0,
@@ -43,6 +54,7 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
         suggested_url_exists_(false),
         suggested_url_checked_(false),
         ssl_interstitial_shown_(false),
+        bad_clock_interstitial_shown_(false),
         captive_portal_interstitial_shown_(false),
         redirected_to_suggested_url_(false),
         is_overridable_error_(true) {}
@@ -72,6 +84,9 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
   int captive_portal_interstitial_shown() const {
     return captive_portal_interstitial_shown_;
   }
+  bool bad_clock_interstitial_shown() const {
+    return bad_clock_interstitial_shown_;
+  }
   bool suggested_url_checked() const { return suggested_url_checked_; }
   bool redirected_to_suggested_url() const {
     return redirected_to_suggested_url_;
@@ -85,6 +100,7 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
     suggested_url_exists_ = false;
     suggested_url_checked_ = false;
     ssl_interstitial_shown_ = false;
+    bad_clock_interstitial_shown_ = false;
     captive_portal_interstitial_shown_ = false;
     redirected_to_suggested_url_ = false;
   }
@@ -104,6 +120,11 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
 
   void ShowSSLInterstitial() override { ssl_interstitial_shown_ = true; }
 
+  void ShowBadClockInterstitial(const base::Time& now,
+                                ssl_errors::ClockState clock_state) override {
+    bad_clock_interstitial_shown_ = true;
+  }
+
   void ShowCaptivePortalInterstitial(const GURL& landing_url) override {
     captive_portal_interstitial_shown_ = true;
   }
@@ -123,6 +144,7 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
   bool suggested_url_exists_;
   bool suggested_url_checked_;
   bool ssl_interstitial_shown_;
+  bool bad_clock_interstitial_shown_;
   bool captive_portal_interstitial_shown_;
   bool redirected_to_suggested_url_;
   bool is_overridable_error_;
@@ -130,9 +152,9 @@ class SSLErrorHandlerForTest : public SSLErrorHandler {
   DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerForTest);
 };
 
-class SSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {
+class CommonNameSSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {

[meacer, 2016/10/26 20:03:02]

nit: maybe rename to SSLErrorHandlerNameMismatchTest?

That way we can use --gtest_filter=SSLErrorHandler* for all tests here.

[estark, 2016/10/31 16:03:25]

Done.

  public:
-  SSLErrorHandlerTest() : field_trial_list_(nullptr) {}
+  CommonNameSSLErrorHandlerTest() : field_trial_list_(nullptr) {}
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
@@ -162,12 +184,82 @@ class SSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {
   net::SSLInfo ssl_info_;
   std::unique_ptr<SSLErrorHandlerForTest> error_handler_;
   base::FieldTrialList field_trial_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CommonNameSSLErrorHandlerTest);
+};
+
+class DateInvalidSSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {

[meacer, 2016/10/26 20:03:03]

And this one to SSLErrorHandlerDateInvalidTest?

[estark, 2016/10/31 16:03:25]

Done.

+ public:
+  DateInvalidSSLErrorHandlerTest()
+      : field_trial_test_(new network_time::FieldTrialTest()),
+        clock_(new base::SimpleTestClock),
+        tick_clock_(new base::SimpleTestTickClock),
+        test_server_(new net::EmbeddedTestServer) {
+    SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD);
+    network_time::NetworkTimeTracker::RegisterPrefs(pref_service_.registry());
+  }
+
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+
+    field_trial_test()->SetNetworkQueriesWithVariationsService(
+        false, 0.0, network_time::FieldTrialTest::DISABLE_FETCHES_ON_DEMAND);
+    tracker_.reset(new network_time::NetworkTimeTracker(
+        std::unique_ptr<base::Clock>(clock_),
+        std::unique_ptr<base::TickClock>(tick_clock_), &pref_service_,
+        new net::TestURLRequestContextGetter(
+            content::BrowserThread::GetTaskRunnerForThread(
+                content::BrowserThread::IO))));
+
+    // Do this to be sure that |is_null| returns false.
+    clock_->Advance(base::TimeDelta::FromDays(111));
+    tick_clock_->Advance(base::TimeDelta::FromDays(222));
+
+    SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta());
+    ssl_info_.cert =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+    ssl_info_.cert_status = net::CERT_STATUS_DATE_INVALID;
+    error_handler_.reset(
+        new SSLErrorHandlerForTest(profile(), web_contents(), ssl_info_));
+    error_handler_->SetNetworkTimeTrackerForTest(tracker_.get());
+  }
+
+  void TearDown() override {
+    if (error_handler()) {
+      EXPECT_FALSE(error_handler()->IsTimerRunning());
+      error_handler_.reset(nullptr);
+    }
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  SSLErrorHandlerForTest* error_handler() { return error_handler_.get(); }
+
+  network_time::FieldTrialTest* field_trial_test() {
+    return field_trial_test_.get();
+  }
+
+  network_time::NetworkTimeTracker* tracker() { return tracker_.get(); }
+
+  net::EmbeddedTestServer* test_server() { return test_server_.get(); }
+
+  void ClearErrorHandler() { error_handler_.reset(nullptr); }
+
+ private:
+  net::SSLInfo ssl_info_;
+  std::unique_ptr<SSLErrorHandlerForTest> error_handler_;
+  std::unique_ptr<network_time::FieldTrialTest> field_trial_test_;
+  base::SimpleTestClock* clock_;
+  base::SimpleTestTickClock* tick_clock_;
+  TestingPrefServiceSimple pref_service_;
+  std::unique_ptr<network_time::NetworkTimeTracker> tracker_;
+  std::unique_ptr<net::EmbeddedTestServer> test_server_;
+
+  DISALLOW_COPY_AND_ASSIGN(DateInvalidSSLErrorHandlerTest);
 };
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 
-TEST_F(SSLErrorHandlerTest,
-       ShouldShowSSLInterstitialOnTimerExpired) {
+TEST_F(CommonNameSSLErrorHandlerTest, ShouldShowSSLInterstitialOnTimerExpired) {
   EXPECT_FALSE(error_handler()->IsTimerRunning());
   error_handler()->StartHandlingError();
 
@@ -185,7 +277,7 @@ TEST_F(SSLErrorHandlerTest,
   EXPECT_FALSE(error_handler()->captive_portal_interstitial_shown());
 }
 
-TEST_F(SSLErrorHandlerTest,
+TEST_F(CommonNameSSLErrorHandlerTest,
        ShouldShowCustomInterstitialOnCaptivePortalResult) {
   EXPECT_FALSE(error_handler()->IsTimerRunning());
   error_handler()->StartHandlingError();
@@ -206,7 +298,7 @@ TEST_F(SSLErrorHandlerTest,
   EXPECT_TRUE(error_handler()->captive_portal_interstitial_shown());
 }
 
-TEST_F(SSLErrorHandlerTest,
+TEST_F(CommonNameSSLErrorHandlerTest,
        ShouldShowSSLInterstitialOnNoCaptivePortalResult) {
   EXPECT_FALSE(error_handler()->IsTimerRunning());
   error_handler()->StartHandlingError();
@@ -229,7 +321,8 @@ TEST_F(SSLErrorHandlerTest,
   EXPECT_FALSE(error_handler()->captive_portal_interstitial_shown());
 }
 
-TEST_F(SSLErrorHandlerTest, ShouldNotCheckSuggestedUrlIfNoSuggestedUrl) {
+TEST_F(CommonNameSSLErrorHandlerTest,
+       ShouldNotCheckSuggestedUrlIfNoSuggestedUrl) {
   error_handler()->StartHandlingError();
 
   EXPECT_TRUE(error_handler()->captive_portal_checked());
@@ -241,7 +334,8 @@ TEST_F(SSLErrorHandlerTest, ShouldNotCheckSuggestedUrlIfNoSuggestedUrl) {
   EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
 }
 
-TEST_F(SSLErrorHandlerTest, ShouldNotCheckCaptivePortalIfSuggestedUrlExists) {
+TEST_F(CommonNameSSLErrorHandlerTest,
+       ShouldNotCheckCaptivePortalIfSuggestedUrlExists) {
   EXPECT_FALSE(error_handler()->IsTimerRunning());
   error_handler()->set_suggested_url_exists();
   error_handler()->StartHandlingError();
@@ -255,7 +349,8 @@ TEST_F(SSLErrorHandlerTest, ShouldNotCheckCaptivePortalIfSuggestedUrlExists) {
   EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
 }
 
-TEST_F(SSLErrorHandlerTest, ShouldNotHandleNameMismatchOnNonOverridableError) {
+TEST_F(CommonNameSSLErrorHandlerTest,
+       ShouldNotHandleNameMismatchOnNonOverridableError) {
   error_handler()->set_non_overridable_error();
   error_handler()->set_suggested_url_exists();
   error_handler()->StartHandlingError();
@@ -271,7 +366,7 @@ TEST_F(SSLErrorHandlerTest, ShouldNotHandleNameMismatchOnNonOverridableError) {
 
 #else  // #if !BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 
-TEST_F(SSLErrorHandlerTest,
+TEST_F(CommonNameSSLErrorHandlerTest,
        ShouldShowSSLInterstitialOnCaptivePortalDetectionDisabled) {
   EXPECT_FALSE(error_handler()->IsTimerRunning());
   error_handler()->StartHandlingError();
@@ -283,7 +378,7 @@ TEST_F(SSLErrorHandlerTest,
 
 #endif  // BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 
-TEST_F(SSLErrorHandlerTest,
+TEST_F(CommonNameSSLErrorHandlerTest,
        ShouldShowSSLInterstitialOnTimerExpiredWhenSuggestedUrlExists) {
   error_handler()->set_suggested_url_exists();
   error_handler()->StartHandlingError();
@@ -300,7 +395,7 @@ TEST_F(SSLErrorHandlerTest,
   EXPECT_FALSE(error_handler()->redirected_to_suggested_url());
 }
 
-TEST_F(SSLErrorHandlerTest, ShouldRedirectOnSuggestedUrlCheckResult) {
+TEST_F(CommonNameSSLErrorHandlerTest, ShouldRedirectOnSuggestedUrlCheckResult) {
   error_handler()->set_suggested_url_exists();
   error_handler()->StartHandlingError();
 
@@ -321,7 +416,8 @@ TEST_F(SSLErrorHandlerTest, ShouldRedirectOnSuggestedUrlCheckResult) {
   EXPECT_TRUE(error_handler()->redirected_to_suggested_url());
 }
 
-TEST_F(SSLErrorHandlerTest, ShouldShowSSLInterstitialOnInvalidUrlCheckResult) {
+TEST_F(CommonNameSSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnInvalidUrlCheckResult) {
   error_handler()->set_suggested_url_exists();
   error_handler()->StartHandlingError();
 
@@ -339,3 +435,90 @@ TEST_F(SSLErrorHandlerTest, ShouldShowSSLInterstitialOnInvalidUrlCheckResult) {
   EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
   EXPECT_FALSE(error_handler()->redirected_to_suggested_url());
 }
+
+TEST_F(DateInvalidSSLErrorHandlerTest, TimeQueryStarted) {
+  base::Time network_time;
+  base::TimeDelta uncertainty;
+  EXPECT_EQ(network_time::NetworkTimeTracker::NETWORK_TIME_NO_SYNC_ATTEMPT,
+            tracker()->GetNetworkTime(&network_time, &uncertainty));
+
+  // Enable network time queries and handle the error. A bad clock interstitial
+  // should be shown.
+  EXPECT_TRUE(test_server()->Start());
+  test_server()->RegisterRequestHandler(
+      base::Bind(&network_time::GoodTimeResponseHandler));
+  tracker()->SetTimeServerURLForTesting(test_server()->GetURL("/"));
+  field_trial_test()->SetNetworkQueriesWithVariationsService(
+      true, 0.0, network_time::FieldTrialTest::ENABLE_FETCHES_ON_DEMAND);
+  error_handler()->StartHandlingError();
+
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  tracker()->WaitForFetchForTesting(123123123);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(error_handler()->bad_clock_interstitial_shown());
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+}
+
+// Tests that an SSL interstitial is shown if the accuracy of the system
+// clock can't be determined because network time is unavailable.
+TEST_F(DateInvalidSSLErrorHandlerTest, NoTimeQueries) {
+  base::Time network_time;
+  base::TimeDelta uncertainty;
+  EXPECT_EQ(network_time::NetworkTimeTracker::NETWORK_TIME_NO_SYNC_ATTEMPT,
+            tracker()->GetNetworkTime(&network_time, &uncertainty));
+
+  // Handle the error without enabling time queries. A bad clock interstitial
+  // should not be shown.

[meacer, 2016/10/26 20:03:03]

Just to confirm: This is assuming we don't have the build time heuristics
enabled, right? Otherwise, we'd still show the bad clock ui?

[estark, 2016/10/31 16:03:25]

Sorta. It's assuming that the build time heuristic doesn't fire because the
build timestamp matches the system clock on which the tests are running. (That
is, in tests we assume that the test build is fresh relative to the clock of the
system on which the tests are running.)

+  error_handler()->StartHandlingError();
+
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  EXPECT_FALSE(error_handler()->bad_clock_interstitial_shown());
+  EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
+}
+
+// Runs |quit_closure| on the UI thread once a URL request has been
+// seen. Returns a request that hangs.
+std::unique_ptr<net::test_server::HttpResponse> WaitForRequest(
+    const base::Closure& quit_closure,
+    const net::test_server::HttpRequest& request) {
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                   quit_closure);
+  return base::MakeUnique<net::test_server::HungResponse>();
+}
+
+// Tests that an SSL interstitial is shown if determing the accuracy of
+// the system clock times out (e.g. because a network time query hangs).
+TEST_F(DateInvalidSSLErrorHandlerTest, TimeQueryHangs) {
+  base::Time network_time;
+  base::TimeDelta uncertainty;
+  EXPECT_EQ(network_time::NetworkTimeTracker::NETWORK_TIME_NO_SYNC_ATTEMPT,
+            tracker()->GetNetworkTime(&network_time, &uncertainty));
+
+  // Enable network time queries and handle the error. Because the
+  // network time cannot be determined before the timer elapses, an SSL
+  // interstitial should be shown.
+  EXPECT_TRUE(test_server()->Start());
+  base::RunLoop wait_for_time_query_loop;
+  test_server()->RegisterRequestHandler(
+      base::Bind(&WaitForRequest, wait_for_time_query_loop.QuitClosure()));
+  tracker()->SetTimeServerURLForTesting(test_server()->GetURL("/"));
+  field_trial_test()->SetNetworkQueriesWithVariationsService(
+      true, 0.0, network_time::FieldTrialTest::ENABLE_FETCHES_ON_DEMAND);
+  error_handler()->StartHandlingError();
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  wait_for_time_query_loop.Run();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(error_handler()->bad_clock_interstitial_shown());
+  EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+
+  // Clear the error handler to test that, when the request completes,
+  // it doesn't try to call a callback on a deleted SSLErrorHandler.
+  ClearErrorHandler();

[meacer, 2016/10/26 20:03:02]

Is the idea here that the test would crash with a null deref if it failed?

[estark, 2016/10/31 16:03:25]

Yep

+
+  // Shut down the server to cancel the pending request.
+  ASSERT_TRUE(test_server()->ShutdownAndWaitUntilComplete());
+  tracker()->WaitForFetchForTesting(123123123);
+}