Add custom interstitial for captive portals.

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
new file mode 100644
index 0000000000000000000000000000000000000000..0e0b4dbca789525914dc31514bae5241b75ddfc4
--- /dev/null
+++ b/chrome/browser/ssl/ssl_error_handler_unittest.cc
@@ -0,0 +1,203 @@
+// Copyright 2014 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/ssl/ssl_error_handler.h"
+
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "chrome/browser/captive_portal/captive_portal_service.h"
+#include "chrome/browser/profiles/profile.h"
+#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 "content/public/browser/notification_service.h"
+#include "content/public/common/content_switches.h"
+#include "net/base/net_errors.h"
+#include "net/ssl/ssl_info.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class TestSSLErrorHandler : public SSLErrorHandler {
+ public:
+  TestSSLErrorHandler(Profile* profile,
+                      content::WebContents* web_contents,
+                      const net::SSLInfo& ssl_info)
+      : SSLErrorHandler(web_contents,
+                        net::ERR_CERT_COMMON_NAME_INVALID,
+                        ssl_info,
+                        GURL(),
+                        0,
+                        base::TimeDelta(),
+                        base::Callback<void(bool)>()),
+        profile_(profile),
+        captive_portal_checked_(false),
+        ssl_interstitial_shown_(false),
+        captive_portal_interstitial_shown_(false) {
+  }
+
+  ~TestSSLErrorHandler() override {
+  }
+
+  void Handle() {
+    SSLErrorHandler::Handle();
+  }
+
+  void SendCaptivePortalNotification(
+      captive_portal::CaptivePortalResult result) {
+    CaptivePortalService::Results results;
+    results.previous_result = captive_portal::RESULT_INTERNET_CONNECTED;
+    results.result = result;
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
+        content::Source<Profile>(profile_),
+        content::Details<CaptivePortalService::Results>(&results));
+  }
+
+  bool IsTimerRunning() const {
+    return timer_.IsRunning();
+  }
+
+  int captive_portal_checked() const {
+    return captive_portal_checked_;
+  }
+
+  int ssl_interstitial_shown() const {
+    return ssl_interstitial_shown_;
+  }
+
+  int captive_portal_interstitial_shown() const {
+    return captive_portal_interstitial_shown_;
+  }
+
+  void Reset() {
+    captive_portal_checked_ = false;
+    ssl_interstitial_shown_ = false;
+    captive_portal_interstitial_shown_ = false;
+  }
+
+ private:
+  void CheckForCaptivePortal() override {
+    captive_portal_checked_ = true;
+  }
+
+  void ShowSSLInterstitial() override {
+    ssl_interstitial_shown_ = true;
+  }
+
+  void ShowCaptivePortalInterstitial() override {
+    captive_portal_interstitial_shown_ = true;
+  }
+
+  Profile* profile_;
+  bool captive_portal_checked_;
+  bool ssl_interstitial_shown_;
+  bool captive_portal_interstitial_shown_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandler);
+};
+
+class SSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {
+ public:
+  void SetUp() override {
+    CommandLine& command_line = *CommandLine::ForCurrentProcess();
+    command_line.AppendSwitch(::switches::kTestType);

[mmenke, 2014/11/12 22:15:08]

What does this do?

[meacer, 2014/11/25 01:39:52]

Good catch, it's an artifact from previous patches to check if ssl error handler
should disable portal checks etc. Removed.

+    ChromeRenderViewHostTestHarness::SetUp();
+    ssl_info_.reset(new net::SSLInfo);
+    error_handler_.reset(new TestSSLErrorHandler(profile(),
+                                                 web_contents(),
+                                                 *ssl_info_.get()));
+  }
+
+  void TearDown() override {
+    EXPECT_FALSE(error_handler().IsTimerRunning());
+    error_handler_.reset(NULL);
+    ssl_info_.reset(NULL);
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  TestSSLErrorHandler& error_handler() { return *error_handler_.get(); }

[mmenke, 2014/11/12 22:15:08]

Should either make this const or make it return a pointer.

[meacer, 2014/11/25 01:39:52]

Done.

+
+ private:
+  scoped_ptr<net::SSLInfo> ssl_info_;

[mmenke, 2014/11/12 22:15:08]

Any reason this has to be a scoped_ptr?  Seems like we could just stuff it on
the stack and be fine.

[meacer, 2014/11/25 01:39:52]

Done.

+  scoped_ptr<TestSSLErrorHandler> error_handler_;
+};
+
+#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnTimerExpired) {
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  error_handler().Handle();
+
+  EXPECT_TRUE(error_handler().IsTimerRunning());
+  EXPECT_TRUE(error_handler().captive_portal_checked());
+  EXPECT_FALSE(error_handler().ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler().captive_portal_interstitial_shown());
+
+  error_handler().Reset();

[mmenke, 2014/11/12 22:15:08]

optional:  Could use counts instead of bools, and get rid of Reset - it's a bit
more robust, as it also catches when things happen more than once before/after
your reset calls, and I think it makes the tests a little clearer.

[meacer, 2014/11/14 00:30:25]

I used counts before, but then switched to bool because the counters only went
up to 1. I find bools easier to read than 1s and 0s in the counts.

+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  EXPECT_FALSE(error_handler().captive_portal_checked());
+  EXPECT_TRUE(error_handler().ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler().captive_portal_interstitial_shown());
+}
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowCustomInterstitialOnCaptivePortalResult) {
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  error_handler().Handle();
+
+  EXPECT_TRUE(error_handler().IsTimerRunning());
+  EXPECT_TRUE(error_handler().captive_portal_checked());
+  EXPECT_FALSE(error_handler().ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler().captive_portal_interstitial_shown());
+  // Fake a captive portal result.
+  error_handler().Reset();
+  error_handler().SendCaptivePortalNotification(
+      captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL);

[mmenke, 2014/11/12 22:15:08]

These aren't really checking that the error handler is unhooking itself
correctly (Because in these tests, it's not).

Maybe a browser test where we make sure the interstitial isn't re-created on new
captive portal checks?  I suspect the captive portal tests would fail if that
happened, anyways, but I'd be happier with tests explicitly checking for that,
in absence of the whole login tab stuff.

Could additionally/alternatively check that here, by having a shared object keep
track of the counts, and calling into the real handler after updating the
counters.  I'm sure what trying to show an interstitial in these tests would do,
though...And we rather depend on browser tests for this, anyways.

[meacer, 2014/11/14 00:30:25]

Will try to add a browser test for this.

+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  EXPECT_FALSE(error_handler().captive_portal_checked());
+  EXPECT_FALSE(error_handler().ssl_interstitial_shown());
+  EXPECT_TRUE(error_handler().captive_portal_interstitial_shown());
+}
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnNoCaptivePortalResult) {
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  error_handler().Handle();
+
+  EXPECT_TRUE(error_handler().IsTimerRunning());
+  EXPECT_TRUE(error_handler().captive_portal_checked());
+  EXPECT_FALSE(error_handler().ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler().captive_portal_interstitial_shown());
+  // Fake a "connected to internet" result for the captive portal check.
+  // This should immediately trigger an SSL interstitial without waiting for
+  // the timer to expire.
+  error_handler().Reset();
+  error_handler().SendCaptivePortalNotification(
+      captive_portal::RESULT_INTERNET_CONNECTED);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  EXPECT_FALSE(error_handler().captive_portal_checked());
+  EXPECT_TRUE(error_handler().ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler().captive_portal_interstitial_shown());
+}
+
+#else  // #if !defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnCaptivePortalDetectionDisabled) {
+  EXPECT_FALSE(error_handler().IsTimerRunning());
+  error_handler().Handle();

[mmenke, 2014/11/12 22:15:08]

EXPECT_FALSE(error_handler().IsTimerRunning());

[meacer, 2014/11/25 01:39:52]

Done.

+  EXPECT_FALSE(error_handler().captive_portal_checked());
+  EXPECT_TRUE(error_handler().ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler().captive_portal_interstitial_shown());
+}
+
+#endif  // defined(ENABLE_CAPTIVE_PORTAL_DETECTION)