Refactor client cert private key handling.

chrome/browser/ui/views/certificate_selector.cc

Index: chrome/browser/ui/views/certificate_selector.cc
diff --git a/chrome/browser/ui/views/certificate_selector.cc b/chrome/browser/ui/views/certificate_selector.cc
index 4ae2049f9d15d6252ed46d6ea1a65c373db4bad8..8986061943872dc49e44f939def37dae62e79ccb 100644
--- a/chrome/browser/ui/views/certificate_selector.cc
+++ b/chrome/browser/ui/views/certificate_selector.cc
@@ -45,8 +45,8 @@ 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:
@@ -67,11 +67,11 @@ class CertificateSelector::CertificateTableModel : public ui::TableModel {
 };
 
 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());
@@ -113,14 +113,13 @@ base::string16 CertificateSelector::CertificateTableModel::GetText(
 void CertificateSelector::CertificateTableModel::SetObserver(
     ui::TableModelObserver* observer) {}
 
-CertificateSelector::CertificateSelector(
-    const net::CertificateList& certificates,
-    content::WebContents* web_contents)
+CertificateSelector::CertificateSelector(net::ClientCertIdentityList identities,

[Peter Kasting, 2017/06/15 22:13:41]

I think we probably still want to pass by reference, especially since we're not
always moving the whole vector below?  For example, see guidance at
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/string...
.

[mattm, 2017/06/16 03:30:42]

I don't think that post is relevant, since the list isn't copyable.

This class is self-owning and completes asynchronously, the caller may not
continue to exist, so we can't depend on it to keep the list alive if it were
passed by reference, so we'd always have to move some or all the elements out of
the list. In the chromeos case it might be tiny bit more efficient(all you'd be
avoiding is a std::move(std::vector) which should be pretty cheap), but the
non-chromeos case would be clearly less efficient.

[mattm, 2017/06/16 03:55:12]

Scratch that last part, you could of course still move the whole list and not
just individual elements, if a reference was passed in.

Still, I think this way is clearer since you have to put the std::move at the
call site, it makes it obvious that the caller is giving up ownership of the
contents of the list.

+                                         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 =
@@ -130,11 +129,12 @@ CertificateSelector::CertificateSelector(
       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.
@@ -151,15 +151,15 @@ CertificateSelector::CertificateSelector(
       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() {
@@ -181,11 +181,11 @@ void CertificateSelector::Show() {
   // 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;
   }
@@ -239,12 +239,21 @@ ui::TableModel* CertificateSelector::table_model_for_testing() const {
   return model_.get();
 }
 
-net::X509Certificate* CertificateSelector::GetSelectedCert() const {
+net::ClientCertIdentity* CertificateSelector::GetSelectedCert() const {
   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());
+  return identities_[selected].get();
+}
+
+std::unique_ptr<net::ClientCertIdentity>
+CertificateSelector::TakeSelectedCert() {
+  const int selected = table_->FirstSelectedRow();
+  if (selected < 0)  // Nothing is selected in |table_|.
+    return nullptr;
+  CHECK_LT(static_cast<size_t>(selected), identities_.size());
+  return std::move(identities_[selected]);

[Peter Kasting, 2017/06/15 22:13:40]

Don't we also need to do identities_.erase()?  Otherwise we leave a dead slot in
the vector.  I mean, maybe that doesn't cause problems today, if nobody calls
this until the vector won't be used anymore anyway, but it feels like an unsafe
pattern.

[mattm, 2017/06/16 03:30:42]

I think the only reasonable thing is that once a cert is accepted no further
accesses should happen. Otherwise things just get weird. I've attempted to
refactor and fix some issues(eg disabling the dialog controls while waiting for
the async callback).

 }
 
 bool CertificateSelector::CanResize() const {
@@ -277,11 +286,10 @@ ui::ModalType CertificateSelector::GetModalType() const {
 
 void CertificateSelector::ButtonPressed(views::Button* sender,
                                         const ui::Event& event) {
-  if (sender == view_cert_button_) {
-    net::X509Certificate* const cert = GetSelectedCert();
-    if (cert)
-      ShowCertificateViewer(web_contents_,
-                            web_contents_->GetTopLevelNativeWindow(), cert);
+  if (sender == view_cert_button_ && GetSelectedCert()) {

[Peter Kasting, 2017/06/15 22:13:40]

Nit: I liked that the old code avoided calling AnExpensiveLookingFunctionName()
repeatedly but stored it in a temp.

[mattm, 2017/06/16 03:30:42]

Done.

+    ShowCertificateViewer(web_contents_,
+                          web_contents_->GetTopLevelNativeWindow(),
+                          GetSelectedCert()->certificate());
   }
 }