Separate cert loading code from CertLibrary and move to src/chromeos

chromeos/network/cert_loader.h

Index: chromeos/network/cert_loader.h
diff --git a/chromeos/network/cert_loader.h b/chromeos/network/cert_loader.h
new file mode 100644
index 0000000000000000000000000000000000000000..6b79fa58b92e1100cf83562cc34da11a044980ee
--- /dev/null
+++ b/chromeos/network/cert_loader.h
@@ -0,0 +1,136 @@
+// Copyright (c) 2013 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.
+
+#ifndef CHROMEOS_NETWORK_CERT_LOADER_H_
+#define CHROMEOS_NETWORK_CERT_LOADER_H_
+
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list_threadsafe.h"
+#include "base/threading/thread_checker.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "chromeos/login/login_state.h"
+#include "net/cert/cert_database.h"
+#include "net/cert/x509_certificate.h"
+
+namespace crypto {
+class SymmetricKey;
+}
+
+namespace chromeos {
+
+// This class is responsible for initializing the TPM token and loading
+// certificates once the TPM is initialized. It is expected to be constructed
+// on the UI thread and public methods should all be called from the UI thread.
+// When certificates have been loaded (after login completes), or the cert
+// database changes, observers are called with OnCertificatesLoaded().
+class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
+                                   public LoginState::Observer {
+ public:
+  class Observer {
+   public:
+    virtual ~Observer() {}
+
+    // Called when the certificates, passed for convenience as |cert_list|,
+    // have completed loading. |initial_load| is true the first time this
+    // is called.
+    virtual void OnCertificatesLoaded(const net::CertificateList& cert_list,
+                                      bool initial_load) = 0;
+
+   protected:
+    Observer() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
+
+  // Manage the global instance.
+  static void Initialize();
+  static void Shutdown();
+  static CertLoader* Get();
+
+  void AddObserver(CertLoader::Observer* observer);
+  void RemoveObserver(CertLoader::Observer* observer);
+
+  // Call this to start the certificate list initialization process.

[pneubeck (no reviews), 2013/05/02 14:58:35]

remove the return value. It's confusing and never used.

Please decide if it's safe to call this if !IsUserLoggedIn or not.
If it's safe: remove all conditionals on the calling side.
Otherwise, document it here and log an error if the precondition is violated.

[stevenjb, 2013/05/03 01:00:31]

Done. It's actually only called within the class now; made private.

+  bool RequestCertificates();
+
+  // Returns true when the certificate list has been requested but not loaded.
+  bool CertificatesLoading() const;
+
+  // Returns true if the TPM is available for hardware-backed certificates.
+  bool IsHardwareBacked() const;
+
+  bool certificates_loaded() const { return certificates_loaded_; }

[pneubeck (no reviews), 2013/05/02 14:58:35]

comment?

[stevenjb, 2013/05/03 01:00:31]

Unnecessary for getters, esp. with obvious names.

+
+  const std::string& tpm_token_name() const { return tpm_token_name_; }

[pneubeck (no reviews), 2013/05/02 14:58:35]

only valid if IsHaredwareBacked() == true.

This cached name doesn't have to be public. Callers (CertLibrary only) can use
crypto::GetTPMTokenInfo instead or the other way round.

[stevenjb, 2013/05/03 01:00:31]

The network code will be using these also (I added slot as well). There may be
some cleanup to be done here, but for now I'd like this all in one place.

+
+  const net::CertificateList& cert_list() const { return cert_list_; }

[pneubeck (no reviews), 2013/05/02 14:58:35]

only valid if certificates_loaded() == true

[stevenjb, 2013/05/03 01:00:31]

Done.

[stevenjb, 2013/05/03 01:00:31]

Done.

+
+ private:
+  typedef ObserverListThreadSafe<CertLoader::Observer> ObserverList;

[pneubeck (no reviews), 2013/05/02 14:58:35]

I don't find a reason why this has to be thread safe.

[stevenjb, 2013/05/03 01:00:31]

Done.

+
+  CertLoader();
+  virtual ~CertLoader();
+
+  void OnTpmIsEnabled(DBusMethodCallStatus call_status,
+                      bool tpm_is_enabled);
+  void OnPkcs11IsTpmTokenReady(DBusMethodCallStatus call_status,
+                               bool is_tpm_token_ready);
+  void OnPkcs11GetTpmTokenInfo(DBusMethodCallStatus call_status,
+                               const std::string& token_name,
+                               const std::string& user_pin);
+  void InitializeTPMToken();
+  void StartLoadCertificates();
+  void UpdateCertificates(net::CertificateList* cert_list);
+  void MaybeRetryRequestCertificates();
+  void RequestCertificatesTask();
+
+  void NotifyCertificatesLoaded(bool initial_load);
+
+  // net::CertDatabase::Observer
+  virtual void OnCertTrustChanged(const net::X509Certificate* cert) OVERRIDE;
+  virtual void OnCertAdded(const net::X509Certificate* cert) OVERRIDE;
+  virtual void OnCertRemoved(const net::X509Certificate* cert) OVERRIDE;
+
+  // LoginState::Observer
+  virtual void LoggedInStateChanged(LoginState::LoggedInState state) OVERRIDE;
+
+  const scoped_refptr<ObserverList> observer_list_;
+
+  // Active request task for re-requests while waiting for TPM init.
+  base::Closure request_task_;
+
+  // Local state.
+  bool tpm_token_ready_;
+  bool certificates_requested_;
+  bool certificates_loaded_;
+  // The key store for the current user has been loaded. This flag is needed to
+  // ensure that the key store will not be loaded twice in the policy recovery
+  // "safe-mode".
+  bool key_store_loaded_;
+
+  // Cached TPM token name.
+  std::string tpm_token_name_;
+
+  // Cached TPM user pin.
+  std::string tpm_user_pin_;
+
+  // Cached Certificates.
+  net::CertificateList cert_list_;
+
+  base::ThreadChecker thread_checker_;
+
+  base::WeakPtrFactory<CertLoader> weak_ptr_factory_;

[pneubeck (no reviews), 2013/05/02 14:58:35]

There are two entry points to callback sequences:
The loop
RequestCerts -> ... -> ... -> MaybeRetry.. -> [RequestCerts]

and
StartLoadCets -> ... -> UpdateCerts

To ensure that these sequences are not accidentally kicked off multiple times,
I'd suggest to have one WeakPtrFactory for each sequence and:
- In each of the functions that are part of a sequence:
DCHECK(!weak_ptr_factory_.HasWeakPtrs());
- Wrap each callback with a static function that resets() the weak ptr before
running the original callback. So that the DCHECK doesn't trigger.

[stevenjb, 2013/05/03 01:00:31]

Added a TODO

+
+  DISALLOW_COPY_AND_ASSIGN(CertLoader);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_CERT_LOADER_H_