IdentityManager: Fix timing of token update observer callbacks

This CL fixes a timing issue with the implementation of
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount():

- It is fired from IdentityManager::WillFireOnRefreshTokenAvailable()
- which is fired from
ProfileOAuth2TokenService::OnRefreshTokenAvailable()
- just *before* ProfileOAuth2TokenService calls
CancelRequestsForAccount().

The upshot is that any access token requests made from
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount()
implementations will be immediately cancelled. Yikes! On desktop, it
also means that access token requests that are made from
ProfileOAuth2TokenService::Observer::OnRefreshTokenAvailable() clients
after any such cancellations will also be cancelled, as
MutableProfileOAuth2TokenServiceDelegate has a policy of immediately
canceling access token requests for some amount of time after it has
seen a cancelled access token request (this is part of its backoff
policy).

The fix is the following:
- Move the firing of the IdentityManager::Observer callbacks to its
  (new) implementation of the corresponding
  OAuth2TokenService::Observer callbacks.

Note that the updating of IdentityManager's internal state must still
be done in IdentityManager::WillFireOnRefreshToken{Available, Revoked};
this ensures that it updates its state before any observers of
ProfileOAuth2TokenService are notified of the state change, and thus
that any such observers see a consistent state between IdentityManager
and ProfileOAuth2TokenService. To decouple the internal update from
the firing of the observer callbacks, some state is now saved from
the internal updates firing to forward in the observer callbacks.

This change is unittested via extensions to the
PrimaryAccountAccessTokenFetcher unittests that ensure that the fetcher
does not have a request cancelled that is made in its implementation of
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount(). Before
the production fix, the extended unittests would have failed, as the
fetchers were seeing a cancelled request and then retrying it (which
succeeded).

Bug: 869376
Change-Id: Ic5d3208ef893ad0e821c38bbb88c713d68042a1a
Reviewed-on: https://chromium-review.googlesource.com/1152731
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579813}

services/identity/public/cpp/identity_manager.cc

@@ -88,6 +88,7 @@ IdentityManager::IdentityManager(
       ->set_diagnostics_client(this);
 #endif
   token_service_->AddDiagnosticsObserver(this);
+  token_service_->AddObserver(this);
   token_service_->set_diagnostics_client(this);
   gaia_cookie_manager_service_->AddObserver(this);
 }
@@ -98,6 +99,7 @@ IdentityManager::~IdentityManager() {
   SigninManager::FromSigninManagerBase(signin_manager_)
       ->set_diagnostics_client(nullptr);
 #endif
+  token_service_->RemoveObserver(this);
   token_service_->RemoveDiagnosticsObserver(this);
   token_service_->set_diagnostics_client(nullptr);
   gaia_cookie_manager_service_->RemoveObserver(this);
@@ -286,6 +288,8 @@ void IdentityManager::GoogleSignedOut(const AccountInfo& account_info) {
 void IdentityManager::WillFireOnRefreshTokenAvailable(
     const std::string& account_id,
     bool is_valid) {
+  DCHECK(!pending_token_available_state_);
+
   AccountInfo account_info =
       account_tracker_service_->GetAccountInfo(account_id);
 
@@ -319,15 +323,16 @@ void IdentityManager::WillFireOnRefreshTokenAvailable(
     iterator = insertion_result.first;
   }
 
-  // Use iterator instead of account_info as it may have been moved, thus is
-  // invalid to use.
-  for (auto& observer : observer_list_) {
-    observer.OnRefreshTokenUpdatedForAccount(iterator->second, is_valid);
-  }
+  PendingTokenAvailableState pending_token_available_state;
+  pending_token_available_state.account_info = iterator->second;
+  pending_token_available_state.refresh_token_is_valid = is_valid;
+  pending_token_available_state_ = std::move(pending_token_available_state);
 }
 
 void IdentityManager::WillFireOnRefreshTokenRevoked(
     const std::string& account_id) {
+  DCHECK(!pending_token_revoked_info_);
+
   auto iterator = accounts_with_refresh_tokens_.find(account_id);
   if (iterator == accounts_with_refresh_tokens_.end()) {
     // A corner case exists wherein the token service revokes tokens while
@@ -345,7 +350,7 @@ void IdentityManager::WillFireOnRefreshTokenRevoked(
 
   accounts_with_refresh_tokens_.erase(iterator);
 
-  AccountInfo account_info =
+  pending_token_revoked_info_ =
       account_tracker_service_->GetAccountInfo(account_id);
 
   // In the context of supervised users, the ProfileOAuth2TokenService is used
@@ -356,14 +361,52 @@ void IdentityManager::WillFireOnRefreshTokenRevoked(
 
   // TODO(860492): Remove this special case once supervised user support is
   // removed.
-  DCHECK(!account_info.IsEmpty() || account_id == kSupervisedUserPseudoEmail);
-  if (account_id == kSupervisedUserPseudoEmail && account_info.IsEmpty()) {
+  DCHECK(!pending_token_revoked_info_->IsEmpty() ||
+         account_id == kSupervisedUserPseudoEmail);
+  if (account_id == kSupervisedUserPseudoEmail &&
+      pending_token_revoked_info_->IsEmpty()) {
     // Populate the information manually to maintain the invariant that the
     // account ID, gaia ID, and email are always set.
-    account_info.account_id = account_id;
-    account_info.email = account_id;
-    account_info.gaia = kSupervisedUserPseudoGaiaID;
+    pending_token_revoked_info_->account_id = account_id;
+    pending_token_revoked_info_->email = account_id;
+    pending_token_revoked_info_->gaia = kSupervisedUserPseudoGaiaID;
   }
+}
+
+void IdentityManager::OnRefreshTokenAvailable(const std::string& account_id) {
+  DCHECK(pending_token_available_state_);
+  DCHECK_EQ(pending_token_available_state_->account_info.account_id,
+            account_id);
+
+  // Move the state out of |pending_token_available_state_| in case any observer
+  // callbacks fired below result in mutations of refresh tokens.
+  AccountInfo account_info =
+      std::move(pending_token_available_state_->account_info);
+  bool refresh_token_is_valid =
+      pending_token_available_state_->refresh_token_is_valid;
+
+  pending_token_available_state_.reset();
+
+  for (auto& observer : observer_list_) {
+    observer.OnRefreshTokenUpdatedForAccount(account_info,
+                                             refresh_token_is_valid);
+  }
+}
+
+void IdentityManager::OnRefreshTokenRevoked(const std::string& account_id) {
+  // NOTE: It is possible for |pending_token_revoked_info_| to be null in the
+  // corner case of tokens being revoked during initial startup (see
+  // WillFireOnRefreshTokenRevoked() above).
+  if (!pending_token_revoked_info_) {
+    return;
+  }
+
+  DCHECK_EQ(pending_token_revoked_info_->account_id, account_id);
+
+  // Move the state out of |pending_token_revoked_info_| in case any observer
+  // callbacks fired below result in mutations of refresh tokens.
+  AccountInfo account_info = pending_token_revoked_info_.value();
+  pending_token_revoked_info_.reset();
 
   for (auto& observer : observer_list_) {
     observer.OnRefreshTokenRemovedForAccount(account_info);

services/identity/public/cpp/identity_manager.h

@@ -48,6 +48,7 @@ class IdentityManager : public SigninManagerBase::Observer,
 #endif
                         public ProfileOAuth2TokenService::DiagnosticsClient,
                         public OAuth2TokenService::DiagnosticsObserver,
+                        public OAuth2TokenService::Observer,
                         public GaiaCookieManagerService::Observer {
  public:
   class Observer {
@@ -179,6 +180,11 @@ class IdentityManager : public SigninManagerBase::Observer,
   void RemoveDiagnosticsObserver(DiagnosticsObserver* observer);
 
  private:
+  struct PendingTokenAvailableState {
+    AccountInfo account_info;
+    bool refresh_token_is_valid = false;
+  };
+
   // These clients need to call SetPrimaryAccountSynchronouslyForTests().
   friend AccountInfo SetPrimaryAccount(SigninManagerBase* signin_manager,
                                        IdentityManager* identity_manager,
@@ -220,6 +226,10 @@ class IdentityManager : public SigninManagerBase::Observer,
                                        bool is_valid) override;
   void WillFireOnRefreshTokenRevoked(const std::string& account_id) override;
 
+  // OAuth2TokenService::Observer:
+  void OnRefreshTokenAvailable(const std::string& account_id) override;
+  void OnRefreshTokenRevoked(const std::string& account_id) override;
+
 #if !defined(OS_CHROMEOS)
   // SigninManager::DiagnosticsClient:
   // Override these to update |primary_account_info_| before any observers of
@@ -265,6 +275,13 @@ class IdentityManager : public SigninManagerBase::Observer,
   using AccountIDToAccountInfoMap = std::map<std::string, AccountInfo>;
   AccountIDToAccountInfoMap accounts_with_refresh_tokens_;
 
+  // Info that is cached from the PO2TS::DiagnosticsClient callbacks in order to
+  // forward on to the observers of this class in the corresponding
+  // O2TS::Observer callbacks (the information is not directly available at the
+  // time of receiving the O2TS::Observer callbacks).
+  base::Optional<PendingTokenAvailableState> pending_token_available_state_;
+  base::Optional<AccountInfo> pending_token_revoked_info_;
+
   // Lists of observers.
   // Makes sure lists are empty on destruction.
   base::ObserverList<Observer, true> observer_list_;

services/identity/public/cpp/identity_manager_unittest.cc

@@ -132,9 +132,9 @@ class TestSigninManagerObserver : public SigninManagerBase::Observer {
   AccountInfo primary_account_from_signout_callback_;
 };
 
-// Class that observes updates from both ProfileOAuth2TokenService and
-// IdentityManager and verifies thereby that IdentityManager receives updates
-// before direct observers of ProfileOAuth2TokenService.
+// Class that observes updates from ProfileOAuth2TokenService and and verifies
+// thereby that IdentityManager receives updates before direct observers of
+// ProfileOAuth2TokenService.
 class TestTokenServiceObserver : public OAuth2TokenService::Observer,
                                  public identity::IdentityManager::Observer {
  public:
@@ -144,12 +144,10 @@ class TestTokenServiceObserver : public OAuth2TokenService::Observer,
   }
   ~TestTokenServiceObserver() override {
     token_service_->RemoveObserver(this);
-    identity_manager_->RemoveObserver(this);
   }
 
   void set_identity_manager(IdentityManager* identity_manager) {
     identity_manager_ = identity_manager;
-    identity_manager_->AddObserver(this);
   }
 
   void set_on_refresh_token_available_callback(base::OnceClosure callback) {
@@ -160,46 +158,22 @@ class TestTokenServiceObserver : public OAuth2TokenService::Observer,
   }
 
  private:
-  // IdentityManager::Observer:
-  void OnRefreshTokenUpdatedForAccount(const AccountInfo& account_info,
-                                       bool is_valid) override {
-    EXPECT_TRUE(
-        account_id_from_identity_manager_token_updated_callback_.empty());
-    account_id_from_identity_manager_token_updated_callback_ =
-        account_info.account_id;
-  }
-  void OnRefreshTokenRemovedForAccount(
-      const AccountInfo& account_info) override {
-    EXPECT_TRUE(
-        account_id_from_identity_manager_token_removed_callback_.empty());
-    account_id_from_identity_manager_token_removed_callback_ =
-        account_info.account_id;
-  }
-
   // OAuth2TokenService::Observer:
   void OnRefreshTokenAvailable(const std::string& account_id) override {
-    // This object should have received the corresponding IdentityManager
-    // callback before receiving this callback.
-    EXPECT_EQ(account_id_from_identity_manager_token_updated_callback_,
-              account_id);
-    account_id_from_identity_manager_token_updated_callback_.clear();
+    // IdentityManager should have already updated its state.
+    EXPECT_TRUE(identity_manager_->HasAccountWithRefreshToken(account_id));
     if (on_refresh_token_available_callback_)
       std::move(on_refresh_token_available_callback_).Run();
   }
   void OnRefreshTokenRevoked(const std::string& account_id) override {
-    // This object should have received the corresponding IdentityManager
-    // callback before receiving this callback.
-    EXPECT_EQ(account_id_from_identity_manager_token_removed_callback_,
-              account_id);
-    account_id_from_identity_manager_token_removed_callback_.clear();
+    // IdentityManager should have already updated its state.
+    EXPECT_FALSE(identity_manager_->HasAccountWithRefreshToken(account_id));
     if (on_refresh_token_revoked_callback_)
       std::move(on_refresh_token_revoked_callback_).Run();
   }
 
   OAuth2TokenService* token_service_;
   IdentityManager* identity_manager_;
-  std::string account_id_from_identity_manager_token_updated_callback_;
-  std::string account_id_from_identity_manager_token_removed_callback_;
   base::OnceClosure on_refresh_token_available_callback_;
   base::OnceClosure on_refresh_token_revoked_callback_;
 };
@@ -1234,6 +1208,7 @@ TEST_F(IdentityManagerTest,
   // both TokenService::Observers would work as IdentityManager would
   // get notified first during the observer callbacks.
   RecreateIdentityManager();
+  EXPECT_TRUE(identity_manager()->GetAccountsWithRefreshTokens().empty());
   token_service_observer.set_identity_manager(identity_manager());
 
   // When the observer receives the callback directly from the token service,

services/identity/public/cpp/primary_account_access_token_fetcher.h

@@ -46,6 +46,9 @@ class PrimaryAccountAccessTokenFetcher : public IdentityManager::Observer {
 
   ~PrimaryAccountAccessTokenFetcher() override;
 
+  // Exposed for tests.
+  bool access_token_request_retried() { return access_token_retried_; }
+
  private:
   // Returns true iff there is a primary account with a refresh token. Should
   // only be called in mode |kWaitUntilAvailable|.

services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

@@ -201,6 +201,9 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest, ShouldWaitForSignIn) {
                             access_token_info()));
   identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
       access_token_info().token, access_token_info().expiration_time);
+
+  // The request should not have to have been retried.
+  EXPECT_FALSE(fetcher->access_token_request_retried());
 }
 
 #endif  // !OS_CHROMEOS
@@ -226,6 +229,9 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest, ShouldWaitForRefreshToken) {
                             access_token_info()));
   identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
       access_token_info().token, access_token_info().expiration_time);
+
+  // The request should not have to have been retried.
+  EXPECT_FALSE(fetcher->access_token_request_retried());
 }
 
 TEST_F(PrimaryAccountAccessTokenFetcherTest,