Common Name Mismatch Handler For WWW Subdomain Mismatch case

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 e93908e9cf7847c2c9e4af7c9bdf3641b9bd0249..3532ff914115cd8bc8c40f1438264ebdbde3dbda 100644
--- a/chrome/browser/ssl/ssl_error_handler_unittest.cc
+++ b/chrome/browser/ssl/ssl_error_handler_unittest.cc
@@ -11,12 +11,15 @@
 #include "base/time/time.h"
 #include "chrome/browser/captive_portal/captive_portal_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ssl/common_name_mismatch_handler.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 "net/base/net_errors.h"
+#include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_info.h"
+#include "net/test/test_certificate_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class TestSSLErrorHandler : public SSLErrorHandler {
@@ -33,8 +36,11 @@ class TestSSLErrorHandler : public SSLErrorHandler {
                         base::Callback<void(bool)>()),
         profile_(profile),
         captive_portal_checked_(false),
+        suggested_url_exists_(false),
+        suggested_url_checked_(false),
         ssl_interstitial_shown_(false),
-        captive_portal_interstitial_shown_(false) {}
+        captive_portal_interstitial_shown_(false),
+        common_name_mismatch_interstitial_shown_(false) {}
 
   ~TestSSLErrorHandler() override {
   }
@@ -52,6 +58,15 @@ class TestSSLErrorHandler : public SSLErrorHandler {
         content::Details<CaptivePortalService::Results>(&results));
   }
 
+  void SendSuggestedUrlCheckResult(
+      const CommonNameMismatchHandler::SuggestedUrlCheckResult& result,
+      const GURL new_url) {

[meacer, 2015/07/15 20:11:46]

const GURL&

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+    CommonNameMismatchHandler::Results results;
+    results.result = result;
+    results.new_url = new_url;
+    CommonNameMismatchHandlerCallback(results);
+  }
+
   bool IsTimerRunning() const {
     return get_timer().IsRunning();
   }
@@ -68,10 +83,30 @@ class TestSSLErrorHandler : public SSLErrorHandler {
     return captive_portal_interstitial_shown_;
   }
 
+  void SetSuggestedUrl() { suggested_url_exists_ = true; }

[meacer, 2015/07/15 20:11:47]

I suggest changing this to SetSuggestedUrlExists(bool). See my second comment in
ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse.

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+
+  bool GetSuggestedUrl(const GURL& request_url,
+                       const std::vector<std::string>& dns_names,
+                       GURL* suggested_url) override {
+    if (!suggested_url_exists_)
+      return false;
+    *suggested_url = GURL("www.example.com");
+    return true;
+  }
+
+  int suggested_url_checked() const { return suggested_url_checked_; }

[meacer, 2015/07/15 20:11:46]

bool return type

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+
+  int common_name_mismatch_interstitial_shown() const {

[meacer, 2015/07/15 20:11:46]

bool return type

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+    return common_name_mismatch_interstitial_shown_;
+  }
+
   void Reset() {
     captive_portal_checked_ = false;
+    suggested_url_exists_ = false;
+    suggested_url_checked_ = false;
     ssl_interstitial_shown_ = false;
     captive_portal_interstitial_shown_ = false;
+    common_name_mismatch_interstitial_shown_ = false;
   }
 
  private:
@@ -79,18 +114,28 @@ class TestSSLErrorHandler : public SSLErrorHandler {
     captive_portal_checked_ = true;
   }
 
-  void ShowSSLInterstitial() override {
-    ssl_interstitial_shown_ = true;
+  void ShowSSLInterstitial(const GURL& suggested_url) override {
+    if (!suggested_url.is_empty())
+      common_name_mismatch_interstitial_shown_ = true;
+    else
+      ssl_interstitial_shown_ = true;
   }
 
   void ShowCaptivePortalInterstitial(const GURL& landing_url) override {
     captive_portal_interstitial_shown_ = true;
   }
 
+  void CheckSuggestedUrl(const GURL& suggested_url) override {
+    suggested_url_checked_ = true;
+  }
+
   Profile* profile_;
   bool captive_portal_checked_;
+  bool suggested_url_exists_;
+  bool suggested_url_checked_;

[meacer, 2015/07/15 20:11:46]

nit: Put these in opposite order here in other places, so that it's a bit
clearer which one is supposed to be true first:

bool suggested_url_checked_;
bool suggested_url_exists_;

[Bhanu Dev, 2015/07/16 23:38:06]

If the suggested URL exists, we check for the validity of it. 
so, the order is correct here.

   bool ssl_interstitial_shown_;
   bool captive_portal_interstitial_shown_;
+  bool common_name_mismatch_interstitial_shown_;
 
   DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandler);
 };
@@ -104,13 +149,15 @@ class SSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     SSLErrorHandler::SetInterstitialDelayTypeForTest(SSLErrorHandler::NONE);
+    ssl_info_.cert = net::X509Certificate::CreateFromBytes(
+        reinterpret_cast<const char*>(google_der), sizeof(google_der));

[meacer, 2015/07/15 20:11:46]

Do you need an actual cert here? If not, you can create an empty one:

ssl_info_.cert = new net::X509Certificate(
          "bad.host", "CA", base::Time::Max(), base::Time::Max());

[Bhanu Dev, 2015/07/16 23:38:06]

|SSLErrorHandler| calls method like |GetDNSNames| on the |X509Certificate| cert.
class. These methods need an actual certificate and are giving a Segmentation
Fault when I use an empty one.   

     error_handler_.reset(new TestSSLErrorHandler(profile(),
                                                  web_contents(),
                                                  ssl_info_));
     // Enable finch experiment for captive portal interstitials.
     ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
                     "CaptivePortalInterstitial", "Enabled"));
-}
+  }
 
   void TearDown() override {
     EXPECT_FALSE(error_handler()->IsTimerRunning());
@@ -191,6 +238,32 @@ TEST_F(SSLErrorHandlerTest,
   EXPECT_FALSE(error_handler()->captive_portal_interstitial_shown());
 }
 
+TEST_F(SSLErrorHandlerTest,
+       ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse) {

[meacer, 2015/07/15 20:11:46]

nit: name this ShouldNotCheckSuggestedUrlIfNoSuggestedUrl?

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  error_handler()->StartHandlingError();

[meacer, 2015/07/15 20:11:46]

Looks like you are using |SetSuggestedURL| to enable suggested url. How about
adding a bool param to |SetSuggestedURL| and set it to false here so that it's
clearer for the reader what to expect? 

[Bhanu Dev, 2015/07/16 23:38:06]

It is good to add a bool param to |SetSuggestedURL|. Done. 

+
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  EXPECT_FALSE(error_handler()->suggested_url_checked());
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_TRUE(error_handler()->ssl_interstitial_shown());

[meacer, 2015/07/15 20:11:46]

 EXPECT_FALSE(error_handler()->IsTimerRunning()); before this.

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+}
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldNotCheckForCaptivePortalIfSuggestedUrlExists) {

[meacer, 2015/07/15 20:11:46]

nit: This name uses CheckFor while the previous one uses Check. Choose one and
be consistent.

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  error_handler()->SetSuggestedUrl();
+  error_handler()->StartHandlingError();
+
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  EXPECT_TRUE(error_handler()->suggested_url_checked());
+  EXPECT_FALSE(error_handler()->captive_portal_checked());
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_TRUE(error_handler()->ssl_interstitial_shown());

[meacer, 2015/07/15 20:11:47]

EXPECT_FALSE(error_handler()->IsTimerRunning()); before this line

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+}
+
 #else  // #if !defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
 
 TEST_F(SSLErrorHandlerTest,
@@ -204,3 +277,62 @@ TEST_F(SSLErrorHandlerTest,
 }
 
 #endif  // defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnTimerExpiredWhenSuggestedUrlExists) {
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  error_handler()->SetSuggestedUrl();
+  error_handler()->StartHandlingError();
+
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  EXPECT_TRUE(error_handler()->suggested_url_checked());
+  EXPECT_FALSE(error_handler()->ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler()->common_name_mismatch_interstitial_shown());
+
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler()->common_name_mismatch_interstitial_shown());
+}
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowCommonNameMismatchInterstitialOnSuggestedUrlCheckResult) {
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  error_handler()->SetSuggestedUrl();
+  error_handler()->StartHandlingError();
+
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  EXPECT_TRUE(error_handler()->suggested_url_checked());
+  EXPECT_FALSE(error_handler()->ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler()->common_name_mismatch_interstitial_shown());
+  // Fake a Valid Suggested URL Check result.

[meacer, 2015/07/15 20:11:46]

nit: don't capitalize "Valid Suggested URL Check"

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+  error_handler()->SendSuggestedUrlCheckResult(
+      CommonNameMismatchHandler::SuggestedUrlCheckResult::
+          RESULT_SUGGESTED_URL_VALID,
+      GURL("https://random.example.com"));

[meacer, 2015/07/15 20:11:46]

Why is this URL different than the return value of GetSuggestedUrl?
(www.example.com)

[meacer, 2015/07/15 20:11:46]

indent 4 more spaces

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

[Bhanu Dev, 2015/07/16 23:38:06]

This URL is the new landing page and this can be different from the return value
of |GetSuggestedUrl|, if there is a redirect. So, I changed it.

I added a comment, let me know if you think its good to have the same URL here.
   

[Bhanu Dev, 2015/07/16 23:38:06]

I tried to indent it, but |git cl format| is reverting the change. Here,
|CommonNameMismatchHandler::SuggestedUrlCheckResult::RESULT_SUGGESTED_URL_VALID|
is a single parameter and |GURL("https://random.example.com")| is the other, so
its making them both aligned.

+
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  EXPECT_FALSE(error_handler()->ssl_interstitial_shown());
+  EXPECT_TRUE(error_handler()->common_name_mismatch_interstitial_shown());
+}
+
+TEST_F(SSLErrorHandlerTest, ShouldShowSSLInterstitialOnInvalidUrlCheckResult) {
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  error_handler()->SetSuggestedUrl();
+  error_handler()->StartHandlingError();
+
+  EXPECT_TRUE(error_handler()->IsTimerRunning());
+  EXPECT_TRUE(error_handler()->suggested_url_checked());
+  EXPECT_FALSE(error_handler()->ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler()->common_name_mismatch_interstitial_shown());
+  // Fake an Invalid Suggested URL Check result.
+  error_handler()->SendSuggestedUrlCheckResult(
+      CommonNameMismatchHandler::SuggestedUrlCheckResult::
+          RESULT_SUGGESTED_URL_INVALID,
+      GURL());

[meacer, 2015/07/15 20:11:47]

indent 4 more spaces

[Bhanu Dev, 2015/07/16 23:38:06]

Done.

+
+  EXPECT_FALSE(error_handler()->IsTimerRunning());
+  EXPECT_TRUE(error_handler()->ssl_interstitial_shown());
+  EXPECT_FALSE(error_handler()->common_name_mismatch_interstitial_shown());
+}