Remove the test-only X509Certificate constructor

net/ssl/ssl_client_auth_cache_unittest.cc

 // Copyright (c) 2009 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/ssl/ssl_client_auth_cache.h"
 
 #include "base/macros.h"
 #include "base/time/time.h"
+#include "net/base/test_data_directory.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_private_key.h"
+#include "net/test/cert_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
 class MockSSLPrivateKey : public SSLPrivateKey {
  public:
   MockSSLPrivateKey() {}
 
   Type GetType() override { return Type::RSA; }
 
@@ skipping 15 matching lines @@
 
  private:
   ~MockSSLPrivateKey() override {}
 
   DISALLOW_COPY_AND_ASSIGN(MockSSLPrivateKey);
 };
 
 TEST(SSLClientAuthCacheTest, LookupAddRemove) {
   SSLClientAuthCache cache;
 
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
-
   HostPortPair server1("foo1", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo1", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
   HostPortPair server2("foo2", 443);
   scoped_refptr<X509Certificate> cert2(
-      new X509Certificate("foo2", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));
+  ASSERT_TRUE(cert2);
 
   HostPortPair server3("foo3", 443);
   scoped_refptr<X509Certificate> cert3(
-    new X509Certificate("foo3", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert3);
 
   scoped_refptr<X509Certificate> cached_cert;
   scoped_refptr<SSLPrivateKey> cached_pkey;
   // Lookup non-existent client certificate.
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
 
   // Add client certificate for server1.
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1, cached_cert);
 
   // Add client certificate for server2.
   cache.Add(server2, cert2.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1.get(), cached_cert.get());
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 
   // Overwrite the client certificate for server1.
   cache.Add(server1, cert3.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert3, cached_cert);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 
   // Remove client certificate of server1.
   cache.Remove(server1);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 
   // Remove non-existent client certificate.
   cache.Remove(server1);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 }
 
 // Check that if the server differs only by port number, it is considered
 // a separate server.
 TEST(SSLClientAuthCacheTest, LookupWithPort) {
   SSLClientAuthCache cache;
 
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
-
   HostPortPair server1("foo", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
   HostPortPair server2("foo", 8443);
   scoped_refptr<X509Certificate> cert2(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));

[eroman, 2016/06/22 16:51:08]

Any reason for changing the test pattern here? (i.e. re-import "ok_cert.pem"
here).
Looks like all the test cares about is that cert1 and cert2 be different
pointers, not data.

I don't really care either way, just reviewing the intent of this change.

[Ryan Sleevi, 2016/06/22 17:31:45]

I'm not sure I follow. Were you suggesting to reimport ok_cert.pem? Not sure why
the comment here vs say line 55-61

If I reused ok_cert.pem, I would have had to sprinkle in assertions that
cert1.get() != cert2.get() and that seemed unnecessary to add new preconditions.

[eroman, 2016/06/22 17:38:21]

55-61 isn't changing the code pattern, whereas this is.

[Ryan Sleevi, 2016/06/22 20:27:33]

I suspect I'm not understanding what you mean by "changing the code pattern". As
best I can understand, you're talking about cert2 having the same input
parameters ("foo", "CA", etc) with the expectation of returning a different
cert, and assuming that was somehow intentional or meaningful. Not really, no -
it was intended that they be 'different' certs (and X509Certificate() has always
obeyed that, as is required by a ctor). However,
X509Certificate::CreateFrom(...) has historically not obeyed that, and that the
other constructor was intentionally private because of this.

That is to say, this change reflects exactly what the test was trying to do and
testing, even if it didn't change "foo" to "foo2", so no, it's not a change in
code pattern.

+  ASSERT_TRUE(cert2);
 
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
   cache.Add(server2, cert2.get(), new MockSSLPrivateKey);
 
   scoped_refptr<X509Certificate> cached_cert;
   scoped_refptr<SSLPrivateKey> cached_pkey;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1.get(), cached_cert.get());
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2.get(), cached_cert.get());
 }
 
-// Check that the a NULL certificate, indicating the user has declined to send
-// a certificate, is properly cached.
+// Check that the a nullptr certificate, indicating the user has declined to
+// send a certificate, is properly cached.
 TEST(SSLClientAuthCacheTest, LookupNullPreference) {
   SSLClientAuthCache cache;
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
 
   HostPortPair server1("foo", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
-  cache.Add(server1, NULL, new MockSSLPrivateKey);
+  cache.Add(server1, nullptr, new MockSSLPrivateKey);
 
   scoped_refptr<X509Certificate> cached_cert(cert1);
   scoped_refptr<SSLPrivateKey> cached_pkey;
-  // Make sure that |cached_cert| is updated to NULL, indicating the user
+  // Make sure that |cached_cert| is updated to nullptr, indicating the user
   // declined to send a certificate to |server1|.
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  EXPECT_EQ(NULL, cached_cert.get());
+  EXPECT_EQ(nullptr, cached_cert.get());
 
   // Remove the existing cached certificate.
   cache.Remove(server1);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
 
   // Add a new preference for a specific certificate.
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1, cached_cert);
 
-  // Replace the specific preference with a NULL certificate.
-  cache.Add(server1, NULL, new MockSSLPrivateKey);
-  cached_cert = NULL;
+  // Replace the specific preference with a nullptr certificate.
+  cache.Add(server1, nullptr, new MockSSLPrivateKey);
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  EXPECT_EQ(NULL, cached_cert.get());
+  EXPECT_EQ(nullptr, cached_cert.get());
 }
 
 // Check that the OnCertAdded() method removes all cache entries.
 TEST(SSLClientAuthCacheTest, OnCertAdded) {
   SSLClientAuthCache cache;
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
 
   HostPortPair server1("foo", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
 
   HostPortPair server2("foo2", 443);
-  cache.Add(server2, NULL, new MockSSLPrivateKey);
+  cache.Add(server2, nullptr, new MockSSLPrivateKey);
 
   scoped_refptr<X509Certificate> cached_cert;
   scoped_refptr<SSLPrivateKey> cached_pkey;
 
   // Demonstrate the set up is correct.
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1, cached_cert);
 
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
-  EXPECT_EQ(NULL, cached_cert.get());
+  EXPECT_EQ(nullptr, cached_cert.get());
 
-  cache.OnCertAdded(NULL);
+  cache.OnCertAdded(nullptr);
 
   // Check that we no longer have entries for either server.
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_FALSE(cache.Lookup(server2, &cached_cert, &cached_pkey));
 }
 
 }  // namespace net