Cert Provisioning: Refresh certificate manager on updates.

Add Observer to net::CertDatabase in certificate_manager_model CertsSourcePlatformNSS to observe the changes in the CertDatabase and Refresh whenever there is a change in the database to solve the bug. Update the tests accordingly.

Bug: 1146607
Change-Id: I3b9ba7f64cf47333cb79430c6a060620290e8f30
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601748Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Irem Uğuz <iremuguz@google.com>
Cr-Commit-Position: refs/heads/master@{#843463}

chrome/browser/certificate_manager_model.cc

@@ -11,6 +11,7 @@
 #include "base/i18n/time_formatting.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observation.h"
 #include "base/sequence_checker.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -26,6 +27,7 @@
 #include "content/public/browser/resource_context.h"
 #include "crypto/nss_util.h"
 #include "net/base/net_errors.h"
+#include "net/cert/cert_database.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_util_nss.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -178,14 +180,24 @@ class CertificateManagerModel::CertsSource {
 
 namespace {
 // Provides certificates enumerable from a NSSCertDatabase.
-class CertsSourcePlatformNSS : public CertificateManagerModel::CertsSource {
+class CertsSourcePlatformNSS : public CertificateManagerModel::CertsSource,
+                               net::CertDatabase::Observer {
  public:
   CertsSourcePlatformNSS(base::RepeatingClosure certs_source_updated_callback,
                          net::NSSCertDatabase* nss_cert_database)
       : CertsSource(certs_source_updated_callback),
-        cert_db_(nss_cert_database) {}
+        cert_db_(nss_cert_database) {
+    // Observe CertDatabase changes to refresh when it's updated.
+    cert_database_observation_.Observe(net::CertDatabase::GetInstance());
+  }
   ~CertsSourcePlatformNSS() override = default;
 
+  // net::CertDatabase::Observer
+  void OnCertDBChanged() override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    Refresh();
+  }
+
   void Refresh() override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     SetHoldBackUpdates(true);
@@ -210,10 +222,7 @@ class CertsSourcePlatformNSS : public CertificateManagerModel::CertsSource {
 
   bool Delete(CERTCertificate* cert) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    bool result = cert_db_->DeleteCertAndKey(cert);
-    if (result)
-      Refresh();
-    return result;
+    return cert_db_->DeleteCertAndKey(cert);
   }
 
  private:
@@ -264,6 +273,10 @@ class CertsSourcePlatformNSS : public CertificateManagerModel::CertsSource {
   // The source NSSCertDatabase used for listing certificates.
   net::NSSCertDatabase* cert_db_;
 
+  // ScopedObservation to keep track of the observer for net::CertDatabase.
+  base::ScopedObservation<net::CertDatabase, net::CertDatabase::Observer>
+      cert_database_observation_{this};
+
   base::WeakPtrFactory<CertsSourcePlatformNSS> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(CertsSourcePlatformNSS);
@@ -615,29 +628,19 @@ int CertificateManagerModel::ImportFromPKCS12(PK11SlotInfo* slot_info,
                                               const std::string& data,
                                               const base::string16& password,
                                               bool is_extractable) {
-  int result = cert_db_->ImportFromPKCS12(slot_info, data, password,
-                                          is_extractable, nullptr);
-  if (result == net::OK)
-    Refresh();
-  return result;
+  return cert_db_->ImportFromPKCS12(slot_info, data, password, is_extractable,
+                                    nullptr);
 }
 
 int CertificateManagerModel::ImportUserCert(const std::string& data) {
-  int result = cert_db_->ImportUserCert(data);
-  if (result == net::OK)
-    Refresh();
-  return result;
+  return cert_db_->ImportUserCert(data);
 }
 
 bool CertificateManagerModel::ImportCACerts(
     const net::ScopedCERTCertificateList& certificates,
     net::NSSCertDatabase::TrustBits trust_bits,
     net::NSSCertDatabase::ImportCertFailureList* not_imported) {
-  const size_t num_certs = certificates.size();
-  bool result = cert_db_->ImportCACerts(certificates, trust_bits, not_imported);
-  if (result && not_imported->size() != num_certs)
-    Refresh();
-  return result;
+  return cert_db_->ImportCACerts(certificates, trust_bits, not_imported);
 }
 
 bool CertificateManagerModel::ImportServerCert(

chrome/browser/certificate_manager_model_unittest.cc

@@ -95,11 +95,14 @@ class CertificateManagerModelTest : public testing::Test {
   }
 
  protected:
-  // Invoke an explicit Refresh and wait until the observer has been notified.
-  void RefreshAndWait() {
+  // Invoke an explicit Refresh if the refresh is triggered and wait until the
+  // observer has been notified.
+  void WaitForRefresh(bool trigger_refresh) {
     base::RunLoop run_loop;
     fake_observer_->RunOnNextRefresh(run_loop.QuitClosure());
-    certificate_manager_model_->Refresh();
+    if (trigger_refresh) {
+      certificate_manager_model_->Refresh();
+    }
     run_loop.Run();
   }
 
@@ -132,7 +135,7 @@ TEST_F(CertificateManagerModelTest, ListsCertsFromPlatform) {
   ASSERT_EQ(SECSuccess,
             PK11_ImportCert(test_nssdb_.slot(), cert.get(), CK_INVALID_HANDLE,
                             "cert", PR_FALSE /* includeTrust (unused) */));
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   {
     CertificateManagerModel::OrgGroupingMap org_grouping_map;
@@ -156,7 +159,9 @@ TEST_F(CertificateManagerModelTest, ListsCertsFromPlatform) {
 
   certificate_manager_model_->SetCertTrust(cert.get(), net::CertType::CA_CERT,
                                            net::NSSCertDatabase::TRUSTED_SSL);
-  RefreshAndWait();
+  // Wait for refresh without triggering because observer should be notified by
+  // net::CertDatabase and refresh automatically.
+  WaitForRefresh(false /*tigger_for_refresh*/);
   {
     CertificateManagerModel::OrgGroupingMap org_grouping_map;
     certificate_manager_model_->FilterAndBuildOrgGroupingMap(
@@ -178,7 +183,7 @@ TEST_F(CertificateManagerModelTest, ListsClientCertsFromPlatform) {
       net::GetTestCertsDirectory(), "client_1.pem", "client_1.pk8",
       test_nssdb_.slot(), &platform_client_cert);
 
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   CertificateManagerModel::OrgGroupingMap org_grouping_map;
   certificate_manager_model_->FilterAndBuildOrgGroupingMap(
@@ -418,7 +423,7 @@ TEST_F(CertificateManagerModelChromeOSTest,
   ASSERT_TRUE(policy_cert.get());
   policy_certs_provider_.SetPolicyProvidedCertificates({policy_cert}, {});
 
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   {
     CertificateManagerModel::OrgGroupingMap org_grouping_map;
@@ -486,7 +491,7 @@ TEST_F(CertificateManagerModelChromeOSTest,
   ASSERT_TRUE(policy_cert.get());
   policy_certs_provider_.SetPolicyProvidedCertificates({}, {policy_cert});
 
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   {
     CertificateManagerModel::OrgGroupingMap org_grouping_map;
@@ -557,7 +562,7 @@ TEST_F(CertificateManagerModelChromeOSTest,
   ASSERT_TRUE(policy_cert.get());
   policy_certs_provider_.SetPolicyProvidedCertificates({policy_cert}, {});
 
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   CertificateManagerModel::OrgGroupingMap org_grouping_map;
   certificate_manager_model_->FilterAndBuildOrgGroupingMap(
@@ -579,7 +584,7 @@ TEST_F(CertificateManagerModelChromeOSTest, ListsExtensionCerts) {
   ASSERT_TRUE(extension_cert.get());
   extension_client_certs_.push_back(extension_cert);
 
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   CertificateManagerModel::OrgGroupingMap org_grouping_map;
   certificate_manager_model_->FilterAndBuildOrgGroupingMap(
@@ -610,7 +615,7 @@ TEST_F(CertificateManagerModelChromeOSTest,
   ASSERT_TRUE(extension_cert.get());
   extension_client_certs_.push_back(extension_cert);
 
-  RefreshAndWait();
+  WaitForRefresh(true /*tigger_for_refresh*/);
 
   {
     CertificateManagerModel::OrgGroupingMap org_grouping_map;