[signin] Handle Dice token exchange failures

When the Dice token exchange fails:
- if this is a Sync signin, the tab is navigated to the NTP, to match
  the behavior of the success case
- if this is a Sync signin or if Dice is fully enabled for this profile,
  display the signin error dialog.

Bug: 789130
Change-Id: I4a5309a18301c4e3112db50296edd625682f0fd3
Reviewed-on: https://chromium-review.googlesource.com/810805
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522416}

chrome/browser/signin/dice_response_handler.cc

@@ -13,6 +13,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/signin/about_signin_internals_factory.h"
 #include "chrome/browser/signin/account_reconcilor_factory.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/chrome_signin_client_factory.h"
@@ -20,6 +21,7 @@
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "components/signin/core/browser/about_signin_internals.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/profile_management_switches.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
@@ -85,6 +87,7 @@ class DiceResponseHandlerFactory : public BrowserContextKeyedServiceFactory {
       : BrowserContextKeyedServiceFactory(
             "DiceResponseHandler",
             BrowserContextDependencyManager::GetInstance()) {
+    DependsOn(AboutSigninInternalsFactory::GetInstance());
     DependsOn(AccountReconcilorFactory::GetInstance());
     DependsOn(AccountTrackerServiceFactory::GetInstance());
     DependsOn(ChromeSigninClientFactory::GetInstance());
@@ -106,7 +109,8 @@ class DiceResponseHandlerFactory : public BrowserContextKeyedServiceFactory {
         SigninManagerFactory::GetForProfile(profile),
         ProfileOAuth2TokenServiceFactory::GetForProfile(profile),
         AccountTrackerServiceFactory::GetForProfile(profile),
-        AccountReconcilorFactory::GetForProfile(profile));
+        AccountReconcilorFactory::GetForProfile(profile),
+        AboutSigninInternalsFactory::GetForProfile(profile));
   }
 };
 
@@ -181,8 +185,7 @@ void DiceResponseHandler::DiceTokenFetcher::OnClientOAuthSuccess(
   RecordDiceFetchTokenResult(kFetchSuccess);
   gaia_auth_fetcher_.reset();
   timeout_closure_.Cancel();
-  dice_response_handler_->OnTokenExchangeSuccess(
-      this, gaia_id_, email_, result.refresh_token, std::move(delegate_));
+  dice_response_handler_->OnTokenExchangeSuccess(this, result.refresh_token);
   // |this| may be deleted at this point.
 }
 
@@ -209,17 +212,20 @@ DiceResponseHandler::DiceResponseHandler(
     SigninManager* signin_manager,
     ProfileOAuth2TokenService* profile_oauth2_token_service,
     AccountTrackerService* account_tracker_service,
-    AccountReconcilor* account_reconcilor)
+    AccountReconcilor* account_reconcilor,
+    AboutSigninInternals* about_signin_internals)
     : signin_manager_(signin_manager),
       signin_client_(signin_client),
       token_service_(profile_oauth2_token_service),
       account_tracker_service_(account_tracker_service),
-      account_reconcilor_(account_reconcilor) {
+      account_reconcilor_(account_reconcilor),
+      about_signin_internals_(about_signin_internals) {
   DCHECK(signin_client_);
   DCHECK(signin_manager_);
   DCHECK(token_service_);
   DCHECK(account_tracker_service_);
   DCHECK(account_reconcilor_);
+  DCHECK(about_signin_internals_);
 }
 
 DiceResponseHandler::~DiceResponseHandler() {}
@@ -392,26 +398,34 @@ void DiceResponseHandler::DeleteTokenFetcher(DiceTokenFetcher* token_fetcher) {
 
 void DiceResponseHandler::OnTokenExchangeSuccess(
     DiceTokenFetcher* token_fetcher,
-    std::string gaia_id,
-    std::string email,
-    std::string refresh_token,
-    std::unique_ptr<ProcessDiceHeaderDelegate> delegate) {
+    const std::string& refresh_token) {
+  const std::string& email = token_fetcher->email();
+  const std::string& gaia_id = token_fetcher->gaia_id();
   if (!CanGetTokenForAccount(gaia_id, email))
     return;
   VLOG(1) << "[Dice] OAuth success for email " << email;
   bool should_enable_sync = token_fetcher->should_enable_sync();
-  DeleteTokenFetcher(token_fetcher);
   std::string account_id =
       account_tracker_service_->SeedAccountInfo(gaia_id, email);
   token_service_->UpdateCredentials(account_id, refresh_token);
+  about_signin_internals_->OnRefreshTokenReceived(
+      base::StringPrintf("Successful (%s)", account_id.c_str()));
   if (should_enable_sync)
-    delegate->EnableSync(account_id);
+    token_fetcher->delegate()->EnableSync(account_id);
+
+  DeleteTokenFetcher(token_fetcher);
 }
 
 void DiceResponseHandler::OnTokenExchangeFailure(
     DiceTokenFetcher* token_fetcher,
     const GoogleServiceAuthError& error) {
-  // TODO(droger): Handle authentication errors.
-  VLOG(1) << "[Dice] OAuth failed with error: " << error.ToString();
+  const std::string& email = token_fetcher->email();
+  const std::string& gaia_id = token_fetcher->gaia_id();
+  std::string account_id =
+      account_tracker_service_->PickAccountIdForAccount(gaia_id, email);
+  about_signin_internals_->OnRefreshTokenReceived(
+      base::StringPrintf("Failure (%s)", account_id.c_str()));
+  token_fetcher->delegate()->HandleTokenExchangeFailure(email, error);
+
   DeleteTokenFetcher(token_fetcher);
 }

chrome/browser/signin/dice_response_handler.h

@@ -16,6 +16,7 @@
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "google_apis/gaia/gaia_auth_consumer.h"
 
+class AboutSigninInternals;
 class AccountTrackerService;
 class GaiaAuthFetcher;
 class GoogleServiceAuthError;
@@ -36,6 +37,11 @@ class ProcessDiceHeaderDelegate {
   // Called after the account was seeded in the account tracker service and
   // after the refresh token was fetched and updated in the token service.
   virtual void EnableSync(const std::string& account_id) = 0;
+
+  // Handles a failure in the token exchange (i.e. shows the error to the user).
+  virtual void HandleTokenExchangeFailure(
+      const std::string& email,
+      const GoogleServiceAuthError& error) = 0;
 };
 
 // Processes the Dice responses from Gaia.
@@ -49,7 +55,8 @@ class DiceResponseHandler : public KeyedService {
                       SigninManager* signin_manager,
                       ProfileOAuth2TokenService* profile_oauth2_token_service,
                       AccountTrackerService* account_tracker_service,
-                      AccountReconcilor* account_reconcilor);
+                      AccountReconcilor* account_reconcilor,
+                      AboutSigninInternals* about_signin_internals);
   ~DiceResponseHandler() override;
 
   // Must be called when receiving a Dice response header.
@@ -81,6 +88,7 @@ class DiceResponseHandler : public KeyedService {
     void set_should_enable_sync(bool should_enable_sync) {
       should_enable_sync_ = should_enable_sync;
     }
+    ProcessDiceHeaderDelegate* delegate() { return delegate_.get(); }
 
    private:
     // Called by |timeout_closure_| when the request times out.
@@ -134,12 +142,8 @@ class DiceResponseHandler : public KeyedService {
 
   // Called after exchanging an OAuth 2.0 authorization code for a refresh token
   // after DiceAction::SIGNIN.
-  void OnTokenExchangeSuccess(
-      DiceTokenFetcher* token_fetcher,
-      std::string gaia_id,
-      std::string email,
-      std::string refresh_token,
-      std::unique_ptr<ProcessDiceHeaderDelegate> delegate);
+  void OnTokenExchangeSuccess(DiceTokenFetcher* token_fetcher,
+                              const std::string& refresh_token);
   void OnTokenExchangeFailure(DiceTokenFetcher* token_fetcher,
                               const GoogleServiceAuthError& error);
 
@@ -148,6 +152,7 @@ class DiceResponseHandler : public KeyedService {
   ProfileOAuth2TokenService* token_service_;
   AccountTrackerService* account_tracker_service_;
   AccountReconcilor* account_reconcilor_;
+  AboutSigninInternals* about_signin_internals_;
   std::vector<std::unique_ptr<DiceTokenFetcher>> token_fetchers_;
 
   DISALLOW_COPY_AND_ASSIGN(DiceResponseHandler);

chrome/browser/signin/dice_response_handler_unittest.cc

@@ -17,18 +17,22 @@
 #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/about_signin_internals.h"
 #include "components/signin/core/browser/account_reconcilor.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/dice_account_reconcilor_delegate.h"
+#include "components/signin/core/browser/fake_gaia_cookie_manager_service.h"
 #include "components/signin/core/browser/fake_signin_manager.h"
 #include "components/signin/core/browser/profile_management_switches.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/scoped_account_consistency.h"
+#include "components/signin/core/browser/signin_error_controller.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "components/signin/core/browser/test_signin_client.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/gaia/fake_oauth2_token_service_delegate.h"
+#include "google_apis/gaia/gaia_constants.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -101,6 +105,12 @@ class DiceResponseHandlerTest : public testing::Test,
     enable_sync_account_id_ = account_id;
   }
 
+  void HandleTokenExchangeFailure(const std::string& email,
+                                  const GoogleServiceAuthError& error) {
+    auth_error_email_ = email;
+    auth_error_ = error;
+  }
+
  protected:
   DiceResponseHandlerTest()
       : loop_(base::MessageLoop::TYPE_IO),  // URLRequestContext requires IO.
@@ -114,11 +124,20 @@ class DiceResponseHandlerTest : public testing::Test,
                         &token_service_,
                         &account_tracker_service_,
                         nullptr),
+        cookie_service_(&token_service_,
+                        GaiaConstants::kChromeSource,
+                        &signin_client_),
+        about_signin_internals_(&token_service_,
+                                &account_tracker_service_,
+                                &signin_manager_,
+                                &signin_error_controller_,
+                                &cookie_service_),
         reconcilor_blocked_count_(0),
         reconcilor_unblocked_count_(0) {
     loop_.SetTaskRunner(task_runner_);
     DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get());
     signin_client_.SetURLRequestContext(request_context_getter_.get());
+    AboutSigninInternals::RegisterPrefs(pref_service_.registry());
     AccountTrackerService::RegisterPrefs(pref_service_.registry());
     SigninManager::RegisterProfilePrefs(pref_service_.registry());
     signin::RegisterAccountConsistencyProfilePrefs(pref_service_.registry());
@@ -129,9 +148,11 @@ class DiceResponseHandlerTest : public testing::Test,
     account_reconcilor_ = std::make_unique<AccountReconcilor>(
         &token_service_, &signin_manager_, &signin_client_, nullptr,
         std::move(account_reconcilor_delegate));
+    about_signin_internals_.Initialize(&signin_client_);
     dice_response_handler_ = std::make_unique<DiceResponseHandler>(
         &signin_client_, &signin_manager_, &token_service_,
-        &account_tracker_service_, account_reconcilor_.get());
+        &account_tracker_service_, account_reconcilor_.get(),
+        &about_signin_internals_);
 
     account_tracker_service_.Initialize(&signin_client_);
     account_reconcilor_->AddObserver(this);
@@ -139,6 +160,14 @@ class DiceResponseHandlerTest : public testing::Test,
 
   ~DiceResponseHandlerTest() override {
     account_reconcilor_->RemoveObserver(this);
+    account_reconcilor_->Shutdown();
+    about_signin_internals_.Shutdown();
+    cookie_service_.Shutdown();
+    signin_error_controller_.Shutdown();
+    signin_manager_.Shutdown();
+    account_tracker_service_.Shutdown();
+    token_service_.Shutdown();
+    signin_client_.Shutdown();
     task_runner_->ClearPendingTasks();
   }
 
@@ -185,11 +214,16 @@ class DiceResponseHandlerTest : public testing::Test,
   ProfileOAuth2TokenService token_service_;
   AccountTrackerService account_tracker_service_;
   FakeSigninManager signin_manager_;
+  SigninErrorController signin_error_controller_;
+  FakeGaiaCookieManagerService cookie_service_;
+  AboutSigninInternals about_signin_internals_;
   std::unique_ptr<AccountReconcilor> account_reconcilor_;
   std::unique_ptr<DiceResponseHandler> dice_response_handler_;
   int reconcilor_blocked_count_;
   int reconcilor_unblocked_count_;
   std::string enable_sync_account_id_;
+  GoogleServiceAuthError auth_error_;
+  std::string auth_error_email_;
 };
 
 class TestProcessDiceHeaderDelegate : public ProcessDiceHeaderDelegate {
@@ -204,6 +238,12 @@ class TestProcessDiceHeaderDelegate : public ProcessDiceHeaderDelegate {
     owner_->EnableSync(account_id);
   }
 
+  void HandleTokenExchangeFailure(
+      const std::string& email,
+      const GoogleServiceAuthError& error) override {
+    owner_->HandleTokenExchangeFailure(email, error);
+  }
+
  private:
   DiceResponseHandlerTest* owner_;
 };
@@ -228,6 +268,8 @@ TEST_F(DiceResponseHandlerTest, Signin) {
                                           false /* is_child_account */));
   // Check that the token has been inserted in the token service.
   EXPECT_TRUE(token_service_.RefreshTokenIsAvailable(account_id));
+  EXPECT_TRUE(auth_error_email_.empty());
+  EXPECT_EQ(GoogleServiceAuthError::NONE, auth_error_.state());
   // Check that the reconcilor was blocked and unblocked exactly once.
   EXPECT_EQ(1, reconcilor_blocked_count_);
   EXPECT_EQ(1, reconcilor_unblocked_count_);
@@ -248,12 +290,16 @@ TEST_F(DiceResponseHandlerTest, SigninFailure) {
   EXPECT_EQ(
       1u, dice_response_handler_->GetPendingDiceTokenFetchersCountForTesting());
   // Simulate GaiaAuthFetcher failure.
+  GoogleServiceAuthError::State error_state =
+      GoogleServiceAuthError::SERVICE_UNAVAILABLE;
   signin_client_.consumer_->OnClientOAuthFailure(
-      GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
+      GoogleServiceAuthError(error_state));
   EXPECT_EQ(
       0u, dice_response_handler_->GetPendingDiceTokenFetchersCountForTesting());
   // Check that the token has not been inserted in the token service.
   EXPECT_FALSE(token_service_.RefreshTokenIsAvailable(account_id));
+  EXPECT_EQ(account_info.email, auth_error_email_);
+  EXPECT_EQ(error_state, auth_error_.state());
 }
 
 // Checks that a second token for the same account is not requested when a

chrome/browser/signin/process_dice_header_delegate_impl.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/signin/process_dice_header_delegate_impl.h"
 
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/dice_tab_helper.h"
@@ -12,6 +14,8 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/webui_url_constants.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/profile_management_switches.h"
@@ -91,7 +95,6 @@ void ProcessDiceHeaderDelegateImpl::EnableSync(const std::string& account_id) {
   Browser* browser = nullptr;
   if (web_contents) {
     browser = chrome::FindBrowserWithWebContents(web_contents);
-
     // After signing in to Chrome, the user should be redirected to the NTP.
     RedirectToNtp(web_contents);
   }
@@ -102,3 +105,25 @@ void ProcessDiceHeaderDelegateImpl::EnableSync(const std::string& account_id) {
   new DiceTurnSyncOnHelper(profile_, browser, signin_access_point_,
                            signin_reason_, account_id);
 }
+
+void ProcessDiceHeaderDelegateImpl::HandleTokenExchangeFailure(
+    const std::string& email,
+    const GoogleServiceAuthError& error) {
+  DCHECK_NE(GoogleServiceAuthError::NONE, error.state());
+  bool should_enable_sync = ShouldEnableSync();
+  if (!should_enable_sync &&
+      !signin::IsDiceEnabledForProfile(profile_->GetPrefs())) {
+    return;
+  }
+
+  // Show pop up-dialog.
+  content::WebContents* web_contents = this->web_contents();
+  Browser* browser = web_contents
+                         ? chrome::FindBrowserWithWebContents(web_contents)
+                         : chrome::FindBrowserWithProfile(profile_);
+  LoginUIServiceFactory::GetForProfile(profile_)->DisplayLoginResult(
+      browser, base::UTF8ToUTF16(error.ToString()), base::UTF8ToUTF16(email));
+
+  if (should_enable_sync && web_contents)
+    RedirectToNtp(web_contents);
+}

chrome/browser/signin/process_dice_header_delegate_impl.h

@@ -21,6 +21,8 @@ class ProcessDiceHeaderDelegateImpl : public ProcessDiceHeaderDelegate,
 
   // ProcessDiceHeaderDelegate:
   void EnableSync(const std::string& account_id) override;
+  void HandleTokenExchangeFailure(const std::string& email,
+                                  const GoogleServiceAuthError& error) override;
 
  private:
   // Returns true if sync should be enabled after the user signs in.