Refactor client cert private key handling.

chrome/browser/ui/views/certificate_selector.cc

 // Copyright 2015 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/ui/views/certificate_selector.h"
 
 #include <stddef.h>  // For size_t.
 #include <string>
 #include <vector>
 
@@ skipping 27 matching lines @@
 #include "extensions/browser/extension_registry_factory.h"
 #endif
 
 namespace chrome {
 
 const int CertificateSelector::kTableViewWidth = 500;
 const int CertificateSelector::kTableViewHeight = 150;
 
 class CertificateSelector::CertificateTableModel : public ui::TableModel {
  public:
-  // |certs| and |provider_names| must have the same size.
-  CertificateTableModel(const net::CertificateList& certs,
+  // |identities| and |provider_names| must have the same size.
+  CertificateTableModel(const net::ClientCertIdentityList& identities,
                         const std::vector<std::string>& provider_names);
 
   // ui::TableModel:
   int RowCount() override;
   base::string16 GetText(int index, int column_id) override;
   void SetObserver(ui::TableModelObserver* observer) override;
 
  private:
   struct Row {
     base::string16 subject;
     base::string16 issuer;
     base::string16 provider;
     base::string16 serial;
   };
   std::vector<Row> rows_;
 
   DISALLOW_COPY_AND_ASSIGN(CertificateTableModel);
 };
 
 CertificateSelector::CertificateTableModel::CertificateTableModel(
-    const net::CertificateList& certs,
+    const net::ClientCertIdentityList& identities,
     const std::vector<std::string>& provider_names) {
-  DCHECK_EQ(certs.size(), provider_names.size());
-  for (size_t i = 0; i < certs.size(); i++) {
-    net::X509Certificate* cert = certs[i].get();
+  DCHECK_EQ(identities.size(), provider_names.size());
+  for (size_t i = 0; i < identities.size(); i++) {
+    net::X509Certificate* cert = identities[i]->certificate();
     Row row;
     row.subject = base::UTF8ToUTF16(cert->subject().GetDisplayName());
     row.issuer = base::UTF8ToUTF16(cert->issuer().GetDisplayName());
     row.provider = base::UTF8ToUTF16(provider_names[i]);
     if (cert->serial_number().size() < std::numeric_limits<size_t>::max() / 2) {
       row.serial = base::UTF8ToUTF16(base::HexEncode(
           cert->serial_number().data(), cert->serial_number().size()));
     }
     rows_.push_back(row);
   }
@@ skipping 21 matching lines @@
       return row.serial;
     default:
       NOTREACHED();
   }
   return base::string16();
 }
 
 void CertificateSelector::CertificateTableModel::SetObserver(
     ui::TableModelObserver* observer) {}
 
-CertificateSelector::CertificateSelector(
-    const net::CertificateList& certificates,
-    content::WebContents* web_contents)
+CertificateSelector::CertificateSelector(net::ClientCertIdentityList identities,
+                                         content::WebContents* web_contents)
     : web_contents_(web_contents), table_(nullptr), view_cert_button_(nullptr) {
   CHECK(web_contents_);
 
-  // |provider_names| and |certificates_| are parallel arrays.
-  // The entry at index |i| is the provider name for |certificates_[i]|.
+  // |provider_names| and |identities_| are parallel arrays.
+  // The entry at index |i| is the provider name for |identities_[i]|.
   std::vector<std::string> provider_names;
 #if defined(OS_CHROMEOS)
   chromeos::CertificateProviderService* service =
       chromeos::CertificateProviderServiceFactory::GetForBrowserContext(
           web_contents->GetBrowserContext());
   extensions::ExtensionRegistry* extension_registry =
       extensions::ExtensionRegistryFactory::GetForBrowserContext(
           web_contents->GetBrowserContext());
 
-  for (const auto& cert : certificates) {
+  for (auto& identity : identities) {
     std::string provider_name;
     bool has_extension = false;
     std::string extension_id;
-    if (service->LookUpCertificate(*cert, &has_extension, &extension_id)) {
+    if (service->LookUpCertificate(*identity->certificate(), &has_extension,
+                                   &extension_id)) {
       if (!has_extension) {
         // This certificate was provided by an extension but isn't provided by
         // any extension currently. Don't expose it to the user.
         continue;
       }
       const auto* extension = extension_registry->GetExtensionById(
           extension_id, extensions::ExtensionRegistry::ENABLED);
       if (!extension) {
         // This extension was unloaded in the meantime. Don't show the
         // certificate.
         continue;
       }
       provider_name = extension->short_name();
       show_provider_column_ = true;
     }  // Otherwise the certificate is provided by the platform.
 
-    certificates_.push_back(cert);
+    identities_.push_back(std::move(identity));
     provider_names.push_back(provider_name);
   }
 #else
-  provider_names.assign(certificates.size(), std::string());
-  certificates_ = certificates;
+  provider_names.assign(identities.size(), std::string());
+  identities_ = std::move(identities);
 #endif
 
-  model_.reset(new CertificateTableModel(certificates_, provider_names));
+  model_.reset(new CertificateTableModel(identities_, provider_names));
 }
 
 CertificateSelector::~CertificateSelector() {
   table_->SetModel(nullptr);
 }
 
 // static
 bool CertificateSelector::CanShow(content::WebContents* web_contents) {
   // GetTopLevelWebContents returns |web_contents| if it is not a guest.
   content::WebContents* top_level_web_contents =
       guest_view::GuestViewBase::GetTopLevelWebContents(web_contents);
   return web_modal::WebContentsModalDialogManager::FromWebContents(
              top_level_web_contents) != nullptr;
 }
 
 void CertificateSelector::Show() {
   constrained_window::ShowWebModalDialogViews(this, web_contents_);
 
   // TODO(isandrk): A certificate that was previously provided by *both* the
   // platform and an extension will get incorrectly filtered out if the
   // extension stops providing it (both instances will be filtered out), hence
-  // the |certificates_| array will be empty. Displaying a dialog with an empty
+  // the |identities_| array will be empty. Displaying a dialog with an empty
   // list won't make much sense for the user, and also there are some CHECKs in
   // the code that will fail when the list is empty and that's why an early exit
   // is performed here. See crbug.com/641440 for more details.
-  if (certificates_.empty()) {
+  if (identities_.empty()) {
     GetWidget()->Close();
     return;
   }
 
   // Select the first row automatically.  This must be done after the dialog has
   // been created.
   table_->Select(0);
 }
 
 void CertificateSelector::InitWithText(
@@ skipping 33 matching lines @@
                   views::GridLayout::FILL, views::GridLayout::FILL,
                   kTableViewWidth, kTableViewHeight);
 
   layout->AddPaddingRow(0, vertical_spacing);
 }
 
 ui::TableModel* CertificateSelector::table_model_for_testing() const {
   return model_.get();
 }
 
-net::X509Certificate* CertificateSelector::GetSelectedCert() const {
+net::ClientCertIdentity* CertificateSelector::GetSelectedCert() const {
+  DCHECK(!certificate_accepted_);
   const int selected = table_->FirstSelectedRow();
   if (selected < 0)  // Nothing is selected in |table_|.
     return nullptr;
-  CHECK_LT(static_cast<size_t>(selected), certificates_.size());
-  return certificates_[selected].get();
+  CHECK_LT(static_cast<size_t>(selected), identities_.size());

[Peter Kasting, 2017/06/16 23:29:52]

Nit: Seems like this should be DCHECK?  (I think the old code was wrong)

[mattm, 2017/06/17 03:20:07]

Done.

+  return identities_[selected].get();
+}
+
+bool CertificateSelector::Accept() {
+  DCHECK(!certificate_accepted_);
+  const int selected = table_->FirstSelectedRow();
+  if (selected < 0)  // Nothing is selected in |table_|.
+    return false;
+
+  certificate_accepted_ = true;
+  table_->SetEnabled(false);
+  view_cert_button_->SetEnabled(false);
+  GetDialogClientView()->ok_button()->SetEnabled(false);
+  GetDialogClientView()->cancel_button()->SetEnabled(false);
+
+  CHECK_LT(static_cast<size_t>(selected), identities_.size());

[Peter Kasting, 2017/06/16 23:29:52]

Nit: Seems like this should be DCHECK too?

[mattm, 2017/06/17 03:20:07]

Done.

+  return AcceptCertificate(std::move(identities_[selected]));

[Peter Kasting, 2017/06/16 23:29:52]

So, if AcceptCertificate() returns false, then we leave the dialog onscreen but
with various things disabled?

It seems like maybe a better solution might be to not disable anything above,
and not return a bool, but instead Close() the view from here, and then just
AcceptCertificate().  (Note that Close() does a sync hide + async close, so it
won't delete any data structures until the callstack returns.)

I think in that case we can nuke the |certificate_accepted_| variable too.

The only question mark here would be if closing the dialog also deletes this
object, and you need to read from this object after this callstack would have
otherwise returned.  If both these are the case, then the ideal solution would
be to either pass along the data you'd have read out of this object (so it can
be deleted now), or make this object's lifetime able to extend past the dialog's
lifetime, or split the data that needs to live longer into a separate model
object that can live longer -- but in any case, to avoid leaving the dialog
onscreen, since that's less responsive UI.

[mattm, 2017/06/17 03:20:07]

Done. (Though I just did it by returning true, I think that's basically
equivalent to calling Close() directly?)
Yeah, the selector needed to continue to exist until the private key was
retrieved. I refactored it to split out the part that needs to stay alive, so
now the dialog can close immediately.

 }
 
 bool CertificateSelector::CanResize() const {
   return true;
 }
 
 base::string16 CertificateSelector::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_CLIENT_CERT_DIALOG_TITLE);
 }
 
 bool CertificateSelector::IsDialogButtonEnabled(ui::DialogButton button) const {
-  return button != ui::DIALOG_BUTTON_OK || GetSelectedCert() != nullptr;
+  return !certificate_accepted_ &&
+         (button != ui::DIALOG_BUTTON_OK || GetSelectedCert() != nullptr);
 }
 
 views::View* CertificateSelector::GetInitiallyFocusedView() {
   DCHECK(table_);
   return table_;
 }
 
 views::View* CertificateSelector::CreateExtraView() {
   DCHECK(!view_cert_button_);
   view_cert_button_ = views::MdTextButton::CreateSecondaryUiButton(
       this, l10n_util::GetStringUTF16(IDS_PAGE_INFO_CERT_INFO_BUTTON));
   return view_cert_button_;
 }
 
 ui::ModalType CertificateSelector::GetModalType() const {
   return ui::MODAL_TYPE_CHILD;
 }
 
 void CertificateSelector::ButtonPressed(views::Button* sender,
                                         const ui::Event& event) {
+  DCHECK(!certificate_accepted_);
   if (sender == view_cert_button_) {
-    net::X509Certificate* const cert = GetSelectedCert();
-    if (cert)
+    net::ClientCertIdentity* const cert = GetSelectedCert();
+    if (cert) {
       ShowCertificateViewer(web_contents_,
-                            web_contents_->GetTopLevelNativeWindow(), cert);
+                            web_contents_->GetTopLevelNativeWindow(),
+                            cert->certificate());
+    }
   }
 }
 
 void CertificateSelector::OnSelectionChanged() {
-  GetDialogClientView()->ok_button()->SetEnabled(GetSelectedCert() != nullptr);
+  GetDialogClientView()->ok_button()->SetEnabled(!certificate_accepted_ &&
+                                                 GetSelectedCert() != nullptr);
 }
 
 void CertificateSelector::OnDoubleClick() {
+  DCHECK(!certificate_accepted_);
   if (GetSelectedCert())
     GetDialogClientView()->AcceptWindow();
 }
 
 }  // namespace chrome