Disk Based Certificate Cache Implementation

net/http/disk_based_cert_cache_unittest.cc

Index: net/http/disk_based_cert_cache_unittest.cc
diff --git a/net/http/disk_based_cert_cache_unittest.cc b/net/http/disk_based_cert_cache_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e6b4afc52ca1e3d1d1fb9af9ef8ba2723d9143b0
--- /dev/null
+++ b/net/http/disk_based_cert_cache_unittest.cc
@@ -0,0 +1,258 @@
+// Copyright (c) 2014 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/http/disk_based_cert_cache.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
+#include "net/base/test_data_directory.h"
+#include "net/disk_cache/memory/mem_backend_impl.h"
+#include "net/http/mock_http_cache.h"
+#include "net/test/cert_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"

[wtc, 2014/06/21 00:43:13]

Add a blank line after the last #include statement.

+namespace net {
+
+namespace {
+
+// MockTransactions are required to use the blocking Mock Disk Cache Backend.

[wtc, 2014/06/21 00:43:13]

Nit: Mock Disk Cache Backend => MockDiskCache Backend ?

+
+// This transaction corresponds to "root_ca_cert.pem" in GetTestCertsDirectory()

[wtc, 2014/06/21 00:43:13]

Nit: add a period at the end of the sentence. Comments should use proper
punctuations.

+const MockTransaction kCertTransaction1 = {
+    "cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4", "",           base::Time(),
+    "",                                              LOAD_NORMAL,  "",
+    "",                                              base::Time(), "",
+    TEST_MODE_NORMAL,                                NULL,         0};

[wtc, 2014/06/21 00:43:13]

Nit: our Style Guide does not require aligning the struct members like this. I
suggest running "git cl format" and let it format the code for you.

[brandonsalmon, 2014/06/21 02:21:32]

In this case I also think this is ugly, however actually git cl format actually
does do it this way.

[wtc, 2014/06/24 00:21:04]

If git cl format formatted the code this way, then it's fine.

+
+// This transaction corresponds to "ok_cert.pem" in GetTestCertsDirectory()
+const MockTransaction kCertTransaction2 = {
+    "cert:9174C7CB9E4919604E7B1BFC430E4929DA45F65F", "",           base::Time(),
+    "",                                              LOAD_NORMAL,  "",
+    "",                                              base::Time(), "",
+    TEST_MODE_NORMAL,                                NULL,         0};
+
+// MockCertCache is used so that results from the DiskBasedCertCache can be
+// achieved

[wtc, 2014/06/21 00:43:13]

Nit: "achieved" seems wrong. Did you mean "received" or "obtained"?

+// using CompletionCallback::WaitForResult.

[wtc, 2014/06/21 00:43:13]

Nit: this fits on the previous line.

+class MockCertCache {
+ public:
+  MockCertCache()
+      : backend(new MockDiskCache()),
+        cert_cache_(new DiskBasedCertCache(backend.get())),
+        weak_factory_(this) {}
+
+  void Set(const X509Certificate::OSCertHandle cert_handle,

[wtc, 2014/06/21 00:43:13]

Nit: this 'const' isn't necessary. (X509Certificate::OSCertHandle is a pointer.)

+           std::string& key,

[wtc, 2014/06/21 00:43:13]

IMPORTANT: our Style Guide requires using pointers instead of references for
output parameters. So this shoudl be

  std::string* key,

and the cert_handle parameter of the Get method should be

  X509Certificate::OSCertHandle* cert_handle,

+           CompletionCallback cb) {

[wtc, 2014/06/21 00:43:13]

Nit: our Style Guide recommends against using uncommon abbreviations for
variable/parameter names. So where possible, use "callback" instead of "cb".

+    cert_cache_->Set(
+        cert_handle,
+        base::Bind(
+            &MockCertCache::FinishSet, weak_factory_.GetWeakPtr(), &key, cb));
+  }
+
+  void Get(std::string key,

[wtc, 2014/06/21 00:43:13]

This should be

  const std::string& key,

Our Style Guide recommends input parameters should be passed as const
references.

+           X509Certificate::OSCertHandle& cert_handle,
+           CompletionCallback cb) {

[wtc, 2014/06/21 00:43:13]

This probably should also be a const reference.

+    cert_cache_->Get(key,
+                     base::Bind(&MockCertCache::FinishGet,
+                                weak_factory_.GetWeakPtr(),
+                                &cert_handle,
+                                cb));
+  }
+
+  void FinishSet(std::string* key_return,
+                 CompletionCallback cb,
+                 const std::string& key_received) {
+    *key_return = key_received;
+    base::ResetAndReturn(&cb).Run(OK);
+  }
+
+  void FinishGet(X509Certificate::OSCertHandle* handle_return,
+                 CompletionCallback cb,
+                 const X509Certificate::OSCertHandle handle_retrieved) {
+    *handle_return = handle_retrieved;
+    base::ResetAndReturn(&cb).Run(OK);
+  }
+
+  void DeleteCertCache() { cert_cache_.reset(); }
+
+ private:
+  scoped_ptr<disk_cache::Backend> backend;
+  scoped_ptr<DiskBasedCertCache> cert_cache_;
+  base::WeakPtrFactory<MockCertCache> weak_factory_;
+};
+
+// ----------------------------------------------------------------------------

[wtc, 2014/06/21 00:43:13]

Move this horizontal separator line after the "} // namespace" line.

+
+}  // namespace
+
+// Tests that a certificate can be stored in the cache.
+TEST(DiskBasedCertCache, SetCert) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+
+  EXPECT_TRUE(cert.get());
+
+  TestCompletionCallback set_callback;
+
+  std::string key;
+
+  user.Set(cert.get()->os_cert_handle(), key, set_callback.callback());
+  set_callback.WaitForResult();
+
+  ASSERT_TRUE(!key.empty());
+}
+
+// Tests that attempting to retrieve a cert that is not in the cache will
+// return NULL.
+TEST(DiskBasedCertCache, GetUncachedCert) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+
+  TestCompletionCallback get_callback;
+
+  X509Certificate::OSCertHandle cert_handle = NULL;
+  user.Get("cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4",
+           cert_handle,
+           get_callback.callback());
+  get_callback.WaitForResult();
+
+  ASSERT_EQ(NULL, cert_handle);
+}
+
+// Tests that the same certificate can be requested to be stored from multiple
+// locations simultaneously.
+TEST(DiskBasedCertCache, SetMultiple) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+
+  TestCompletionCallback set_callback1;
+  TestCompletionCallback set_callback2;
+
+  std::string key;
+
+  user.Set(cert.get()->os_cert_handle(), key, set_callback1.callback());
+  user.Set(cert.get()->os_cert_handle(), key, set_callback2.callback());
+  set_callback1.WaitForResult();
+  set_callback2.WaitForResult();
+}
+
+// Stores a certificate in DiskBasedCertCache, then retrieves it
+// and makes sure it was retrieved successfully.
+TEST(DiskBasedCertCache, SimpleSetAndGet) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+
+  EXPECT_TRUE(cert.get());
+
+  TestCompletionCallback set_callback;
+  TestCompletionCallback get_callback;
+
+  std::string key;
+  X509Certificate::OSCertHandle retrieved_cert_handle = NULL;
+
+  user.Set(cert.get()->os_cert_handle(), key, set_callback.callback());
+  set_callback.WaitForResult();
+
+  user.Get(key, retrieved_cert_handle, get_callback.callback());
+  get_callback.WaitForResult();
+
+  ASSERT_TRUE(X509Certificate::IsSameOSCert(retrieved_cert_handle,
+                                            cert.get()->os_cert_handle()));
+}
+
+// Test some basic usage with multiple certificates being stored and retrieved
+// at the same time.
+TEST(DiskBasedCertCache, BasicUsage) {
+  AddMockTransaction(&kCertTransaction1);
+  AddMockTransaction(&kCertTransaction2);
+
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert1(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+
+  scoped_refptr<X509Certificate> cert2(
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+
+  EXPECT_TRUE(cert1.get());
+  EXPECT_TRUE(cert2.get());
+  EXPECT_TRUE(!X509Certificate::IsSameOSCert(cert1->os_cert_handle(),
+                                             cert2->os_cert_handle()));
+
+  TestCompletionCallback set_callback1, set_callback2;
+  std::string key1, key2;
+
+  user.Set(cert1.get()->os_cert_handle(), key1, set_callback1.callback());
+  user.Set(cert2.get()->os_cert_handle(), key2, set_callback2.callback());
+  set_callback1.WaitForResult();
+  set_callback2.WaitForResult();
+
+  TestCompletionCallback get_callback1, get_callback2;
+  X509Certificate::OSCertHandle cert_handle1, cert_handle2;
+
+  user.Get(key2, cert_handle2, get_callback2.callback());
+  user.Get(key1, cert_handle1, get_callback1.callback());
+  get_callback2.WaitForResult();
+  get_callback1.WaitForResult();
+
+  ASSERT_TRUE(
+      X509Certificate::IsSameOSCert(cert1->os_cert_handle(), cert_handle1));
+  ASSERT_TRUE(
+      X509Certificate::IsSameOSCert(cert2->os_cert_handle(), cert_handle2));
+}
+
+// Tests result of if a certificate is simultaneously asked to be stored and
+// retrieved
+// from the cache.
+// TODO(brandonsalmon): improve the functionality of this circumstance.
+TEST(DiskBasedCertCache, SimultaneousSetGet) {
+  AddMockTransaction(&kCertTransaction1);
+
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+
+  TestCompletionCallback set_callback, get_callback;
+  X509Certificate::OSCertHandle cert_handle;
+  std::string key("cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4");
+
+  user.Set(cert.get()->os_cert_handle(), key, set_callback.callback());
+  user.Get(key, cert_handle, get_callback.callback());
+  get_callback.WaitForResult();
+  set_callback.WaitForResult();
+}
+
+// Tests whether or not an operation will be correctly canceled in the
+// circumstance of the DiskBasedCertCache being deleted early.
+TEST(DiskBasedCertCache, DeletedCertCache) {
+  AddMockTransaction(&kCertTransaction1);
+
+  MockCertCache user;
+
+  std::string key;
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+
+  TestCompletionCallback set_callback;
+  user.Set(cert.get()->os_cert_handle(), key, set_callback.callback());
+
+  user.DeleteCertCache();
+  set_callback.WaitForResult();
+  ASSERT_EQ(key, std::string());
+}
+
+}  // namespace net