Refactor PageInfo EV handling

This refactors how Page Info handles showing the organization details
for sites with EV certificates by moving creation of the subtitle text
into the PageInfoBubbleView directly, since this is only used there.
This fixes an issue where we were over-using the site_details_details_
member to track both EV details and Safe Browsing details, causing the
certificate button subtitle to get filled with the wrong text for sites
that have an EV certificate *and* trigger SB warnings.

As part of this, tracking the organization name in PageInfo is no
longer necessary, so this also removes that entirely.

This change also includes two new tests: a unit test for checking that
the certificate button subtitle text is correctly set for sites with
EV certificates, and a regression browser test for sites that have an
EV cert and trigger SB warnings.

Bug: 1014240
Change-Id: I414cb07097fb5781c4b3a22ac7cca6e52bf14365
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1863643Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706579}

chrome/browser/ui/page_info/page_info.cc

@@ -686,17 +686,6 @@ void PageInfo::ComputeUIInputs(
       if (visible_security_state.cert_status & net::CERT_STATUS_IS_EV) {
         // EV HTTPS page.
         site_identity_status_ = SITE_IDENTITY_STATUS_EV_CERT;
-        DCHECK(!certificate_->subject().organization_names.empty());
-        organization_name_ =
-            UTF8ToUTF16(certificate_->subject().organization_names[0]);
-        // An EV Cert is required to have a city (localityName) and country but
-        // state is "if any".
-        DCHECK(!certificate_->subject().locality_name.empty());
-        DCHECK(!certificate_->subject().country_name.empty());
-        site_details_message_.assign(l10n_util::GetStringFUTF16(
-            IDS_PAGE_INFO_SECURITY_TAB_SECURE_IDENTITY_EV_VERIFIED,
-            organization_name_,
-            UTF8ToUTF16(certificate_->subject().country_name)));
       } else {
         // Non-EV OK HTTPS page.
         site_identity_status_ = SITE_IDENTITY_STATUS_CERT;
@@ -989,10 +978,7 @@ void PageInfo::PresentSiteIdentity() {
   DCHECK_NE(site_identity_status_, SITE_IDENTITY_STATUS_UNKNOWN);
   DCHECK_NE(site_connection_status_, SITE_CONNECTION_STATUS_UNKNOWN);
   PageInfoUI::IdentityInfo info;
-  if (site_identity_status_ == SITE_IDENTITY_STATUS_EV_CERT)
-    info.site_identity = UTF16ToUTF8(organization_name());
-  else
-    info.site_identity = UTF16ToUTF8(GetSimpleSiteName(site_url_));
+  info.site_identity = UTF16ToUTF8(GetSimpleSiteName(site_url_));
 
   info.connection_status = site_connection_status_;
   info.connection_status_description = UTF16ToUTF8(site_connection_details_);

chrome/browser/ui/page_info/page_info.h

@@ -208,8 +208,6 @@ class PageInfo : public content::WebContentsObserver {
     return site_details_message_;
   }
 
-  const base::string16& organization_name() const { return organization_name_; }
-
  private:
   FRIEND_TEST_ALL_PREFIXES(PageInfoTest,
                            NonFactoryDefaultAndRecentlyChangedPermissionsShown);

chrome/browser/ui/page_info/page_info_unittest.cc

@@ -510,7 +510,6 @@ TEST_F(PageInfoTest, HTTPConnection) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_NO_CERT,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 }
 
 TEST_F(PageInfoTest, HTTPSConnection) {
@@ -530,7 +529,6 @@ TEST_F(PageInfoTest, HTTPSConnection) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_CERT,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 }
 
 // Define some dummy constants for Android-only resources.
@@ -745,7 +743,6 @@ TEST_F(PageInfoTest, InsecureContent) {
         test.expected_connection_icon_id,
         PageInfoUI::GetConnectionIconID(page_info()->site_connection_status()));
 #endif
-    EXPECT_EQ(base::string16(), page_info()->organization_name());
   }
 }
 
@@ -772,9 +769,6 @@ TEST_F(PageInfoTest, HTTPSEVCert) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_EV_CERT,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::UTF8ToUTF16("Google Inc"), page_info()->organization_name());
-  EXPECT_EQ(base::UTF8ToUTF16("Issued to: Google Inc [US]"),
-            page_info()->site_details_message());
 }
 
 TEST_F(PageInfoTest, HTTPSConnectionError) {
@@ -796,7 +790,6 @@ TEST_F(PageInfoTest, HTTPSConnectionError) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_CERT,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 }
 
 #if defined(OS_CHROMEOS)
@@ -817,7 +810,6 @@ TEST_F(PageInfoTest, HTTPSPolicyCertConnection) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_ADMIN_PROVIDED_CERT,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 }
 #endif
 
@@ -839,7 +831,6 @@ TEST_F(PageInfoTest, HTTPSSHA1) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_DEPRECATED_SIGNATURE_ALGORITHM,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 #if defined(OS_ANDROID)
   EXPECT_EQ(IDR_PAGEINFO_WARNING_MINOR,
             PageInfoUI::GetIdentityIconID(page_info()->site_identity_status()));
@@ -958,7 +949,6 @@ TEST_F(PageInfoTest, AboutBlankPage) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_NO_CERT,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 }
 
 // On desktop, internal URLs aren't handled by PageInfo class. Instead, a
@@ -971,7 +961,6 @@ TEST_F(PageInfoTest, InternalPage) {
             page_info()->site_connection_status());
   EXPECT_EQ(PageInfo::SITE_IDENTITY_STATUS_INTERNAL_PAGE,
             page_info()->site_identity_status());
-  EXPECT_EQ(base::string16(), page_info()->organization_name());
 }
 #endif
 

chrome/browser/ui/views/page_info/page_info_bubble_view.cc

@@ -787,8 +787,16 @@ void PageInfoBubbleView::SetIdentityInfo(const IdentityInfo& identity_info) {
               PageInfo::SITE_IDENTITY_STATUS_EV_CERT &&
           identity_info.connection_status ==
               PageInfo::SITE_CONNECTION_STATUS_ENCRYPTED) {
-        subtitle_text =
-            base::UTF8ToUTF16(identity_info.identity_status_description);
+        // An EV cert is required to have an organization name, a city
+        // (localityName), and country, but state is "if any".
+        if (!certificate_->subject().organization_names.empty() &&
+            !certificate_->subject().locality_name.empty() &&
+            !certificate_->subject().country_name.empty()) {
+          subtitle_text = l10n_util::GetStringFUTF16(
+              IDS_PAGE_INFO_SECURITY_TAB_SECURE_IDENTITY_EV_VERIFIED,
+              base::UTF8ToUTF16(certificate_->subject().organization_names[0]),
+              base::UTF8ToUTF16(certificate_->subject().country_name));
+        }
       }
     }
 

chrome/browser/ui/views/page_info/page_info_bubble_view_browsertest.cc

@@ -204,7 +204,6 @@ class PageInfoBubbleViewBrowserTest : public DialogBrowserTest {
       // the certificate button subtitle.
       identity.identity_status = PageInfo::SITE_IDENTITY_STATUS_EV_CERT;
       identity.connection_status = PageInfo::SITE_CONNECTION_STATUS_ENCRYPTED;
-      identity.identity_status_description = "Issued to: Thawte Inc [US]";
       scoped_refptr<net::X509Certificate> ev_cert =
           net::X509Certificate::CreateFromBytes(
               reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
@@ -367,6 +366,13 @@ class PageInfoBubbleViewBrowserTest : public DialogBrowserTest {
     return page_info_bubble_view->certificate_button_->title()->GetText();
   }
 
+  base::string16 GetCertificateButtonSubtitle() const {
+    PageInfoBubbleView* page_info_bubble_view =
+        static_cast<PageInfoBubbleView*>(
+            PageInfoBubbleView::GetPageInfoBubbleForTesting());
+    return page_info_bubble_view->certificate_button_->subtitle()->GetText();
+  }
+
   const base::string16 GetPageInfoBubbleViewDetailText() {
     PageInfoBubbleView* page_info_bubble_view =
         static_cast<PageInfoBubbleView*>(
@@ -745,6 +751,48 @@ IN_PROC_BROWSER_TEST_F(PageInfoBubbleViewBrowserTest, BlockedAndInvalidCert) {
                                        invalid_parens));
 }
 
+// Ensure a page that has an EV certificate *and* is blocked by Safe Browsing
+// shows the correct PageInfo UI. Regression test for crbug.com/1014240.
+IN_PROC_BROWSER_TEST_F(PageInfoBubbleViewBrowserTest, MalwareAndEvCert) {
+  net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_server.AddDefaultHandlers(
+      base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
+  ASSERT_TRUE(https_server.Start());
+
+  ui_test_utils::NavigateToURL(browser(), https_server.GetURL("/simple.html"));
+
+  // Generate a valid mock EV HTTPS identity, with an EV certificate. Must
+  // match conditions in PageInfoBubbleView::SetIdentityInfo() for setting
+  // the certificate button subtitle.
+  PageInfoUI::IdentityInfo identity;
+  identity.identity_status = PageInfo::SITE_IDENTITY_STATUS_EV_CERT;
+  identity.connection_status = PageInfo::SITE_CONNECTION_STATUS_ENCRYPTED;
+  scoped_refptr<net::X509Certificate> ev_cert =
+      net::X509Certificate::CreateFromBytes(
+          reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  ASSERT_TRUE(ev_cert);
+  identity.certificate = ev_cert;
+
+  // Have the page also trigger an SB malware warning.
+  identity.safe_browsing_status = PageInfo::SAFE_BROWSING_STATUS_MALWARE;
+
+  OpenPageInfoBubble(browser());
+  SetPageInfoBubbleIdentityInfo(identity);
+
+  views::BubbleDialogDelegateView* page_info =
+      PageInfoBubbleView::GetPageInfoBubbleForTesting();
+
+  // Verify bubble complains of malware...
+  EXPECT_EQ(page_info->GetWindowTitle(),
+            l10n_util::GetStringUTF16(IDS_PAGE_INFO_MALWARE_SUMMARY));
+
+  // ...and has the correct organization details in the Certificate button.
+  EXPECT_EQ(GetCertificateButtonSubtitle(),
+            l10n_util::GetStringFUTF16(
+                IDS_PAGE_INFO_SECURITY_TAB_SECURE_IDENTITY_EV_VERIFIED,
+                base::UTF8ToUTF16("Thawte Inc"), base::UTF8ToUTF16("US")));
+}
+
 namespace {
 
 // Tracks focus of an arbitrary UI element.

chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/hover_button.h"
 #include "chrome/browser/ui/views/page_info/chosen_object_view.h"
+#include "chrome/browser/ui/views/page_info/page_info_hover_button.h"
 #include "chrome/browser/ui/views/page_info/permission_selector_row.h"
 #include "chrome/browser/usb/usb_chooser_context.h"
 #include "chrome/browser/usb/usb_chooser_context_factory.h"
@@ -31,8 +32,10 @@
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_web_contents_factory.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
+#include "net/cert/cert_status_flags.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "net/test/cert_test_util.h"
+#include "net/test/test_certificate_data.h"
 #include "net/test/test_data_directory.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "services/device/public/cpp/test/fake_usb_device_manager.h"
@@ -129,6 +132,12 @@ class PageInfoBubbleViewTestApi {
     CreateView();
   }
 
+  base::string16 GetCertificateButtonSubtitleText() const {
+    EXPECT_TRUE(view_->certificate_button_);
+    EXPECT_TRUE(view_->certificate_button_->subtitle());
+    return view_->certificate_button_->subtitle()->GetText();
+  }
+
   void WaitForBubbleClose() { run_loop_.Run(); }
 
  private:
@@ -711,3 +720,41 @@ TEST_F(PageInfoBubbleViewTest, EnsureCloseCallback) {
             api_->closed_reason());
 }
 
+TEST_F(PageInfoBubbleViewTest, CertificateButtonShowsEvCertDetails) {
+  content::BrowserSideNavigationSetUp();
+  SecurityStateTabHelper::CreateForWebContents(
+      web_contents_helper_.web_contents());
+  std::unique_ptr<content::NavigationSimulator> navigation =
+      content::NavigationSimulator::CreateRendererInitiated(
+          GURL(kSecureUrl),
+          web_contents_helper_.web_contents()->GetMainFrame());
+  navigation->Start();
+  api_->CreateView();
+
+  // Set up a test SSLInfo so that Page Info sees the connection as secure and
+  // using an EV certificate.
+  uint16_t cipher_suite = 0xc02f;  // TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
+  int connection_status = 0;
+  net::SSLConnectionStatusSetCipherSuite(cipher_suite, &connection_status);
+  net::SSLConnectionStatusSetVersion(net::SSL_CONNECTION_VERSION_TLS1_2,
+                                     &connection_status);
+  net::SSLInfo ssl_info;
+  ssl_info.connection_status = connection_status;
+  ssl_info.cert = net::X509Certificate::CreateFromBytes(
+      reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  ASSERT_TRUE(ssl_info.cert);
+  ssl_info.cert_status = net::CERT_STATUS_IS_EV;
+
+  navigation->SetSSLInfo(ssl_info);
+
+  navigation->Commit();
+  EXPECT_EQ(l10n_util::GetStringUTF16(IDS_PAGE_INFO_SECURE_SUMMARY),
+            api_->GetWindowTitle());
+
+  // The certificate button subtitle should show the EV certificate organization
+  // name and country of incorporation.
+  EXPECT_EQ(l10n_util::GetStringFUTF16(
+                IDS_PAGE_INFO_SECURITY_TAB_SECURE_IDENTITY_EV_VERIFIED,
+                base::UTF8ToUTF16("Thawte Inc"), base::UTF8ToUTF16("US")),
+            api_->GetCertificateButtonSubtitleText());
+}

chrome/browser/ui/views/page_info/page_info_hover_button.h

@@ -13,6 +13,10 @@ namespace gfx {
 class ImageSkia;
 }  // namespace gfx
 
+namespace test {
+class PageInfoBubbleViewTestApi;
+}  // namespace test
+
 namespace views {
 class ButtonListener;
 class Label;
@@ -66,6 +70,7 @@ class PageInfoHoverButton : public HoverButton {
 
  private:
   friend class PageInfoBubbleViewBrowserTest;
+  friend class test::PageInfoBubbleViewTestApi;
 
   void UpdateAccessibleName();