settings: Add async system salt retrieval logic in DeviceOAuth2TokenServiceFactory

Along the way, remove GetCachedSystemSalt() from SystemSaltGetter.

BUG=309959, 306547
TEST=none
R=hashimoto@chromium.org, pastarmovj@chromium.org

Review URL: https://codereview.chromium.org/39443002

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

5 files changed, 269 insertions, 52 deletions

diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
index 5f7fb30..253a833 100644
--- a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
+++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
@@ -4,11 +4,14 @@
 
 #include "chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h"
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/message_loop/message_loop.h"
 #include "base/tracked_objects.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/settings/device_oauth2_token_service.h"
 #include "chrome/browser/chromeos/settings/token_encryptor.h"
+#include "chromeos/cryptohome/system_salt_getter.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace chromeos {
@@ -19,10 +22,9 @@ DeviceOAuth2TokenServiceFactory* g_factory = NULL;
 }  // namespace
 
 DeviceOAuth2TokenServiceFactory::DeviceOAuth2TokenServiceFactory()
-    : token_service_(new DeviceOAuth2TokenService(
-        g_browser_process->system_request_context(),
-        g_browser_process->local_state(),
-        new CryptohomeTokenEncryptor)) {
+    : initialized_(false),
+      token_service_(NULL),
+      weak_ptr_factory_(this) {
 }
 
 DeviceOAuth2TokenServiceFactory::~DeviceOAuth2TokenServiceFactory() {
@@ -33,30 +35,15 @@ DeviceOAuth2TokenServiceFactory::~DeviceOAuth2TokenServiceFactory() {
 void DeviceOAuth2TokenServiceFactory::Get(const GetCallback& callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  DeviceOAuth2TokenService* token_service = NULL;
-  if (g_factory)
-    token_service = g_factory->token_service_;
-
-  // TODO(satorux): Implement async initialization logic for
-  // DeviceOAuth2TokenService. crbug.com/309959.
-  // Here's how that should work:
-  //
-  // if token_service is ready:
-  //   run callback asynchronously via MessageLoop
-  //   return
-  //
-  // add callback to the pending callback list
-  //
-  // if there is only one pending callback:
-  //   start getting the system salt asynchronously...
-  //
-  // upon receiving the system salt:
-  //   create CryptohomeTokenEncryptor with that key
-  //   create DeviceOAuth2TokenService
-  //   run all the pending callbacks
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(callback, token_service));
+  if (!g_factory) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback,
+                   static_cast<DeviceOAuth2TokenService*>(NULL)));
+    return;
+  }
+
+  g_factory->RunAsync(callback);
 }
 
 // static
@@ -65,6 +52,7 @@ void DeviceOAuth2TokenServiceFactory::Initialize() {
 
   DCHECK(!g_factory);
   g_factory = new DeviceOAuth2TokenServiceFactory;
+  g_factory->CreateTokenService();
 }
 
 // static
@@ -77,4 +65,53 @@ void DeviceOAuth2TokenServiceFactory::Shutdown() {
   }
 }
 
+void DeviceOAuth2TokenServiceFactory::CreateTokenService() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  SystemSaltGetter::Get()->GetSystemSalt(
+      base::Bind(&DeviceOAuth2TokenServiceFactory::DidGetSystemSalt,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DeviceOAuth2TokenServiceFactory::DidGetSystemSalt(
+    const std::string& system_salt) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(!token_service_);
+
+  if (system_salt.empty()) {
+    LOG(ERROR) << "Failed to get the system salt";
+  } else {
+    token_service_= new DeviceOAuth2TokenService(
+        g_browser_process->system_request_context(),
+        g_browser_process->local_state(),
+        new CryptohomeTokenEncryptor(system_salt));
+  }
+  // Mark that the factory is initialized.
+  initialized_ = true;
+
+  // Run callbacks regardless of whether token_service_ is created or not,
+  // but don't run callbacks immediately. Each callback would cause an
+  // interesting action, hence running them consecutively could be
+  // potentially expensive and dangerous.
+  while (!pending_callbacks_.empty()) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(pending_callbacks_.front(), token_service_));
+    pending_callbacks_.pop();
+  }
+}
+
+void DeviceOAuth2TokenServiceFactory::RunAsync(const GetCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (initialized_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, token_service_));
+    return;
+  }
+
+  pending_callbacks_.push(callback);
+}
+
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
index 16bd680..cd68763 100644
--- a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
+++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
@@ -5,8 +5,12 @@
 #ifndef CHROME_BROWSER_CHROMEOS_SETTINGS_DEVICE_OAUTH2_TOKEN_SERVICE_FACTORY_H_
 #define CHROME_BROWSER_CHROMEOS_SETTINGS_DEVICE_OAUTH2_TOKEN_SERVICE_FACTORY_H_
 
+#include <queue>
+#include <string>
+
 #include "base/basictypes.h"
 #include "base/callback_forward.h"
+#include "base/memory/weak_ptr.h"
 
 namespace chromeos {
 
@@ -45,7 +49,28 @@ class DeviceOAuth2TokenServiceFactory {
   DeviceOAuth2TokenServiceFactory();
   ~DeviceOAuth2TokenServiceFactory();
 
+  // Creates the token service asynchronously in the following steps:
+  // 1) Get the system salt from cryptohomed asynchronously
+  // 2) Create CryptohomeTokenEncryptor using the system salt
+  // 3) Create DeviceOAuth2TokenServiceFactory using the token encryptor
+  void CreateTokenService();
+
+  // Continuation of CreateTokenService(). Called when GetSystemSalt() is
+  // complete.
+  void DidGetSystemSalt(const std::string& system_salt);
+
+  // Runs the callback asynchronously. If |token_service_| is ready, the
+  // callback will be simply run via MessageLoop. Otherwise, the callback
+  // will be queued in |pending_callbacks_| and run when |token_service_| is
+  // ready.
+  void RunAsync(const GetCallback& callback);
+
+  // True if the factory is initialized (i.e. system salt retrieval is done
+  // regardless of whether it succeeded or failed).
+  bool initialized_;
   DeviceOAuth2TokenService* token_service_;
+  std::queue<GetCallback> pending_callbacks_;
+  base::WeakPtrFactory<DeviceOAuth2TokenServiceFactory> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceOAuth2TokenServiceFactory);
 };
diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc
new file mode 100644
index 0000000..e7d5f32
--- /dev/null
+++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc
@@ -0,0 +1,165 @@
+// Copyright 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.
+
+#include "chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "chromeos/cryptohome/system_salt_getter.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_cryptohome_client.h"
+#include "chromeos/dbus/fake_dbus_thread_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace {
+
+// Copies the token service and increments the counter.
+void CopyTokenServiceAndCount(
+    chromeos::DeviceOAuth2TokenService** out_token_service,
+    int* counter,
+    chromeos::DeviceOAuth2TokenService* in_token_service) {
+  *out_token_service = in_token_service;
+  ++(*counter);
+}
+
+// Sets up and tears down DeviceOAuth2TokenServiceFactory and its
+// dependencies. Also exposes FakeDBusThreadManager.
+class ScopedDeviceOAuth2TokenServiceFactorySetUp {
+ public:
+  ScopedDeviceOAuth2TokenServiceFactorySetUp()
+      : fake_dbus_manager_(new FakeDBusThreadManager) {
+    // Take ownership of fake_dbus_manager_.
+    DBusThreadManager::InitializeForTesting(fake_dbus_manager_);
+    SystemSaltGetter::Initialize();
+    DeviceOAuth2TokenServiceFactory::Initialize();
+  }
+
+  ~ScopedDeviceOAuth2TokenServiceFactorySetUp() {
+    DeviceOAuth2TokenServiceFactory::Shutdown();
+    SystemSaltGetter::Shutdown();
+    DBusThreadManager::Shutdown();
+  }
+
+  FakeDBusThreadManager* fake_dbus_manager() {
+    return fake_dbus_manager_;
+  }
+
+ private:
+  FakeDBusThreadManager* fake_dbus_manager_;
+};
+
+}  // namespace
+
+class DeviceOAuth2TokenServiceFactoryTest : public testing::Test {
+ protected:
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+// Test a case where Get() is called before the factory is initialized.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_Uninitialized) {
+  DeviceOAuth2TokenService* token_service = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  // The callback will be run asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service);
+
+  // This lets the factory run the callback with NULL.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, counter);
+  EXPECT_FALSE(token_service);
+}
+
+// Test a case where Get() is called from only one caller.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_Simple) {
+  ScopedDeviceOAuth2TokenServiceFactorySetUp scoped_setup;
+
+  DeviceOAuth2TokenService* token_service = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  // The callback will be run asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service);
+
+  // This lets FakeCryptohomeClient return the system salt.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, counter);
+  EXPECT_TRUE(token_service);
+
+  // Call Get() again, and confirm it works.
+  token_service = NULL;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, counter);
+  EXPECT_TRUE(token_service);
+}
+
+// Test a case where Get() is called from multiple callers, before the token
+// service is ready, and confirm that the callback of every caller is run.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_MultipleCallers) {
+  ScopedDeviceOAuth2TokenServiceFactorySetUp scoped_setup;
+
+  DeviceOAuth2TokenService* token_service1 = NULL;
+  DeviceOAuth2TokenService* token_service2 = NULL;
+  DeviceOAuth2TokenService* token_service3 = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service1, &counter));
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service2, &counter));
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service3, &counter));
+  // The token service will be returned asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service1);
+  EXPECT_FALSE(token_service2);
+  EXPECT_FALSE(token_service3);
+
+  // This lets FakeCryptohomeClient return the system salt.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(3, counter);
+  EXPECT_TRUE(token_service1);
+  EXPECT_TRUE(token_service2);
+  EXPECT_TRUE(token_service3);
+
+  // Make sure that token_service1,2,3 are the same one.
+  EXPECT_EQ(token_service1, token_service2);
+  EXPECT_EQ(token_service1, token_service3);
+}
+
+// Test a case where it failed to obtain the system salt.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_NoSystemSalt) {
+  ScopedDeviceOAuth2TokenServiceFactorySetUp scoped_setup;
+  scoped_setup.fake_dbus_manager()->fake_cryptohome_client()->
+      set_system_salt(std::vector<uint8>());
+
+  DeviceOAuth2TokenService* token_service = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  // The callback will be run asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service);
+
+  // This lets FakeCryptohomeClient return the system salt, which is empty.
+  // NULL should be returned to the callback in this case.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, counter);
+  EXPECT_FALSE(token_service);
+
+  // Try it again, but the result should remain the same (NULL returned).
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, counter);
+  EXPECT_FALSE(token_service);
+}
+
+}  // namespace chromeos
diff --git a/chrome/browser/chromeos/settings/token_encryptor.cc b/chrome/browser/chromeos/settings/token_encryptor.cc
index 269e8cb8..fe555f9 100644
--- a/chrome/browser/chromeos/settings/token_encryptor.cc
+++ b/chrome/browser/chromeos/settings/token_encryptor.cc
@@ -22,7 +22,13 @@ namespace {
 const size_t kNonceSize = 16;
 }  // namespace
 
-CryptohomeTokenEncryptor::CryptohomeTokenEncryptor() {
+CryptohomeTokenEncryptor::CryptohomeTokenEncryptor(
+    const std::string& system_salt)
+    : system_salt_(system_salt) {
+  DCHECK(!system_salt.empty());
+  // TODO(davidroche): should this use the system salt for both the password
+  // and the salt value, or should this use a separate salt value?
+  system_salt_key_.reset(PassphraseToKey(system_salt_, system_salt_));
 }
 
 CryptohomeTokenEncryptor::~CryptohomeTokenEncryptor() {
@@ -34,7 +40,7 @@ std::string CryptohomeTokenEncryptor::EncryptWithSystemSalt(
   if (!base::SysInfo::IsRunningOnChromeOS())
     return token;
 
-  if (!LoadSystemSaltKey()) {
+  if (!system_salt_key_) {
     LOG(WARNING) << "System salt key is not available for encrypt.";
     return std::string();
   }
@@ -49,7 +55,7 @@ std::string CryptohomeTokenEncryptor::DecryptWithSystemSalt(
   if (!base::SysInfo::IsRunningOnChromeOS())
     return encrypted_token_hex;
 
-  if (!LoadSystemSaltKey()) {
+  if (!system_salt_key_) {
     LOG(WARNING) << "System salt key is not available for decrypt.";
     return std::string();
   }
@@ -58,19 +64,6 @@ std::string CryptohomeTokenEncryptor::DecryptWithSystemSalt(
                              encrypted_token_hex);
 }
 
-// TODO: should this use the system salt for both the password and the salt
-// value, or should this use a separate salt value?
-bool CryptohomeTokenEncryptor::LoadSystemSaltKey() {
-  // Assume the system salt should be obtained beforehand at login time.
-  if (system_salt_.empty())
-    system_salt_ = SystemSaltGetter::Get()->GetCachedSystemSalt();
-  if (system_salt_.empty())
-    return false;
-  if (!system_salt_key_.get())
-    system_salt_key_.reset(PassphraseToKey(system_salt_, system_salt_));
-  return system_salt_key_.get();
-}
-
 crypto::SymmetricKey* CryptohomeTokenEncryptor::PassphraseToKey(
     const std::string& passphrase,
     const std::string& salt) {
diff --git a/chrome/browser/chromeos/settings/token_encryptor.h b/chrome/browser/chromeos/settings/token_encryptor.h
index 8892dc6..61d6d47 100644
--- a/chrome/browser/chromeos/settings/token_encryptor.h
+++ b/chrome/browser/chromeos/settings/token_encryptor.h
@@ -33,11 +33,11 @@ class TokenEncryptor {
       const std::string& encrypted_token_hex) = 0;
 };
 
-// TokenEncryptor based on the cryptohome daemon. This implementation is used
-// in production.
+// TokenEncryptor based on the system salt from cryptohome daemon. This
+// implementation is used in production.
 class CryptohomeTokenEncryptor : public TokenEncryptor {
  public:
-  CryptohomeTokenEncryptor();
+  explicit CryptohomeTokenEncryptor(const std::string& system_salt);
   virtual ~CryptohomeTokenEncryptor();
 
   // TokenEncryptor overrides:
@@ -46,10 +46,6 @@ class CryptohomeTokenEncryptor : public TokenEncryptor {
       const std::string& encrypted_token_hex) OVERRIDE;
 
  private:
-  // Loads the system salt key based on the system salt from the cryptohome
-  // daemon. Returns true on success.
-  bool LoadSystemSaltKey();
-
   // Converts |passphrase| to a SymmetricKey using the given |salt|.
   crypto::SymmetricKey* PassphraseToKey(const std::string& passphrase,
                                         const std::string& salt);
@@ -64,7 +60,8 @@ class CryptohomeTokenEncryptor : public TokenEncryptor {
                                   const std::string& salt,
                                   const std::string& encrypted_token_hex);
 
-  // The cached system salt obtained from the cryptohome daemon.
+  // The cached system salt passed to the constructor, originally coming
+  // from cryptohome daemon.
   std::string system_salt_;
 
   // A key based on the system salt.  Useful for encrypting device-level