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..d9c2912bfa27148bc2d45527f4eeacd2bf924959
--- /dev/null
+++ b/net/http/disk_based_cert_cache_unittest.cc
@@ -0,0 +1,277 @@
+// 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/23 18:38:40]

Nit: add a blank line after the header #include statements.

+namespace net {
+
+namespace {
+
+// MockTransactions are required to use the MockDiskCache backend.
+
+// This transaction corresponds to "root_ca_cert.pem" in
+// GetTestCertsDirectory().
+const MockTransaction kCertTransaction1 = {
+  "cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4",
+  "",
+  base::Time(),
+  "",
+  LOAD_NORMAL,
+  "",
+  "",
+  base::Time(),
+  "",
+  TEST_MODE_NORMAL,
+  NULL,
+  0

[wtc, 2014/06/23 18:38:40]

I suggest adding an initializer for the final field "net::Error return_code".
Use "OK" if that field is never used as a byte count.

+};
+
+// 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
+// recieved using CompletionCallback::WaitForResult.

[wtc, 2014/06/23 18:38:40]

Nit: recieved => received

+class MockCertCache {
+ public:
+  MockCertCache()
+      : backend(new MockDiskCache()),
+        cert_cache_(new DiskBasedCertCache(backend.get())),
+        weak_factory_(this) {}
+
+  void Set(X509Certificate::OSCertHandle cert_handle,
+           std::string* key,
+           const CompletionCallback& callback) {
+    cert_cache_->Set(cert_handle,
+                     base::Bind(&MockCertCache::FinishSet,

[wtc, 2014/06/23 18:38:40]

Nit: our naming convention for this kind of function seems to be "DidSet" or
"OnSetComplete".

+                                weak_factory_.GetWeakPtr(),
+                                key,
+                                callback));
+  }
+
+  void Get(const std::string& key,
+           X509Certificate::OSCertHandle* cert_handle,
+           const CompletionCallback& callback) {
+    cert_cache_->Get(key,
+                     base::Bind(&MockCertCache::FinishGet,
+                                weak_factory_.GetWeakPtr(),
+                                cert_handle,
+                                callback));
+  }
+
+  void FinishSet(std::string* key_return,
+                 CompletionCallback callback,
+                 const std::string& key_received) {
+    *key_return = key_received;
+    base::ResetAndReturn(&callback).Run(OK);

[wtc, 2014/06/23 18:38:40]

It's not necessary to use ResetAndReturn. You can just do
  callback.Run(OK);

+  }
+
+  void FinishGet(X509Certificate::OSCertHandle* handle_return,
+                 CompletionCallback callback,
+                 const X509Certificate::OSCertHandle handle_retrieved) {

[wtc, 2014/06/23 18:38:40]

Be consistent in using "xxx_received" or "xxx_retrieved" for the last parameter
of your FinishXXX functions.

+    *handle_return = handle_retrieved;
+    base::ResetAndReturn(&callback).Run(OK);
+  }
+
+  void DeleteCertCache() { cert_cache_.reset(); }
+
+ private:
+  scoped_ptr<disk_cache::Backend> backend;
+  scoped_ptr<DiskBasedCertCache> cert_cache_;
+  base::WeakPtrFactory<MockCertCache> weak_factory_;
+};
+
+}  // 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());

[wtc, 2014/06/23 18:38:40]

Use ASSERT_TRUE instead of EXPECT_TRUE here, because if |cert| is null, the
subsequent code that dereferences |cert| will crash.

+
+  TestCompletionCallback set_callback;
+
+  std::string key;
+
+  user.Set(cert.get()->os_cert_handle(), &key, set_callback.callback());
+  set_callback.WaitForResult();

[wtc, 2014/06/23 18:38:40]

You should check that this returns OK.

+
+  ASSERT_TRUE(!key.empty());

[wtc, 2014/06/23 18:38:40]

You should check with EXPECT_EQ that |key| has the right value. Or do you
consider that as an implementation detail?

+}
+
+// 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();

[wtc, 2014/06/23 18:38:40]

You should check that this returns OK.

+
+  ASSERT_EQ(NULL, cert_handle);

[wtc, 2014/06/23 18:38:40]

Nit: you can use EXPECT_EQ here.

+}
+
+// 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;

[wtc, 2014/06/23 18:38:40]

Use |key1| and |key2|, and pass &key1 and key2 to the two user.Set() calls. I
think the expected result is EXPECT_EQ(key1, key2)?

+
+  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(),

[wtc, 2014/06/23 18:38:40]

If there is EXPECT_FALSE, use it.

+                                             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(

[wtc, 2014/06/23 18:38:40]

Use EXPECT_TRUE.

+      X509Certificate::IsSameOSCert(cert2->os_cert_handle(), cert_handle2));
+}
+
+// Tests result of if a certificate is simultaneously asked to be stored and

[wtc, 2014/06/23 18:38:40]

Nit: the first few words of this comment don't read well...

+// retrieved
+// from the cache.

[wtc, 2014/06/23 18:38:40]

Nit: move to the previous line.

+// 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();

[wtc, 2014/06/23 18:38:39]

What is the expected result?

+}
+
+// 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());

[wtc, 2014/06/23 18:38:40]

1. We should verify that the expected result of |key| does not depend on the
timing.

2. Use EXPECT_EQ.

3. The two arguments of EXPECT_EQ are 'expected' and 'actual', so this should be
  EXPECT_EQ(std::string(), key);

+}
+
+}  // namespace net