[Signin] Add timeout to Dice token exchange requests

BUG=730589

Review-Url: https://codereview.chromium.org/2951263002
Cr-Commit-Position: refs/heads/master@{#481906}

chrome/browser/signin/dice_response_handler.cc

@@ -4,8 +4,13 @@
 
 #include "chrome/browser/signin/dice_response_handler.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/chrome_signin_client_factory.h"
@@ -21,6 +26,8 @@
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 
+const int kDiceTokenFetchTimeoutSeconds = 10;
+
 namespace {
 
 class DiceResponseHandlerFactory : public BrowserContextKeyedServiceFactory {
@@ -78,27 +85,45 @@ DiceResponseHandler::DiceTokenFetcher::DiceTokenFetcher(
     : gaia_id_(gaia_id),
       email_(email),
       authorization_code_(authorization_code),
-      dice_response_handler_(dice_response_handler) {
+      dice_response_handler_(dice_response_handler),
+      timeout_closure_(
+          base::Bind(&DiceResponseHandler::DiceTokenFetcher::OnTimeout,
+                     base::Unretained(this))) {
+  DCHECK(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.
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, timeout_closure_.callback(),
+      base::TimeDelta::FromSeconds(kDiceTokenFetchTimeoutSeconds));
 }
 
 DiceResponseHandler::DiceTokenFetcher::~DiceTokenFetcher() {}
 
+void DiceResponseHandler::DiceTokenFetcher::OnTimeout() {
+  gaia_auth_fetcher_.reset();
+  timeout_closure_.Cancel();
+  dice_response_handler_->OnTokenExchangeFailure(
+      this, GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
+  // |this| may be deleted at this point.
+}
+
 void DiceResponseHandler::DiceTokenFetcher::OnClientOAuthSuccess(
     const GaiaAuthConsumer::ClientOAuthResult& result) {
+  gaia_auth_fetcher_.reset();
+  timeout_closure_.Cancel();
   dice_response_handler_->OnTokenExchangeSuccess(this, gaia_id_, email_,
                                                  result);
+  // |this| may be deleted at this point.
 }
 
 void DiceResponseHandler::DiceTokenFetcher::OnClientOAuthFailure(
     const GoogleServiceAuthError& error) {
+  gaia_auth_fetcher_.reset();
+  timeout_closure_.Cancel();
   dice_response_handler_->OnTokenExchangeFailure(this, error);
+  // |this| may be deleted at this point.
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -147,6 +172,10 @@ void DiceResponseHandler::ProcessDiceHeader(
   return;
 }
 
+size_t DiceResponseHandler::GetPendingDiceTokenFetchersCountForTesting() const {
+  return token_fetchers_.size();
+}
+
 void DiceResponseHandler::ProcessDiceSigninHeader(
     const std::string& gaia_id,
     const std::string& email,

chrome/browser/signin/dice_response_handler.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/cancelable_callback.h"
 #include "base/macros.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "google_apis/gaia/gaia_auth_consumer.h"
@@ -24,6 +25,9 @@ class SigninClient;
 class ProfileOAuth2TokenService;
 class Profile;
 
+// Exposed for testing.
+extern const int kDiceTokenFetchTimeoutSeconds;
+
 // Processes the Dice responses from Gaia.
 class DiceResponseHandler : public KeyedService {
  public:
@@ -39,6 +43,9 @@ class DiceResponseHandler : public KeyedService {
   // Must be called when receiving a Dice response header.
   void ProcessDiceHeader(const signin::DiceResponseParams& dice_params);
 
+  // Returns the number of pending DiceTokenFetchers. Exposed for testing.
+  size_t GetPendingDiceTokenFetchersCountForTesting() const;
+
  private:
   // Helper class to fetch a refresh token from an authorization code.
   class DiceTokenFetcher : public GaiaAuthConsumer {
@@ -57,6 +64,9 @@ class DiceResponseHandler : public KeyedService {
     }
 
    private:
+    // Called by |timeout_closure_| when the request times out.
+    void OnTimeout();
+
     // GaiaAuthConsumer implementation:
     void OnClientOAuthSuccess(
         const GaiaAuthConsumer::ClientOAuthResult& result) override;
@@ -66,6 +76,7 @@ class DiceResponseHandler : public KeyedService {
     std::string email_;
     std::string authorization_code_;
     DiceResponseHandler* dice_response_handler_;
+    base::CancelableClosure timeout_closure_;
     std::unique_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
 
     DISALLOW_COPY_AND_ASSIGN(DiceTokenFetcher);

chrome/browser/signin/dice_response_handler_unittest.cc

@@ -7,7 +7,9 @@
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
-#include "base/test/test_simple_task_runner.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
@@ -59,7 +61,8 @@ class DiceTestSigninClient : public TestSigninClient, public GaiaAuthConsumer {
 class DiceResponseHandlerTest : public testing::Test {
  protected:
   DiceResponseHandlerTest()
-      : task_runner_(new base::TestSimpleTaskRunner()),
+      : loop_(base::MessageLoop::TYPE_IO),  // URLRequestContext requires IO.
+        task_runner_(new base::TestMockTimeTaskRunner()),
         request_context_getter_(
             new net::TestURLRequestContextGetter(task_runner_)),
         signin_client_(&pref_service_),
@@ -68,6 +71,8 @@ class DiceResponseHandlerTest : public testing::Test {
         dice_response_handler_(&signin_client_,
                                &token_service_,
                                &account_tracker_service_) {
+    loop_.SetTaskRunner(task_runner_);
+    DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get());
     switches::EnableAccountConsistencyDiceForTesting(
         base::CommandLine::ForCurrentProcess());
     signin_client_.SetURLRequestContext(request_context_getter_.get());
@@ -88,7 +93,7 @@ class DiceResponseHandlerTest : public testing::Test {
   }
 
   base::MessageLoop loop_;
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
   sync_preferences::TestingPrefServiceSyncable pref_service_;
   DiceTestSigninClient signin_client_;
@@ -170,6 +175,22 @@ TEST_F(DiceResponseHandlerTest, SigninWithTwoAccounts) {
       token_service_.RefreshTokenIsAvailable(dice_params_2.obfuscated_gaia_id));
 }
 
+TEST_F(DiceResponseHandlerTest, Timeout) {
+  DiceResponseParams dice_params = MakeDiceParams(DiceAction::SIGNIN);
+  ASSERT_FALSE(
+      token_service_.RefreshTokenIsAvailable(dice_params.obfuscated_gaia_id));
+  dice_response_handler_.ProcessDiceHeader(dice_params);
+  // Check that a GaiaAuthFetcher has been created.
+  ASSERT_THAT(signin_client_.consumer_, testing::NotNull());
+  EXPECT_EQ(
+      1u, dice_response_handler_.GetPendingDiceTokenFetchersCountForTesting());
+  // Force a timeout.
+  task_runner_->FastForwardBy(
+      base::TimeDelta::FromSeconds(kDiceTokenFetchTimeoutSeconds + 1));
+  EXPECT_EQ(
+      0u, dice_response_handler_.GetPendingDiceTokenFetchersCountForTesting());
+}
+
 // Tests that the DiceResponseHandler is created for a normal profile but not
 // for an incognito profile.
 TEST(DiceResponseHandlerFactoryTest, NotInIncognito) {