[Signin] Handle multiple concurrent Dice responses

chrome/browser/signin/dice_response_handler.cc

Index: chrome/browser/signin/dice_response_handler.cc
diff --git a/chrome/browser/signin/dice_response_handler.cc b/chrome/browser/signin/dice_response_handler.cc
index 6a7aaf20681cccc66f1ae8aab5bd195279a93262..1369eb1d86d86219b402549283c5c144e7d890cd 100644
--- a/chrome/browser/signin/dice_response_handler.cc
+++ b/chrome/browser/signin/dice_response_handler.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/signin/dice_response_handler.h"
 
+#include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
@@ -64,6 +65,45 @@ class DiceResponseHandlerFactory : public BrowserContextKeyedServiceFactory {
 
 }  // namespace
 
+////////////////////////////////////////////////////////////////////////////////
+// DiceTokenFetcher
+////////////////////////////////////////////////////////////////////////////////
+
+DiceResponseHandler::DiceTokenFetcher::DiceTokenFetcher(
+    const std::string& gaia_id,
+    const std::string& email,
+    const std::string& authorization_code,
+    SigninClient* signin_client,
+    DiceResponseHandler* dice_response_handler)
+    : gaia_id_(gaia_id),
+      email_(email),
+      dice_response_handler_(dice_response_handler) {
+  gaia_auth_fetcher_ = signin_client->CreateGaiaAuthFetcher(
+      this, GaiaConstants::kChromeSource,
+      signin_client->GetURLRequestContext());
+  gaia_auth_fetcher_->StartAuthCodeForOAuth2TokenExchange(authorization_code);
+
+  // TODO(droger): The token exchange must complete quickly or be cancelled. Add
+  // a timeout logic.
+}
+
+DiceResponseHandler::DiceTokenFetcher::~DiceTokenFetcher() {}
+
+void DiceResponseHandler::DiceTokenFetcher::OnClientOAuthSuccess(
+    const GaiaAuthConsumer::ClientOAuthResult& result) {
+  dice_response_handler_->OnTokenExchangeSuccess(this, gaia_id_, email_,
+                                                 result);
+}
+
+void DiceResponseHandler::DiceTokenFetcher::OnClientOAuthFailure(
+    const GoogleServiceAuthError& error) {
+  dice_response_handler_->OnTokenExchangeFailure(this, error);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// DiceResponseHandler
+////////////////////////////////////////////////////////////////////////////////
+
 // static
 DiceResponseHandler* DiceResponseHandler::GetForProfile(Profile* profile) {
   return DiceResponseHandlerFactory::GetForProfile(profile);
@@ -110,36 +150,41 @@ void DiceResponseHandler::ProcessDiceSigninHeader(
     const std::string& gaia_id,
     const std::string& email,
     const std::string& authorization_code) {
-  DCHECK(!gaia_auth_fetcher_);
-  DCHECK(gaia_id_.empty());
-  DCHECK(email_.empty());
-  gaia_id_ = gaia_id;
-  email_ = email;
-  gaia_auth_fetcher_ = signin_client_->CreateGaiaAuthFetcher(
-      this, GaiaConstants::kChromeSource,
-      signin_client_->GetURLRequestContext());
-  gaia_auth_fetcher_->StartAuthCodeForOAuth2TokenExchange(authorization_code);
+  for (auto it = token_fetchers_.begin(); it != token_fetchers_.end(); ++it) {
+    if ((it->get()->gaia_id() == gaia_id) && (it->get()->email() == email))

[msarda, 2017/06/22 09:51:31]

This is a tricky case - in this case the Gaia cookie contains an account that
was reauthenticated while a token request was in progress. It is not clear to me
if the old authorization code can still be used. Also, it is not clear to me how
this would work once the cookie and the refresh tokens are bound one to another.

I think it should be safer to remove the old token fetcher and start a new one
with a fresher authorization code.

WDYT?

[droger, 2017/06/22 11:54:20]

Thanks fro bringing that up.

Summary of offline discussion:
1) we can cancel the request if the authorization code is the same
2) if requests are different and we receive a different token, then we should
probably revoke the old token. Note that this should be done even if the
requests don't happen at the same time, and thus belongs to another layer
(probably the token service: UpdateCredentials should revoke the old
credentials). Filed https://crbug.com/735898

+      return;  // There is already a request in flight with the same parameters.
+  }
 
-  // TODO(droger): The token exchange must complete quickly or be cancelled. Add
-  // a timeout logic.
+  token_fetchers_.push_back(base::MakeUnique<DiceTokenFetcher>(
+      gaia_id, email, authorization_code, signin_client_, this));
 }
 
-void DiceResponseHandler::OnClientOAuthSuccess(
-    const ClientOAuthResult& result) {
+void DiceResponseHandler::DeleteTokenFetcher(DiceTokenFetcher* token_fetcher) {
+  for (auto it = token_fetchers_.begin(); it != token_fetchers_.end(); ++it) {
+    if (it->get() == token_fetcher) {
+      token_fetchers_.erase(it);
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
+void DiceResponseHandler::OnTokenExchangeSuccess(
+    DiceTokenFetcher* token_fetcher,
+    const std::string& gaia_id,
+    const std::string& email,
+    const GaiaAuthConsumer::ClientOAuthResult& result) {
   std::string account_id =
-      account_tracker_service_->SeedAccountInfo(gaia_id_, email_);
+      account_tracker_service_->SeedAccountInfo(gaia_id, email);
   profile_oauth2_token_service_->UpdateCredentials(account_id,
                                                    result.refresh_token);
-  gaia_id_.clear();
-  email_.clear();
-  gaia_auth_fetcher_.reset();
+  DeleteTokenFetcher(token_fetcher);
 }
 
-void DiceResponseHandler::OnClientOAuthFailure(
+void DiceResponseHandler::OnTokenExchangeFailure(
+    DiceTokenFetcher* token_fetcher,
     const GoogleServiceAuthError& error) {
   // TODO(droger): Handle authentication errors.
   DLOG(ERROR) << "Dice OAuth failed: " << error.error_message();
-  gaia_id_.clear();
-  email_.clear();
-  gaia_auth_fetcher_.reset();
+  DeleteTokenFetcher(token_fetcher);
 }