CertDatabase cleanups: comments for CertType and trust enums, s/ImportCertResult/ImportCertFailure/

BUG=19991
TEST=none

Review URL: http://codereview.chromium.org/3351002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59318 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 42 insertions, 27 deletions

diff --git a/net/base/cert_database.cc b/net/base/cert_database.cc
index 3f6f9e9..961638c 100644
--- a/net/base/cert_database.cc
+++ b/net/base/cert_database.cc
@@ -8,7 +8,7 @@
 
 namespace net {
 
-CertDatabase::ImportCertResult::ImportCertResult(
+CertDatabase::ImportCertFailure::ImportCertFailure(
     X509Certificate* cert, int err)
     : certificate(cert), net_error(err) {
 }
diff --git a/net/base/cert_database.h b/net/base/cert_database.h
index dab72eb..52888fe 100644
--- a/net/base/cert_database.h
+++ b/net/base/cert_database.h
@@ -21,6 +21,12 @@ typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 // Constants to classify the type of a certificate.
 // This is only used in the context of CertDatabase, but is defined outside to
 // avoid an awkwardly long type name.
+// The type is a combination of intrinsic properties, such as the presense of an
+// email address or Certificate Authority Basic Constraint, and assigned trust
+// values.  For example, a cert with no email address, basic constraints, or
+// trust, would be classified as UNKNOWN_CERT.  If that cert is then trusted
+// with SetCertTrust(cert, SERVER_CERT, TRUSTED_SSL), it would become a
+// SERVER_CERT.
 enum CertType {
   UNKNOWN_CERT,
   CA_CERT,
@@ -40,6 +46,14 @@ enum CertType {
 class CertDatabase {
  public:
   // Constants that define which usages a certificate is trusted for.
+  // They are used in combination with CertType to specify trust for each type
+  // of certificate.
+  // For a CA_CERT, they specify that the CA is trusted for issuing server and
+  // client certs of each type.
+  // For SERVER_CERT, only TRUSTED_SSL makes sense, and specifies the cert is
+  // trusted as a server.
+  // For EMAIL_CERT, only TRUSTED_EMAIL makes sense, and specifies the cert is
+  // trusted for email.
   enum {
     UNTRUSTED        =      0,
     TRUSTED_SSL      = 1 << 0,
@@ -47,15 +61,15 @@ class CertDatabase {
     TRUSTED_OBJ_SIGN = 1 << 2,
   };
 
-  // Stores per-certificate import results.
-  struct ImportCertResult {
+  // Stores per-certificate error codes for import failures.
+  struct ImportCertFailure {
    public:
-    ImportCertResult(X509Certificate* cert, int err);
+    ImportCertFailure(X509Certificate* cert, int err);
 
     scoped_refptr<X509Certificate> certificate;
     int net_error;
   };
-  typedef std::vector<ImportCertResult> ImportCertResultList;
+  typedef std::vector<ImportCertFailure> ImportCertFailureList;
 
   CertDatabase();
 
@@ -99,7 +113,7 @@ class CertDatabase {
   // imported.
   bool ImportCACerts(const CertificateList& certificates,
                      unsigned int trust_bits,
-                     ImportCertResultList* not_imported);
+                     ImportCertFailureList* not_imported);
 
   // Set trust values for certificate.
   // Returns true on success or false on failure.
diff --git a/net/base/cert_database_nss.cc b/net/base/cert_database_nss.cc
index 5ce1389..18c7eba 100644
--- a/net/base/cert_database_nss.cc
+++ b/net/base/cert_database_nss.cc
@@ -142,7 +142,7 @@ X509Certificate* CertDatabase::FindRootInList(
 
 bool CertDatabase::ImportCACerts(const CertificateList& certificates,
                                  unsigned int trust_bits,
-                                 ImportCertResultList* not_imported) {
+                                 ImportCertFailureList* not_imported) {
   X509Certificate* root = FindRootInList(certificates);
   return psm::ImportCACerts(certificates, root, trust_bits, not_imported);
 }
diff --git a/net/base/cert_database_nss_unittest.cc b/net/base/cert_database_nss_unittest.cc
index 6aa7095..b115ac0 100644
--- a/net/base/cert_database_nss_unittest.cc
+++ b/net/base/cert_database_nss_unittest.cc
@@ -186,7 +186,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACert_SSLTrust) {
   EXPECT_FALSE(certs[0]->os_cert_handle()->isperm);
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(certs, CertDatabase::TRUSTED_SSL,
                                          &failed));
 
@@ -215,7 +215,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACert_EmailTrust) {
   EXPECT_FALSE(certs[0]->os_cert_handle()->isperm);
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(certs, CertDatabase::TRUSTED_EMAIL,
                                          &failed));
 
@@ -243,7 +243,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACert_ObjSignTrust) {
   EXPECT_FALSE(certs[0]->os_cert_handle()->isperm);
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(certs, CertDatabase::TRUSTED_OBJ_SIGN,
                                          &failed));
 
@@ -271,7 +271,7 @@ TEST_F(CertDatabaseNSSTest, ImportCA_NotCACert) {
   EXPECT_FALSE(certs[0]->os_cert_handle()->isperm);
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true,
             cert_db_.ImportCACerts(certs, CertDatabase::TRUSTED_SSL, &failed));
   ASSERT_EQ(1U, failed.size());
@@ -291,7 +291,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACertHierarchy) {
   ASSERT_TRUE(ReadCertIntoList("www_us_army_mil_cert.der", &certs));
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   // Have to specify email trust for the cert verification of the child cert to
   // work (see
   // http://mxr.mozilla.org/mozilla/source/security/nss/lib/certhigh/certvfy.c#752
@@ -315,7 +315,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACertHierarchyDupeRoot) {
   ASSERT_TRUE(ReadCertIntoList("dod_root_ca_2_cert.der", &certs));
 
   // First import just the root.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(
       certs, CertDatabase::TRUSTED_SSL | CertDatabase::TRUSTED_EMAIL,
       &failed));
@@ -353,7 +353,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACertHierarchyUntrusted) {
   ASSERT_TRUE(ReadCertIntoList("dod_ca_17_cert.der", &certs));
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(certs, CertDatabase::UNTRUSTED,
                                          &failed));
 
@@ -375,7 +375,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACertHierarchyTree) {
   ASSERT_TRUE(ReadCertIntoList("dod_ca_17_cert.der", &certs));
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(
       certs, CertDatabase::TRUSTED_SSL | CertDatabase::TRUSTED_EMAIL,
       &failed));
@@ -399,7 +399,7 @@ TEST_F(CertDatabaseNSSTest, ImportCACertNotHierarchy) {
   ASSERT_TRUE(ReadCertIntoList("dod_ca_17_cert.der", &certs));
 
   // Import it.
-  CertDatabase::ImportCertResultList failed;
+  CertDatabase::ImportCertFailureList failed;
   EXPECT_EQ(true, cert_db_.ImportCACerts(
       certs, CertDatabase::TRUSTED_SSL | CertDatabase::TRUSTED_EMAIL |
       CertDatabase::TRUSTED_OBJ_SIGN, &failed));
diff --git a/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp b/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
index 798b140..b32458d 100644
--- a/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
+++ b/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
@@ -55,7 +55,7 @@ namespace mozilla_security_manager {
 bool ImportCACerts(const net::CertificateList& certificates,
                    net::X509Certificate* root,
                    unsigned int trustBits,
-                   net::CertDatabase::ImportCertResultList* failed) {
+                   net::CertDatabase::ImportCertFailureList* not_imported) {
   base::ScopedPK11Slot slot(base::GetDefaultNSSKeySlot());
   if (!slot.get()) {
     LOG(ERROR) << "Couldn't get internal key slot!";
@@ -67,14 +67,14 @@ bool ImportCACerts(const net::CertificateList& certificates,
   // itself, so we skip it here.
 
   if (!CERT_IsCACert(root->os_cert_handle(), NULL)) {
-    failed->push_back(net::CertDatabase::ImportCertResult(
+    not_imported->push_back(net::CertDatabase::ImportCertFailure(
         root, net::ERR_IMPORT_CA_CERT_NOT_CA));
   } else if (root->os_cert_handle()->isperm) {
     // Mozilla just returns here, but we continue in case there are other certs
     // in the list which aren't already imported.
     // TODO(mattm): should we set/add trust if it differs from the present
     // settings?
-    failed->push_back(net::CertDatabase::ImportCertResult(
+    not_imported->push_back(net::CertDatabase::ImportCertFailure(
         root, net::ERR_IMPORT_CERT_ALREADY_EXISTS));
   } else {
     // Mozilla uses CERT_AddTempCertToPerm, however it is privately exported,
@@ -100,8 +100,9 @@ bool ImportCACerts(const net::CertificateList& certificates,
   // Import additional delivered certificates that can be verified.
   // This is sort of merged in from Mozilla's ImportValidCACertsInList.  Mozilla
   // uses CERT_FilterCertListByUsage to filter out non-ca certs, but we want to
-  // keep using X509Certificates, so that we can use them to build the |failed|
-  // result.  So, we keep using our net::CertificateList and filter it ourself.
+  // keep using X509Certificates, so that we can use them to build the
+  // |not_imported| result.  So, we keep using our net::CertificateList and
+  // filter it ourself.
   for (size_t i = 0; i < certificates.size(); i++) {
     const scoped_refptr<net::X509Certificate>& cert = certificates[i];
     if (cert == root) {
@@ -112,14 +113,14 @@ bool ImportCACerts(const net::CertificateList& certificates,
     // Mozilla uses CERT_FilterCertListByUsage(certList, certUsageAnyCA,
     // PR_TRUE).  Afaict, checking !CERT_IsCACert on each cert is equivalent.
     if (!CERT_IsCACert(cert->os_cert_handle(), NULL)) {
-      failed->push_back(net::CertDatabase::ImportCertResult(
+      not_imported->push_back(net::CertDatabase::ImportCertFailure(
           cert, net::ERR_IMPORT_CA_CERT_NOT_CA));
       LOG(INFO) << "skipping cert (non-ca)";
       continue;
     }
 
     if (cert->os_cert_handle()->isperm) {
-      failed->push_back(net::CertDatabase::ImportCertResult(
+      not_imported->push_back(net::CertDatabase::ImportCertFailure(
           cert, net::ERR_IMPORT_CERT_ALREADY_EXISTS));
       LOG(INFO) << "skipping cert (perm)";
       continue;
@@ -130,7 +131,7 @@ bool ImportCACerts(const net::CertificateList& certificates,
       // TODO(mattm): use better error code (map PORT_GetError to an appropriate
       // error value).  (maybe make MapSecurityError or MapCertErrorToCertStatus
       // public.)
-      failed->push_back(net::CertDatabase::ImportCertResult(
+      not_imported->push_back(net::CertDatabase::ImportCertFailure(
           cert, net::ERR_FAILED));
       LOG(INFO) << "skipping cert (verify) " << PORT_GetError();
       continue;
@@ -150,12 +151,12 @@ bool ImportCACerts(const net::CertificateList& certificates,
       LOG(ERROR) << "PK11_ImportCert failed with error " << PORT_GetError();
       // TODO(mattm): Should we bail or continue on error here?  Mozilla doesn't
       // check error code at all.
-      failed->push_back(net::CertDatabase::ImportCertResult(
+      not_imported->push_back(net::CertDatabase::ImportCertFailure(
           cert, net::ERR_IMPORT_CA_CERT_FAILED));
     }
   }
 
-  // Any errors importing individual certs will be in listed in |failed|.
+  // Any errors importing individual certs will be in listed in |not_imported|.
   return true;
 }
 
diff --git a/net/third_party/mozilla_security_manager/nsNSSCertificateDB.h b/net/third_party/mozilla_security_manager/nsNSSCertificateDB.h
index 199491f..58c1a2b 100644
--- a/net/third_party/mozilla_security_manager/nsNSSCertificateDB.h
+++ b/net/third_party/mozilla_security_manager/nsNSSCertificateDB.h
@@ -55,7 +55,7 @@ namespace mozilla_security_manager {
 bool ImportCACerts(const net::CertificateList& certificates,
                    net::X509Certificate* root,
                    unsigned int trustBits,
-                   net::CertDatabase::ImportCertResultList* failed);
+                   net::CertDatabase::ImportCertFailureList* not_imported);
 
 bool SetCertTrust(const net::X509Certificate* cert,
                   net::CertType type,