HTTP Bad: Split out UMA metrics for password vs credit card "Not secure" warnings

chrome/browser/ssl/chrome_security_state_model_client_unittest.cc

 // Copyright 2016 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/chrome_security_state_model_client.h"
 
 #include "base/command_line.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/security_state/security_state_model.h"
 #include "components/security_state/switches.h"
 #include "content/public/browser/security_style_explanation.h"
 #include "content/public/browser/security_style_explanations.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
-const char kHTTPBadHistogram[] =
-    "Security.HTTPBad.UserWarnedAboutSensitiveInput";
+const char kHTTPBadHistogramPassword[] =
+    "Security.HTTPBad.UserWarnedAboutSensitiveInput.Password";
 
 // Tests that SecurityInfo flags for subresources with certificate
 // errors are reflected in the SecurityStyleExplanations produced by
 // ChromeSecurityStateModelClient.
 TEST(ChromeSecurityStateModelClientTest,
      GetSecurityStyleForContentWithCertErrors) {
   content::SecurityStyleExplanations explanations;
   security_state::SecurityStateModel::SecurityInfo security_info;
   security_info.cert_status = 0;
   security_info.scheme_is_cryptographic = true;
@@ skipping 196 matching lines @@
       security_state::SecurityStateModel::HTTP_SHOW_WARNING;
   blink::WebSecurityStyle security_style =
       ChromeSecurityStateModelClient::GetSecurityStyle(security_info,
                                                        &explanations);
   EXPECT_EQ(blink::WebSecurityStyleUnauthenticated, security_style);
   EXPECT_EQ(1u, explanations.unauthenticated_explanations.size());
 }
 
 // Tests that a security level of NONE when there is a password or
 // credit card field on HTTP produces a content::SecurityStyle of
-// UNAUTHENTICATED, with an info explanation.
+// UNAUTHENTICATED, with an info explanation for each.
 TEST(ChromeSecurityStateModelClientTest, HTTPWarningInFuture) {
   security_state::SecurityStateModel::SecurityInfo security_info;
   content::SecurityStyleExplanations explanations;
   security_info.security_level = security_state::SecurityStateModel::NONE;
-  security_info.displayed_private_user_data_input_on_http = true;
+  security_info.displayed_password_field_on_http = true;
   blink::WebSecurityStyle security_style =
       ChromeSecurityStateModelClient::GetSecurityStyle(security_info,
                                                        &explanations);
   EXPECT_EQ(blink::WebSecurityStyleUnauthenticated, security_style);
   EXPECT_EQ(1u, explanations.info_explanations.size());
+
+  security_info.displayed_credit_card_field_on_http = true;

[estark, 2016/11/09 15:56:46]

Maybe you could call explanations.info_explanations.clear() here. (It's a little
confusing to see 2u on line 255 because normally |explanations| is empty when
GetSecurityStyle() is called.)

[lshang, 2016/11/10 04:53:54]

Done.

+  security_style = ChromeSecurityStateModelClient::GetSecurityStyle(
+      security_info, &explanations);
+  EXPECT_EQ(blink::WebSecurityStyleUnauthenticated, security_style);
+  EXPECT_EQ(2u, explanations.info_explanations.size());

[estark, 2016/11/09 15:56:46]

Could you please add another section that one info explanation is added if
*both* the password and credit card fields are true? (I'd like to have a test
that we don't add two explanations if both fields are set.)

[lshang, 2016/11/10 04:53:55]

Done.

 }
 
 class ChromeSecurityStateModelClientHistogramTest
     : public ChromeRenderViewHostTestHarness {
  public:
   ChromeSecurityStateModelClientHistogramTest() {}
   ~ChromeSecurityStateModelClientHistogramTest() override {}
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
@@ skipping 22 matching lines @@
 // HTTP_SHOW_WARNING.
 TEST_F(ChromeSecurityStateModelClientHistogramTest,
        HTTPOmniboxWarningHistogram) {
   // Show Warning Chip.
   base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       security_state::switches::kMarkHttpAs,
       security_state::switches::kMarkHttpWithPasswordsOrCcWithChip);
 
   base::HistogramTester histograms;
   signal_password();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, true, 1);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, true, 1);
 
   // Fire again and ensure no sample is recorded.
   signal_password();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, true, 1);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, true, 1);
 
   // Navigate to a new page and ensure a sample is recorded.
   navigate_to_http();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, true, 1);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, true, 1);
   signal_password();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, true, 2);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, true, 2);
 }
 
 // Tests that UMA logs the console warning when security level is NONE.
 TEST_F(ChromeSecurityStateModelClientHistogramTest,
        HTTPConsoleWarningHistogram) {
   // Show Neutral for HTTP
   base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       security_state::switches::kMarkHttpAs,
       security_state::switches::kMarkHttpAsNeutral);
 
   base::HistogramTester histograms;
   signal_password();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, false, 1);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, false, 1);
 
   // Fire again and ensure no sample is recorded.
   signal_password();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, false, 1);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, false, 1);
 
   // Navigate to a new page and ensure a sample is recorded.
   navigate_to_http();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, false, 1);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, false, 1);
   signal_password();
-  histograms.ExpectUniqueSample(kHTTPBadHistogram, false, 2);
+  histograms.ExpectUniqueSample(kHTTPBadHistogramPassword, false, 2);
 }
 

[estark, 2016/11/09 15:56:46]

I think you can also use INSTANTIATE_TEST_CASE_P for unit tests. Could you use
that here so that each of these tests runs for both the password histogram and
the credit card histogram?

[lshang, 2016/11/10 04:53:55]

Done.

 }  // namespace