Remove NSSCertDatabase from ClientCertStoreChromeOS unittest.

net/ssl/client_cert_store_chromeos_unittest.cc

Index: net/ssl/client_cert_store_chromeos_unittest.cc
diff --git a/net/ssl/client_cert_store_chromeos_unittest.cc b/net/ssl/client_cert_store_chromeos_unittest.cc
index 65b668dbb25d21696337db71aeef49b0d5c58ded..fb08fa20ceef2169c2a1934e10311af4cd397bd4 100644
--- a/net/ssl/client_cert_store_chromeos_unittest.cc
+++ b/net/ssl/client_cert_store_chromeos_unittest.cc
@@ -4,94 +4,167 @@
 
 #include "net/ssl/client_cert_store_chromeos.h"
 
+#include <string>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/run_loop.h"
-#include "base/strings/utf_string_conversions.h"
 #include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
-#include "net/cert/nss_cert_database.h"
+#include "crypto/rsa_private_key.h"
+#include "net/base/test_data_directory.h"
+#include "net/cert/cert_type.h"
+#include "net/cert/x509_certificate.h"
 #include "net/ssl/client_cert_store_unittest-inl.h"
+#include "net/test/cert_test_util.h"
 
 namespace net {
 
+namespace {
+
+bool ImportClientCertToSlot(scoped_refptr<X509Certificate> cert,
+                            PK11SlotInfo* slot) {
+  CK_OBJECT_HANDLE key;
+  crypto::ScopedPK11Slot key_slot(
+      PK11_KeyForCertExists(cert->os_cert_handle(), &key, NULL));
+  if (slot != key_slot.get()) {
+    LOG(ERROR) << "Did not find key in the right slot";
+    return false;
+  }
+
+  std::string nickname = cert->GetDefaultNickname(USER_CERT);

[pneubeck (no reviews), 2014/07/17 13:40:50]

Changed the nickname from filename (because it's not provided anymore) to this
DefaultNickname.

+  {
+    crypto::AutoNSSWriteLock lock;
+    SECStatus rv = PK11_ImportCert(
+        slot, cert->os_cert_handle(), key, nickname.c_str(), PR_FALSE);
+    if (rv != SECSuccess) {
+      LOG(ERROR) << "Could not import cert";
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace
+
+class ClientCertStoreChromeOSTestDelegate {
+ public:
+  ClientCertStoreChromeOSTestDelegate()
+      : user_("scopeduser"),
+        store_(user_.username_hash(),
+               ClientCertStoreChromeOS::PasswordDelegateFactory()) {
+    CHECK(user_.constructed_successfully());

[pneubeck (no reviews), 2014/07/17 13:40:50]

very likely you don't like these CHECKs, although IMO this is OK because our
test infrastructure automatically runs tests isolatedly in case of crashes.

ASSERTs work neither because of the return type not being 'void'.
Do you require this to be changed to EXPECT and a ASSERT_NO_FATAL_FAILURES
wrapper at the call site? (I didn't try whether that actually works)

[Ryan Sleevi, 2014/07/17 18:42:20]

This is my preference, in part because I almost exclusively run tests without
the multi-process harness, because it makes debugging IMPOSSIBLE.

You can't quite wrapper a CTOR. Instead, it means an Init() method with a bool.

[pneubeck (no reviews), 2014/07/17 21:45:10]

I can do that tomorrow or in a follow up. Will mean adapting all the tests of
the different platforms (mechanical change however).
Would be good however to get this one landed at some finite time ;-)

+    user_.FinishInit();
+
+    slot_ = crypto::GetPublicSlotForChromeOSUser(user_.username_hash());
+    CHECK(slot_);
+
+    // Client certs can only be imported to |slot_| if the respective private
+    // key is present. Import the private keys for all client certificates that
+    // are used during the test.
+    ImportSensitiveKeyFromFile(
+        GetTestCertsDirectory(), "client_1.pk8", slot_.get());
+    ImportSensitiveKeyFromFile(
+        GetTestCertsDirectory(), "client_2.pk8", slot_.get());
+  }
+
+  bool SelectClientCerts(const CertificateList& input_certs,
+                         const SSLCertRequestInfo& cert_request_info,
+                         CertificateList* selected_certs) {
+    for (CertificateList::const_iterator it = input_certs.begin();
+         it != input_certs.end();
+         ++it) {
+      if (!ImportClientCertToSlot(*it, slot_.get()))
+        return false;
+    }
+    base::RunLoop run_loop;
+    store_.GetClientCerts(
+        cert_request_info, selected_certs, run_loop.QuitClosure());
+    run_loop.Run();
+    return true;
+  }
+
+ private:
+  crypto::ScopedTestNSSChromeOSUser user_;
+  ClientCertStoreChromeOS store_;
+  crypto::ScopedPK11Slot slot_;
+};
+
+// This tests whether the filtering functionality is correctly delegated to the
+// base class ClientCertStoreNSS.
+INSTANTIATE_TYPED_TEST_CASE_P(ChromeOS,
+                              ClientCertStoreTest,
+                              ClientCertStoreChromeOSTestDelegate);
 class ClientCertStoreChromeOSTest : public ::testing::Test {
  public:
   scoped_refptr<X509Certificate> ImportCertForUser(
       const std::string& username_hash,
-      const std::string& filename,
-      const std::string& password) {
+      const std::string& cert_filename,
+      const std::string& key_filename) {
     crypto::ScopedPK11Slot slot(
         crypto::GetPublicSlotForChromeOSUser(username_hash));
-    EXPECT_TRUE(slot.get());
-    if (!slot.get())
+    if (!slot) {
+      LOG(ERROR) << "No slot for user " << username_hash;
       return NULL;
+    }
 
-    net::CertificateList cert_list;
-
-    base::FilePath p12_path = GetTestCertsDirectory().AppendASCII(filename);
-    std::string p12_data;
-    if (!base::ReadFileToString(p12_path, &p12_data)) {
-      EXPECT_TRUE(false);
+    if (!ImportSensitiveKeyFromFile(
+            GetTestCertsDirectory(), key_filename, slot.get())) {
+      LOG(ERROR) << "Could not import private key for user " << username_hash;
       return NULL;
     }
 
-    scoped_refptr<net::CryptoModule> module(
-        net::CryptoModule::CreateFromHandle(slot.get()));
-    int rv = NSSCertDatabase::GetInstance()->ImportFromPKCS12(
-        module.get(), p12_data, base::UTF8ToUTF16(password), false, &cert_list);
+    scoped_refptr<X509Certificate> cert(
+        ImportCertFromFile(GetTestCertsDirectory(), cert_filename));
+
+    if (!cert) {
+      LOG(ERROR) << "Failed to parse cert from file " << cert_filename;
+      return NULL;
+    }
 
-    EXPECT_EQ(0, rv);
-    EXPECT_EQ(1U, cert_list.size());
-    if (rv || cert_list.size() != 1)
+    if (!ImportClientCertToSlot(cert, slot.get()))
       return NULL;
 
-    return cert_list[0];
+    return cert;
   }
 };
 
-// TODO(mattm): Do better testing of cert_authorities matching below. Update
-// net/data/ssl/scripts/generate-client-certificates.sh so that it actually
-// saves the .p12 files, and regenerate them.
-
-TEST_F(ClientCertStoreChromeOSTest, WaitForNSSInit) {
+TEST_F(ClientCertStoreChromeOSTest,
+       IfRequestCertsBeforeNSSDBInitializedThenRequestWaitsForInitAndSucceeds) {
   crypto::ScopedTestNSSChromeOSUser user("scopeduser");
   ASSERT_TRUE(user.constructed_successfully());
   ClientCertStoreChromeOS store(
       user.username_hash(), ClientCertStoreChromeOS::PasswordDelegateFactory());
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user.username_hash(), "client.p12", "12345"));
-  scoped_refptr<X509Certificate> cert_2(
-      ImportCertForUser(user.username_hash(), "websocket_client_cert.p12", ""));
-
-  std::vector<std::string> authority_1(
-      1,
-      std::string(reinterpret_cast<const char*>(kAuthority1DN),
-                  sizeof(kAuthority1DN)));
-  scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
-  request_1->cert_authorities = authority_1;
+      ImportCertForUser(user.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
 
+  // Request any client certificate, which is expected to match client_1.
   scoped_refptr<SSLCertRequestInfo> request_all(new SSLCertRequestInfo());
 
-  base::RunLoop run_loop_1;
-  base::RunLoop run_loop_all;
-  store.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
+  base::RunLoop run_loop;
   store.GetClientCerts(
-      *request_all, &request_all->client_certs, run_loop_all.QuitClosure());
-
-  // Callbacks won't be run until nss_util init finishes for the user.
+      *request_all, &request_all->client_certs, run_loop.QuitClosure());
+
+  {
+    base::RunLoop run_loop_inner;
+    run_loop_inner.RunUntilIdle();
+    // GetClientCerts should wait for the initialization of the user's DB to
+    // finish.
+    ASSERT_EQ(0u, request_all->client_certs.size());
+  }
+  // This should trigger the GetClientCerts operation to finish and to call
+  // back.
   user.FinishInit();
 
-  run_loop_1.Run();
-  run_loop_all.Run();
+  run_loop.Run();
 
-  ASSERT_EQ(0u, request_1->client_certs.size());
-  ASSERT_EQ(2u, request_all->client_certs.size());
+  ASSERT_EQ(1u, request_all->client_certs.size());
 }
 
-TEST_F(ClientCertStoreChromeOSTest, NSSAlreadyInitialized) {
+TEST_F(ClientCertStoreChromeOSTest,
+       IfRequestCertsAfterNSSDBInitializedThenRequestSucceeds) {
   crypto::ScopedTestNSSChromeOSUser user("scopeduser");
   ASSERT_TRUE(user.constructed_successfully());
   user.FinishInit();
@@ -99,70 +172,68 @@ TEST_F(ClientCertStoreChromeOSTest, NSSAlreadyInitialized) {
   ClientCertStoreChromeOS store(
       user.username_hash(), ClientCertStoreChromeOS::PasswordDelegateFactory());
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user.username_hash(), "client.p12", "12345"));
-  scoped_refptr<X509Certificate> cert_2(
-      ImportCertForUser(user.username_hash(), "websocket_client_cert.p12", ""));
-
-  std::vector<std::string> authority_1(
-      1,
-      std::string(reinterpret_cast<const char*>(kAuthority1DN),
-                  sizeof(kAuthority1DN)));
-  scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
-  request_1->cert_authorities = authority_1;
+      ImportCertForUser(user.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
 
   scoped_refptr<SSLCertRequestInfo> request_all(new SSLCertRequestInfo());
 
-  base::RunLoop run_loop_1;
-  base::RunLoop run_loop_all;
+  base::RunLoop run_loop;
   store.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
-  store.GetClientCerts(
-      *request_all, &request_all->client_certs, run_loop_all.QuitClosure());
+      *request_all, &request_all->client_certs, run_loop.QuitClosure());
 
-  run_loop_1.Run();
-  run_loop_all.Run();
+  run_loop.Run();
 
-  ASSERT_EQ(0u, request_1->client_certs.size());
-  ASSERT_EQ(2u, request_all->client_certs.size());
+  ASSERT_EQ(1u, request_all->client_certs.size());
 }
 
-TEST_F(ClientCertStoreChromeOSTest, TwoUsers) {
+// This verifies that a request in the context of User1 doesn't see certificates
+// of User2, and the other way round. We check both directions, to ensure that
+// the behavior doesn't depend on initialization order of the DBs, for example.
+TEST_F(ClientCertStoreChromeOSTest,
+       IfTwoNSSDBsExistThenCertRequestsReturnOnlyCertsOfTheGivenDB) {
   crypto::ScopedTestNSSChromeOSUser user1("scopeduser1");
   ASSERT_TRUE(user1.constructed_successfully());
   crypto::ScopedTestNSSChromeOSUser user2("scopeduser2");
   ASSERT_TRUE(user2.constructed_successfully());
+
+  user1.FinishInit();
+  user2.FinishInit();
+
   ClientCertStoreChromeOS store1(
       user1.username_hash(),
       ClientCertStoreChromeOS::PasswordDelegateFactory());
   ClientCertStoreChromeOS store2(
       user2.username_hash(),
       ClientCertStoreChromeOS::PasswordDelegateFactory());
+
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user1.username_hash(), "client.p12", "12345"));
-  scoped_refptr<X509Certificate> cert_2(ImportCertForUser(
-      user2.username_hash(), "websocket_client_cert.p12", ""));
+      ImportCertForUser(user1.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
+  scoped_refptr<X509Certificate> cert_2(
+      ImportCertForUser(user2.username_hash(), "client_2.pem", "client_2.pk8"));
+  ASSERT_TRUE(cert_2);
 
-  scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
-  scoped_refptr<SSLCertRequestInfo> request_2(new SSLCertRequestInfo());
+  scoped_refptr<SSLCertRequestInfo> request_all(new SSLCertRequestInfo());
 
   base::RunLoop run_loop_1;
   base::RunLoop run_loop_2;
+
+  CertificateList selected_certs1, selected_certs2;
   store1.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
+      *request_all, &selected_certs1, run_loop_1.QuitClosure());
   store2.GetClientCerts(
-      *request_2, &request_2->client_certs, run_loop_2.QuitClosure());
-
-  // Callbacks won't be run until nss_util init finishes for the user.
-  user1.FinishInit();
-  user2.FinishInit();
+      *request_all, &selected_certs2, run_loop_2.QuitClosure());
 
   run_loop_1.Run();
   run_loop_2.Run();
 
-  ASSERT_EQ(1u, request_1->client_certs.size());
-  EXPECT_TRUE(cert_1->Equals(request_1->client_certs[0]));
-  // TODO(mattm): Request for second user will have zero results due to
-  // crbug.com/315285.  Update the test once that is fixed.
+  // store1 should only return certs of user1, namely cert_1.
+  ASSERT_EQ(1u, selected_certs1.size());
+  EXPECT_TRUE(cert_1->Equals(selected_certs1[0]));
+
+  // store2 should only return certs of user2, namely cert_2.
+  ASSERT_EQ(1u, selected_certs2.size());
+  EXPECT_TRUE(cert_2->Equals(selected_certs2[0]));
 }
 
 }  // namespace net