diff options
11 files changed, 282 insertions, 65 deletions
diff --git a/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc b/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc index 4d321dc..eb02a0a 100644 --- a/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc +++ b/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc @@ -108,10 +108,9 @@ class DeviceCloudPolicyManagerChromeOSTest TestingBrowserProcess::GetGlobal()->SetSystemRequestContext( request_context_getter_.get()); TestingBrowserProcess::GetGlobal()->SetLocalState(&local_state_); - chromeos::DeviceOAuth2TokenServiceFactory::Initialize(); - // This is needed as SystemSaltGetter is used for encrypting tokens in - // CryptohomeTokenEncryptor. + // SystemSaltGetter is used in DeviceOAuth2TokenServiceFactory. chromeos::SystemSaltGetter::Initialize(); + chromeos::DeviceOAuth2TokenServiceFactory::Initialize(); url_fetcher_response_code_ = 200; url_fetcher_response_string_ = "{\"access_token\":\"accessToken4Test\"," "\"expires_in\":1234," 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 diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 685152f..b903f38 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -717,6 +717,7 @@ 'browser/chromeos/session_length_limiter_unittest.cc', 'browser/chromeos/proxy_config_service_impl_unittest.cc', 'browser/chromeos/settings/cros_settings_unittest.cc', + 'browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc', 'browser/chromeos/settings/device_oauth2_token_service_unittest.cc', 'browser/chromeos/settings/device_settings_provider_unittest.cc', 'browser/chromeos/settings/device_settings_service_unittest.cc', diff --git a/chromeos/cryptohome/system_salt_getter.cc b/chromeos/cryptohome/system_salt_getter.cc index f299f0f..1fa25a3 100644 --- a/chromeos/cryptohome/system_salt_getter.cc +++ b/chromeos/cryptohome/system_salt_getter.cc @@ -38,10 +38,6 @@ std::string SystemSaltGetter::GetSystemSaltSync() { return system_salt_; } -std::string SystemSaltGetter::GetCachedSystemSalt() { - return system_salt_; -} - void SystemSaltGetter::GetSystemSaltInternal( const GetSystemSaltCallback& callback, bool service_is_available) { diff --git a/chromeos/cryptohome/system_salt_getter.h b/chromeos/cryptohome/system_salt_getter.h index a47b617..1e4cf81 100644 --- a/chromeos/cryptohome/system_salt_getter.h +++ b/chromeos/cryptohome/system_salt_getter.h @@ -40,11 +40,6 @@ class CHROMEOS_EXPORT SystemSaltGetter { // DEPRECATED: DO NOT USE. std::string GetSystemSaltSync(); - // Returns system hash in hex encoded ascii format, cached by a prior call - // to GetSystemSalt(). Note: this may return an empty string (e.g. if - // GetSystemSalt() is not yet called). - std::string GetCachedSystemSalt(); - protected: SystemSaltGetter(); ~SystemSaltGetter(); diff --git a/chromeos/dbus/fake_cryptohome_client.cc b/chromeos/dbus/fake_cryptohome_client.cc index d23be71..861a1c5 100644 --- a/chromeos/dbus/fake_cryptohome_client.cc +++ b/chromeos/dbus/fake_cryptohome_client.cc @@ -17,6 +17,7 @@ FakeCryptohomeClient::FakeCryptohomeClient() async_call_id_(1), tpm_is_ready_counter_(0), unmount_result_(true), + system_salt_(GetStubSystemSalt()), locked_(false), weak_ptr_factory_(this) {} @@ -80,7 +81,7 @@ void FakeCryptohomeClient::AsyncRemove( } bool FakeCryptohomeClient::GetSystemSalt(std::vector<uint8>* salt) { - *salt = GetStubSystemSalt(); + *salt = system_salt_; return true; } diff --git a/chromeos/dbus/fake_cryptohome_client.h b/chromeos/dbus/fake_cryptohome_client.h index 3c960e8..450678b 100644 --- a/chromeos/dbus/fake_cryptohome_client.h +++ b/chromeos/dbus/fake_cryptohome_client.h @@ -162,6 +162,13 @@ class CHROMEOS_EXPORT FakeCryptohomeClient : public CryptohomeClient { unmount_result_= result; } + // Sets the system salt which will be returned from GetSystemSalt(). By + // default, GetSystemSalt() returns the value generated by + // GetStubSystemSalt(). + void set_system_salt(const std::vector<uint8>& system_salt) { + system_salt_ = system_salt; + } + // Returns the stub system salt as raw bytes. (not as a string encoded in the // format used by SystemSaltGetter::ConvertRawSaltToHexString()). static std::vector<uint8> GetStubSystemSalt(); @@ -181,6 +188,7 @@ class CHROMEOS_EXPORT FakeCryptohomeClient : public CryptohomeClient { AsyncCallStatusWithDataHandler async_call_status_data_handler_; int tpm_is_ready_counter_; bool unmount_result_; + std::vector<uint8> system_salt_; std::vector<WaitForServiceToBeAvailableCallback> pending_wait_for_service_to_be_available_callbacks_; |