From 0a23c78c57ecb2405837155aa0a0def7b5ba9c22 Mon Sep 17 00:00:00 2001 From: Kush Sinha Date: Tue, 29 Jan 2019 16:45:21 +0000 Subject: [PATCH] Pre-emptively reject dummy tokens in ChromeOSOAuth2TokenServiceDelegate |ChromeOSOAuth2TokenServiceDelegate| should pre-emptively reject access token fetches and other operations on known dummy Gaia tokens instead of letting them go over the network and get rejected by Gaia server-side. This brings feature parity between |ChromeOSOAuth2TokenServiceDelegate| and |MutableProfileOAuth2TokenServiceDelegate|, which does the same thing. Bug: 915628 Test: unit_tests --gtest_filter="*CrOSOAuthDelegateTest*" Change-Id: I812a847fb1afd040a3ae743a9e491753878ca523 Reviewed-on: https://chromium-review.googlesource.com/c/1437627 Commit-Queue: Kush Sinha Reviewed-by: Xiyuan Xia Cr-Commit-Position: refs/heads/master@{#627034} --- .../chromeos/oauth2_token_service_delegate.cc | 15 +++++++-- .../oauth2_token_service_delegate_unittest.cc | 31 +++++++++++++++++-- chromeos/account_manager/account_manager.cc | 8 +++++ chromeos/account_manager/account_manager.h | 6 ++++ 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/chrome/browser/chromeos/oauth2_token_service_delegate.cc b/chrome/browser/chromeos/oauth2_token_service_delegate.cc index 467ff7b3a0402d..dc33522d6506b6 100644 --- a/chrome/browser/chromeos/oauth2_token_service_delegate.cc +++ b/chrome/browser/chromeos/oauth2_token_service_delegate.cc @@ -146,7 +146,7 @@ void ChromeOSOAuth2TokenServiceDelegate::UpdateAuthError( } auto it = errors_.find(account_id); - if ((it != errors_.end())) { + if (it != errors_.end()) { // Update the existing error. if (error.state() == GoogleServiceAuthError::NONE) errors_.erase(it); @@ -268,13 +268,24 @@ void ChromeOSOAuth2TokenServiceDelegate::OnTokenUpserted( // invoked here, regardless. See the comment above |FireAuthErrorChanged| few // lines down. errors_.erase(account_id); + GoogleServiceAuthError error(GoogleServiceAuthError::AuthErrorNone()); + + // However, if we know that |account_key| has a dummy token, store a + // persistent error against it, so that we can pre-emptively reject access + // token requests for it. + if (account_manager_->HasDummyGaiaToken(account_key)) { + error = GoogleServiceAuthError::FromInvalidGaiaCredentialsReason( + GoogleServiceAuthError::InvalidGaiaCredentialsReason:: + CREDENTIALS_REJECTED_BY_CLIENT); + errors_.emplace(account_id, AccountErrorStatus{error}); + } ScopedBatchChange batch(this); FireRefreshTokenAvailable(account_id); // See |OAuth2TokenService::Observer::OnAuthErrorChanged|. // |OnAuthErrorChanged| must be always called after // |OnRefreshTokenAvailable|, when refresh token is updated. - FireAuthErrorChanged(account_id, GoogleServiceAuthError::AuthErrorNone()); + FireAuthErrorChanged(account_id, error); } void ChromeOSOAuth2TokenServiceDelegate::OnAccountRemoved( diff --git a/chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc b/chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc index 2fd9d9dc27bbcd..9edb49b4fc204c 100644 --- a/chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc +++ b/chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc @@ -84,9 +84,16 @@ class TokenServiceObserver : public OAuth2TokenService::Observer { void OnRefreshTokenAvailable(const std::string& account_id) override { EXPECT_TRUE(is_inside_batch_); - // We should not be seeing any cached errors for a freshly updated account. - EXPECT_EQ(GoogleServiceAuthError::AuthErrorNone(), - delegate_->GetAuthError(account_id)); + // We should not be seeing any cached errors for a freshly updated account, + // except when they have been generated by us (i.e. + // CREDENTIALS_REJECTED_BY_CLIENT). + const GoogleServiceAuthError error = delegate_->GetAuthError(account_id); + EXPECT_TRUE((error == GoogleServiceAuthError::AuthErrorNone()) || + (error.state() == + GoogleServiceAuthError::State::INVALID_GAIA_CREDENTIALS && + error.GetInvalidGaiaCredentialsReason() == + GoogleServiceAuthError::InvalidGaiaCredentialsReason:: + CREDENTIALS_REJECTED_BY_CLIENT)); account_ids_.insert(account_id); @@ -273,6 +280,24 @@ TEST_F(CrOSOAuthDelegateTest, delegate_->UpdateCredentials(account_info_.account_id, "new-token"); } +TEST_F(CrOSOAuthDelegateTest, DummyTokensArePreEmptivelyRejected) { + TokenServiceObserver observer(delegate_.get()); + delegate_->UpdateCredentials(account_info_.account_id, + AccountManager::kInvalidToken); + + const GoogleServiceAuthError error = + delegate_->GetAuthError(account_info_.account_id); + EXPECT_EQ(GoogleServiceAuthError::State::INVALID_GAIA_CREDENTIALS, + error.state()); + EXPECT_EQ(GoogleServiceAuthError::InvalidGaiaCredentialsReason:: + CREDENTIALS_REJECTED_BY_CLIENT, + error.GetInvalidGaiaCredentialsReason()); + + // Observer notification should also have notified about the same error. + EXPECT_EQ(error, observer.last_err_); + EXPECT_EQ(account_info_.account_id, observer.last_err_account_id_); +} + TEST_F(CrOSOAuthDelegateTest, ObserversAreNotifiedOnCredentialsUpdate) { TokenServiceObserver observer(delegate_.get()); delegate_->UpdateCredentials(account_info_.account_id, kGaiaToken); diff --git a/chromeos/account_manager/account_manager.cc b/chromeos/account_manager/account_manager.cc index 92c0accf0916a3..8337f22659df96 100644 --- a/chromeos/account_manager/account_manager.cc +++ b/chromeos/account_manager/account_manager.cc @@ -398,6 +398,14 @@ bool AccountManager::IsTokenAvailable(const AccountKey& account_key) const { it->second != kActiveDirectoryDummyToken; } +bool AccountManager::HasDummyGaiaToken(const AccountKey& account_key) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + DCHECK_EQ(init_state_, InitializationState::kInitialized); + + auto it = tokens_.find(account_key); + return it != tokens_.end() && it->second == kInvalidToken; +} + void AccountManager::MaybeRevokeTokenOnServer(const AccountKey& account_key) { auto it = tokens_.find(account_key); if (it == tokens_.end()) { diff --git a/chromeos/account_manager/account_manager.h b/chromeos/account_manager/account_manager.h index d2d182993ed836..5e2065bd609f63 100644 --- a/chromeos/account_manager/account_manager.h +++ b/chromeos/account_manager/account_manager.h @@ -174,6 +174,12 @@ class CHROMEOS_EXPORT AccountManager { // initialized yet. bool IsTokenAvailable(const AccountKey& account_key) const; + // Returns true if the token stored against |account_key| is a dummy Gaia + // token. This is meant to be used only by + // |ChromeOSOAuth2TokenServiceDelegate| to pre-emptively reject access token + // requests for |account_key|. + bool HasDummyGaiaToken(const AccountKey& account_key) const; + private: enum InitializationState { kNotStarted, // Initialize has not been called