From c9e620e90057f214879658eca758493e1ea3cbdb Mon Sep 17 00:00:00 2001 From: bartfab Date: Tue, 3 Feb 2015 03:17:05 -0800 Subject: [PATCH] Do not announce robot account token before account ID is available When announcing the availability of the robot account token to its observers, DeviceOAuth2TokenService needs to know both the token and the robot account ID. During enrollment, the token becomes available before the account ID. Thus, DeviceOAuth2TokenService needs to wait for the account ID to be available before announcing the token. BUG=453828 TEST=New unit test Review URL: https://codereview.chromium.org/892633003 Cr-Commit-Position: refs/heads/master@{#314312} --- ...iated_invalidation_service_provider_unittest.cc | 2 +- .../device_cloud_policy_invalidator_unittest.cc | 3 +- .../settings/device_oauth2_token_service.cc | 29 +++++++++++---- .../settings/device_oauth2_token_service.h | 7 ++++ .../settings/device_oauth2_token_service_factory.h | 6 ++-- .../device_oauth2_token_service_unittest.cc | 41 ++++++++++++++++++++++ 6 files changed, 77 insertions(+), 11 deletions(-) diff --git a/chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc b/chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc index 5c1cb609f551..c0798064f64a 100644 --- a/chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc +++ b/chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc @@ -143,12 +143,12 @@ AffiliatedInvalidationServiceProviderTest() void AffiliatedInvalidationServiceProviderTest::SetUp() { chromeos::SystemSaltGetter::Initialize(); chromeos::DBusThreadManager::Initialize(); - chromeos::DeviceOAuth2TokenServiceFactory::Initialize(); ASSERT_TRUE(profile_manager_.SetUp()); test_device_settings_service_.reset(new chromeos::ScopedTestDeviceSettingsService); test_cros_settings_.reset(new chromeos::ScopedTestCrosSettings); + chromeos::DeviceOAuth2TokenServiceFactory::Initialize(); invalidation::ProfileInvalidationProviderFactory::GetInstance()-> RegisterTestingFactory(BuildProfileInvalidationProvider); diff --git a/chrome/browser/chromeos/policy/device_cloud_policy_invalidator_unittest.cc b/chrome/browser/chromeos/policy/device_cloud_policy_invalidator_unittest.cc index 0aa81c4ce3e3..3c80871f9817 100644 --- a/chrome/browser/chromeos/policy/device_cloud_policy_invalidator_unittest.cc +++ b/chrome/browser/chromeos/policy/device_cloud_policy_invalidator_unittest.cc @@ -76,7 +76,6 @@ DeviceCloudPolicyInvalidatorTest::~DeviceCloudPolicyInvalidatorTest() { void DeviceCloudPolicyInvalidatorTest::SetUp() { chromeos::SystemSaltGetter::Initialize(); chromeos::DBusThreadManager::Initialize(); - chromeos::DeviceOAuth2TokenServiceFactory::Initialize(); TestingBrowserProcess::GetGlobal()->SetSystemRequestContext( system_request_context_.get()); ASSERT_TRUE(profile_manager_.SetUp()); @@ -84,6 +83,8 @@ void DeviceCloudPolicyInvalidatorTest::SetUp() { test_device_settings_service_.reset(new chromeos::ScopedTestDeviceSettingsService); test_cros_settings_.reset(new chromeos::ScopedTestCrosSettings); + chromeos::DeviceOAuth2TokenServiceFactory::Initialize(); + scoped_refptr owner_key_util( new ownership::MockOwnerKeyUtil); owner_key_util->SetPublicKeyFromPrivateKey( diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service.cc index 041ccb6a9b8e..1dc6bde934c3 100644 --- a/chrome/browser/chromeos/settings/device_oauth2_token_service.cc +++ b/chrome/browser/chromeos/settings/device_oauth2_token_service.cc @@ -8,16 +8,17 @@ #include #include "base/bind.h" +#include "base/bind_helpers.h" #include "base/memory/weak_ptr.h" #include "base/message_loop/message_loop.h" #include "base/prefs/pref_registry_simple.h" #include "base/prefs/pref_service.h" #include "base/values.h" #include "chrome/browser/browser_process.h" -#include "chrome/browser/chromeos/settings/cros_settings.h" #include "chrome/browser/chromeos/settings/token_encryptor.h" #include "chrome/common/pref_names.h" #include "chromeos/cryptohome/system_salt_getter.h" +#include "chromeos/settings/cros_settings_names.h" #include "google_apis/gaia/gaia_constants.h" #include "google_apis/gaia/gaia_urls.h" #include "google_apis/gaia/google_service_auth_error.h" @@ -42,6 +43,11 @@ struct DeviceOAuth2TokenService::PendingRequest { const ScopeSet scopes; }; +void DeviceOAuth2TokenService::OnServiceAccountIdentityChanged() { + if (!GetRobotAccountId().empty() && !refresh_token_.empty()) + FireRefreshTokenAvailable(GetRobotAccountId()); +} + DeviceOAuth2TokenService::DeviceOAuth2TokenService( net::URLRequestContextGetter* getter, PrefService* local_state) @@ -49,6 +55,12 @@ DeviceOAuth2TokenService::DeviceOAuth2TokenService( local_state_(local_state), state_(STATE_LOADING), max_refresh_token_validation_retries_(3), + service_account_identity_subscription_( + CrosSettings::Get()->AddSettingsObserver( + kServiceAccountIdentity, + base::Bind( + &DeviceOAuth2TokenService::OnServiceAccountIdentityChanged, + base::Unretained(this))).Pass()), weak_ptr_factory_(this) { // Pull in the system salt. SystemSaltGetter::Get()->GetSystemSalt( @@ -75,7 +87,12 @@ void DeviceOAuth2TokenService::SetAndSaveRefreshToken( bool waiting_for_salt = state_ == STATE_LOADING; refresh_token_ = refresh_token; state_ = STATE_VALIDATION_PENDING; - FireRefreshTokenAvailable(GetRobotAccountId()); + + // If the robot account ID is not available yet, do not announce the token. It + // will be done from OnServiceAccountIdentityChanged() once the robot account + // ID becomes available as well. + if (!GetRobotAccountId().empty()) + FireRefreshTokenAvailable(GetRobotAccountId()); token_save_callbacks_.push_back(result_callback); if (!waiting_for_salt) { @@ -261,10 +278,10 @@ void DeviceOAuth2TokenService::CheckRobotAccountId( const std::string& gaia_robot_id) { // Make sure the value returned by GetRobotAccountId has been validated // against current device settings. - switch (CrosSettings::Get()->PrepareTrustedValues( - base::Bind(&DeviceOAuth2TokenService::CheckRobotAccountId, - weak_ptr_factory_.GetWeakPtr(), - gaia_robot_id))) { + switch (CrosSettings::Get()->PrepareTrustedValues(base::Bind( + &DeviceOAuth2TokenService::CheckRobotAccountId, + weak_ptr_factory_.GetWeakPtr(), + gaia_robot_id))) { case CrosSettingsProvider::TRUSTED: // All good, compare account ids below. break; diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service.h b/chrome/browser/chromeos/settings/device_oauth2_token_service.h index 0d5f1fe6bc04..4941c0279dcd 100644 --- a/chrome/browser/chromeos/settings/device_oauth2_token_service.h +++ b/chrome/browser/chromeos/settings/device_oauth2_token_service.h @@ -15,6 +15,7 @@ #include "base/memory/weak_ptr.h" #include "base/stl_util.h" #include "base/time/time.h" +#include "chrome/browser/chromeos/settings/cros_settings.h" #include "google_apis/gaia/gaia_oauth_client.h" #include "google_apis/gaia/oauth2_token_service.h" #include "net/url_request/url_request_context_getter.h" @@ -104,6 +105,9 @@ class DeviceOAuth2TokenService : public OAuth2TokenService, STATE_TOKEN_VALID, }; + // Invoked by CrosSettings when the robot account ID becomes available. + void OnServiceAccountIdentityChanged(); + // Use DeviceOAuth2TokenServiceFactory to get an instance of this class. // Ownership of |token_encryptor| will be taken. explicit DeviceOAuth2TokenService(net::URLRequestContextGetter* getter, @@ -161,6 +165,9 @@ class DeviceOAuth2TokenService : public OAuth2TokenService, scoped_ptr gaia_oauth_client_; + scoped_ptr + service_account_identity_subscription_; + base::WeakPtrFactory weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(DeviceOAuth2TokenService); 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 8b20c88c8049..ccb370bb4273 100644 --- a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h +++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h @@ -26,13 +26,13 @@ class DeviceOAuth2TokenServiceFactory { // Called by ChromeBrowserMainPartsChromeOS in order to bootstrap the // DeviceOAuth2TokenService instance after the required global data is - // available (local state and request context getter). + // available (local state, request context getter and CrosSettings). static void Initialize(); // Called by ChromeBrowserMainPartsChromeOS in order to shutdown the // DeviceOAuth2TokenService instance and cancel all in-flight requests before - // the required global data is destroyed (local state and request context - // getter). + // the required global data is destroyed (local state, request context getter + // and CrosSettings). static void Shutdown(); private: diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc index 8ddfdace5be0..e499498ff375 100644 --- a/chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc +++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc @@ -28,10 +28,29 @@ #include "net/url_request/test_url_fetcher_factory.h" #include "net/url_request/url_fetcher_delegate.h" #include "net/url_request/url_request_test_util.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" namespace chromeos { +namespace { + +class MockOAuth2TokenServiceObserver : public OAuth2TokenService::Observer { + public: + MockOAuth2TokenServiceObserver(); + ~MockOAuth2TokenServiceObserver() override; + + MOCK_METHOD1(OnRefreshTokenAvailable, void(const std::string&)); +}; + +MockOAuth2TokenServiceObserver::MockOAuth2TokenServiceObserver() { +} + +MockOAuth2TokenServiceObserver::~MockOAuth2TokenServiceObserver() { +} + +} // namespace + static const int kOAuthTokenServiceUrlFetcherId = 0; static const int kValidatorUrlFetcherId = gaia::GaiaOAuthClient::kUrlFetcherId; @@ -96,6 +115,7 @@ class DeviceOAuth2TokenServiceTest : public testing::Test { } void TearDown() override { + oauth2_service_.reset(); CrosSettings::Shutdown(); TestingBrowserProcess::GetGlobal()->SetBrowserPolicyConnector(NULL); content::BrowserThread::GetBlockingPool()->FlushForTesting(); @@ -430,4 +450,25 @@ TEST_F(DeviceOAuth2TokenServiceTest, RefreshTokenValidation_Retry) { AssertConsumerTokensAndErrors(1, 1); } +TEST_F(DeviceOAuth2TokenServiceTest, DoNotAnnounceTokenWithoutAccountID) { + CreateService(); + + testing::StrictMock observer; + oauth2_service_->AddObserver(&observer); + + // Make a token available during enrollment. Verify that the token is not + // announced yet. + oauth2_service_->SetAndSaveRefreshToken( + "test-token", DeviceOAuth2TokenService::StatusCallback()); + testing::Mock::VerifyAndClearExpectations(&observer); + + // Also make the robot account ID available. Verify that the token is + // announced now. + EXPECT_CALL(observer, OnRefreshTokenAvailable("robot@example.com")); + SetRobotAccountId("robot@example.com"); + testing::Mock::VerifyAndClearExpectations(&observer); + + oauth2_service_->RemoveObserver(&observer); +} + } // namespace chromeos -- 2.11.4.GIT