ClientCertStoreChromeOS: support additional non-platform certs.

chrome/browser/chromeos/net/client_cert_store_chromeos.cc

Index: chrome/browser/chromeos/net/client_cert_store_chromeos.cc
diff --git a/chrome/browser/chromeos/net/client_cert_store_chromeos.cc b/chrome/browser/chromeos/net/client_cert_store_chromeos.cc
index 79c7b80e3dbdbdd186ed62966e557745ac554f98..692d61966a5c48c409b3538045a6148993d9cb46 100644
--- a/chrome/browser/chromeos/net/client_cert_store_chromeos.cc
+++ b/chrome/browser/chromeos/net/client_cert_store_chromeos.cc
@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
 
 namespace chromeos {
 
@@ -31,9 +32,11 @@ class CertNotAllowedPredicate {
 }  // namespace
 
 ClientCertStoreChromeOS::ClientCertStoreChromeOS(
+    scoped_ptr<CertificateProvider> cert_provider,
     scoped_ptr<CertFilter> cert_filter,
     const PasswordDelegateFactory& password_delegate_factory)
     : ClientCertStoreNSS(password_delegate_factory),
+      cert_provider_(cert_provider.Pass()),
       cert_filter_(cert_filter.Pass()) {}
 
 ClientCertStoreChromeOS::~ClientCertStoreChromeOS() {}
@@ -42,14 +45,25 @@ void ClientCertStoreChromeOS::GetClientCerts(
     const net::SSLCertRequestInfo& cert_request_info,
     net::CertificateList* selected_certs,
     const base::Closure& callback) {
-  base::Closure bound_callback = base::Bind(
-      &ClientCertStoreChromeOS::CertFilterInitialized,
-      // Caller is responsible for keeping the ClientCertStore alive
-      // until the callback is run.
-      base::Unretained(this), &cert_request_info, selected_certs, callback);
-
-  if (cert_filter_->Init(bound_callback))
-    bound_callback.Run();
+  // Caller is responsible for keeping the ClientCertStore alive until the
+  // callback is run.
+  base::Closure get_certs_after_init = base::Bind(
+      &ClientCertStoreChromeOS::GetClientCertsAfterInit, base::Unretained(this),
+      &cert_request_info, selected_certs, callback);
+
+  base::Closure get_additional_certs_and_continue;
+  if (cert_provider_) {
+    get_additional_certs_and_continue =
+        base::Bind(&CertificateProvider::GetCertificates,
+                   base::Unretained(cert_provider_.get()),
+                   base::Bind(&ClientCertStoreChromeOS::SetAdditionalCerts,
+                              base::Unretained(this), get_certs_after_init));
+  } else {
+    get_additional_certs_and_continue = get_certs_after_init;
+  }
+
+  if (cert_filter_->Init(get_additional_certs_and_continue))
+    get_additional_certs_and_continue.Run();
 }
 
 void ClientCertStoreChromeOS::GetClientCertsImpl(
@@ -67,9 +81,31 @@ void ClientCertStoreChromeOS::GetClientCertsImpl(
       selected_certs->end());
   DVLOG(1) << "filtered " << pre_size - selected_certs->size() << " of "
            << pre_size << " certs";
+
+  // Additionally to the matching certs from the platform's store, allow
+  // matching certificates from |additional_certs_|.
+  CERTCertList* additional_cert_list = CERT_NewCertList();
+  for (const auto& cert : additional_certs_) {
+    CERT_AddCertToListTail(additional_cert_list,
+                           CERT_DupCertificate(cert->os_cert_handle()));
+  }
+  net::CertificateList filtered_additional_certs;
+  net::ClientCertStoreNSS::GetClientCertsImpl(
+      additional_cert_list, request, query_nssdb, &filtered_additional_certs);
+
+  selected_certs->insert(selected_certs->end(),
+                         filtered_additional_certs.begin(),
+                         filtered_additional_certs.end());
+}
+
+void ClientCertStoreChromeOS::SetAdditionalCerts(
+    const base::Closure& callback,
+    const net::CertificateList& certs) {
+  additional_certs_ = certs;

[davidben, 2015/08/14 21:49:16]

Hrm. Although we never actually call GetClientCerts more than once and this
interface is a mess, it is sort of weird to implement it this way, making it
stateful. (What if we have two GetClientCerts going on in parallel?)

Actually, why not do this:

1. GetClientCertsAfterInit takes a net::CertificateList parameter.
2. Use net::ClientCertStoreNSS::GetClientCertsImpl to filter that additional
list.
3. Call net::ClientCertStoreNSS::GetClientCerts with a different callback which
concatenates your lists together and calls the real callback.

In fact, if you do this (or even as they are now), it starts to seem like this
whole subclassing thing is kind of unnecessary to begin with. Instead you could:

1. Split net::ClientCertStoreNSS::GetClientCertsImpl into a random utility
function you can just call. FilterCertificateList or something.
2. ClientCertStoreChromeOS implements ClientCertStore directly and contains a
ClientCertStoreNSS.
3. ClientCertStoreChromeOS::GetClientCerts does:
   a. Look up certificates from ClientCertStoreNSS.
   -- async --
   b. Call additional CertNotAllowedPredicate on that list.
   c. Look up certificates from CertificateProvider.
   -- async --
   d. Call FilterCertificateList on that list.
   e. Call the callback with the concatenation of those two.

Composition and all that good stuff. :-)

[pneubeck (no reviews), 2015/08/17 12:01:32]

Not as easy because a lot of the NSS stuff was executed on the worker pool. What
everyone overlooked so far, is that the complete list of matching certs must be
sorted. I fixed that in the last patch set.

I went ahead an broke the ClientCertStoreNSS into static functions and removed
any inheritance. That should make this class much simpler now.

There's some duplication now to post to the worker pool. But IMO that's
acceptable and avoiding this duplication will mean more callbacks/virtuals...

+  callback.Run();
 }
 
-void ClientCertStoreChromeOS::CertFilterInitialized(
+void ClientCertStoreChromeOS::GetClientCertsAfterInit(
     const net::SSLCertRequestInfo* request,
     net::CertificateList* selected_certs,
     const base::Closure& callback) {