NSSCertDatabaseChromeOS

net/cert/nss_cert_database_chromeos_unittest.cc

Index: net/cert/nss_cert_database_chromeos_unittest.cc
diff --git a/net/cert/nss_cert_database_chromeos_unittest.cc b/net/cert/nss_cert_database_chromeos_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..470529b6c457f43f67b5ac4079783af6c7bf8057
--- /dev/null
+++ b/net/cert/nss_cert_database_chromeos_unittest.cc
@@ -0,0 +1,191 @@
+// Copyright 2013 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 "net/cert/nss_cert_database_chromeos.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/run_loop.h"
+#include "crypto/nss_util.h"
+#include "crypto/nss_util_internal.h"
+#include "net/base/test_data_directory.h"
+#include "net/cert/cert_database.h"
+#include "net/test/cert_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+bool IsCertInCertificateList(const X509Certificate* cert,
+                             const CertificateList& cert_list) {
+  for (CertificateList::const_iterator it = cert_list.begin();
+       it != cert_list.end();
+       ++it) {
+    if ((*it)->os_cert_handle() == cert->os_cert_handle())

[Ryan Sleevi, 2013/12/18 21:28:32]

BUG? Are you sure comparing OS cert handle equality is what you want?

There are cases where it can be the same (logical, under the hood, PKCS#11
cert), but different CERTCertificate*s

Use X509Certificate::IsSameOSCert because of this

you could also:

return find_if(cert_list.begin(), cert_list.end(),
ptr_fun(X509Certificate::IsSameOSCert)) != cert_list.end()

if you like the STL functional approach :)

[mattm, 2013/12/19 22:35:00]

Done.
 
Would need a bit of extra magic to get the os_cert_handle, and to bind one arg
to the one we are looking for ..

+      return true;
+  }
+  return false;
+}
+
+}  // namespace
+
+class NSSCertDatabaseChromeOSTest : public testing::Test,
+                                    public CertDatabase::Observer {
+ public:
+  NSSCertDatabaseChromeOSTest() : user_1_("user1"), user_2_("user2") {}
+
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(user_1_.constructed_successfully());
+    ASSERT_TRUE(user_2_.constructed_successfully());
+    user_1_.FinishInit();
+    user_2_.FinishInit();
+    db_1_.reset(new NSSCertDatabaseChromeOS(
+        crypto::GetPublicSlotForChromeOSUser(user_1_.username_hash()),
+        crypto::GetPrivateSlotForChromeOSUser(
+            user_1_.username_hash(),
+            base::Callback<void(crypto::ScopedPK11Slot)>())));
+    db_2_.reset(new NSSCertDatabaseChromeOS(
+        crypto::GetPublicSlotForChromeOSUser(user_2_.username_hash()),
+        crypto::GetPrivateSlotForChromeOSUser(
+            user_2_.username_hash(),
+            base::Callback<void(crypto::ScopedPK11Slot)>())));
+    CertDatabase::GetInstance()->AddObserver(this);
+  }
+
+  virtual void TearDown() OVERRIDE {
+    CertDatabase::GetInstance()->RemoveObserver(this);

[Ryan Sleevi, 2013/12/18 21:28:32]

If either user_1_.constructed_successfully() or
user_2_.constructed_successfully() is false, you will attempt to remove an
invalid observer (since ASSERT_TRUE will short-circuit return)

[mattm, 2013/12/19 22:35:00]

The observer list seems to be fine with trying to remove something not in it,
but I'll add a check anyway to be safe.

+  }
+
+  // CertDatabase::Observer:
+  virtual void OnCertAdded(const X509Certificate* cert) OVERRIDE {
+    added_.push_back(cert ? cert->os_cert_handle() : NULL);
+  }
+
+  virtual void OnCertRemoved(const X509Certificate* cert) OVERRIDE {}
+
+  virtual void OnCACertChanged(const X509Certificate* cert) OVERRIDE {
+    added_ca_.push_back(cert ? cert->os_cert_handle() : NULL);
+  }
+
+ protected:
+  std::vector<CERTCertificate*> added_ca_;
+  std::vector<CERTCertificate*> added_;

[Ryan Sleevi, 2013/12/18 21:28:32]

comments for |added_| and |added_ca_|

[mattm, 2013/12/19 22:35:00]

Done.

+  crypto::ScopedTestNSSChromeOSUser user_1_;
+  crypto::ScopedTestNSSChromeOSUser user_2_;
+  scoped_ptr<NSSCertDatabaseChromeOS> db_1_;
+  scoped_ptr<NSSCertDatabaseChromeOS> db_2_;
+};
+
+TEST_F(NSSCertDatabaseChromeOSTest, ListModules) {

[Ryan Sleevi, 2013/12/18 21:28:32]

Please add a comment before each test describing what you're testing.

Please also feel free to liberally comment what each "logical block" within the
tests are doing.

Ideally, without 'reading code', one can quickly scan the comments to
understand:

What is being tested
 How each case is tested

[mattm, 2013/12/19 22:35:00]

Done.

+  CryptoModuleList modules_1;
+  CryptoModuleList modules_2;
+
+  db_1_->ListModules(&modules_1, false /* need_rw */);
+  db_2_->ListModules(&modules_2, false /* need_rw */);
+
+  bool found_1 = false;
+  for (CryptoModuleList::iterator it = modules_1.begin(); it != modules_1.end();
+       ++it) {
+    EXPECT_NE(db_2_->GetPublicSlot().get(), (*it)->os_module_handle());
+    if ((*it)->os_module_handle() == db_1_->GetPublicSlot().get())
+      found_1 = true;
+  }
+  EXPECT_TRUE(found_1);
+
+  bool found_2 = false;
+  for (CryptoModuleList::iterator it = modules_2.begin(); it != modules_2.end();
+       ++it) {
+    EXPECT_NE(db_1_->GetPublicSlot().get(), (*it)->os_module_handle());
+    if ((*it)->os_module_handle() == db_2_->GetPublicSlot().get())
+      found_2 = true;
+  }
+  EXPECT_TRUE(found_2);
+}
+
+TEST_F(NSSCertDatabaseChromeOSTest, ImportCACerts) {
+  CertificateList certs_1 =
+      CreateCertificateListFromFile(GetTestCertsDirectory(),
+                                    "root_ca_cert.pem",
+                                    X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(1U, certs_1.size());
+  EXPECT_FALSE(certs_1[0]->os_cert_handle()->isperm);
+
+  CertificateList certs_2 =
+      CreateCertificateListFromFile(GetTestCertsDirectory(),
+                                    "2048-rsa-root.pem",
+                                    X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(1U, certs_2.size());
+  EXPECT_FALSE(certs_2[0]->os_cert_handle()->isperm);

[Ryan Sleevi, 2013/12/18 21:28:32]

I don't understand why you're doing this check. Same with 113.

[mattm, 2013/12/19 22:35:00]

Yeah, it's not necessary here. copy-pasta from the other test, where it was
useful to diagnose test failures if the home-dir nss DB happened to have some of
the test certs in it for whatever reason. But that shouldn't affect these tests
since they don't open the home-dir db.

+
+  NSSCertDatabase::ImportCertFailureList failed;
+  EXPECT_TRUE(
+      db_1_->ImportCACerts(certs_1, NSSCertDatabase::TRUSTED_SSL, &failed));
+  EXPECT_EQ(0U, failed.size());
+  failed.clear();
+  EXPECT_TRUE(
+      db_2_->ImportCACerts(certs_2, NSSCertDatabase::TRUSTED_SSL, &failed));
+  EXPECT_EQ(0U, failed.size());
+
+  CertificateList user_1_certlist;
+  CertificateList user_2_certlist;
+  db_1_->ListCerts(&user_1_certlist);
+  db_2_->ListCerts(&user_2_certlist);
+
+  EXPECT_TRUE(IsCertInCertificateList(certs_1[0], user_1_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_1[0], user_2_certlist));
+
+  EXPECT_TRUE(IsCertInCertificateList(certs_2[0], user_2_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_2[0], user_1_certlist));
+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(2U, added_ca_.size());
+  // TODO(mattm): make NSSCertDatabase actually pass the cert to the callback,
+  // and enable these checks:
+  // EXPECT_EQ(certs_1[0]->os_cert_handle(), added_ca_[0]);
+  // EXPECT_EQ(certs_2[0]->os_cert_handle(), added_ca_[1]);
+  EXPECT_EQ(0U, added_.size());
+}
+
+TEST_F(NSSCertDatabaseChromeOSTest, ImportServerCert) {
+  CertificateList certs_1 = CreateCertificateListFromFile(
+      GetTestCertsDirectory(), "ok_cert.pem", X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(1U, certs_1.size());
+  EXPECT_FALSE(certs_1[0]->os_cert_handle()->isperm);
+
+  CertificateList certs_2 =
+      CreateCertificateListFromFile(GetTestCertsDirectory(),
+                                    "2048-rsa-ee-by-2048-rsa-intermediate.pem",
+                                    X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(1U, certs_2.size());
+  EXPECT_FALSE(certs_2[0]->os_cert_handle()->isperm);
+
+  NSSCertDatabase::ImportCertFailureList failed;
+  EXPECT_TRUE(
+      db_1_->ImportServerCert(certs_1, NSSCertDatabase::TRUSTED_SSL, &failed));
+  EXPECT_EQ(0U, failed.size());
+  failed.clear();
+  EXPECT_TRUE(
+      db_2_->ImportServerCert(certs_2, NSSCertDatabase::TRUSTED_SSL, &failed));
+  EXPECT_EQ(0U, failed.size());
+
+  CertificateList user_1_certlist;
+  CertificateList user_2_certlist;
+  db_1_->ListCerts(&user_1_certlist);
+  db_2_->ListCerts(&user_2_certlist);
+
+  EXPECT_TRUE(IsCertInCertificateList(certs_1[0], user_1_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_1[0], user_2_certlist));
+
+  EXPECT_TRUE(IsCertInCertificateList(certs_2[0], user_2_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_2[0], user_1_certlist));
+
+  base::RunLoop().RunUntilIdle();
+  // TODO(mattm): ImportServerCert doesn't actually cause any observers to
+  // fire. Is that correct?
+  EXPECT_EQ(0U, added_ca_.size());
+  EXPECT_EQ(0U, added_.size());
+}
+
+}  // namespace net