[signin] Stop using empty refresh tokens

When loading a token fails, kInvalidRefreshToken is used instead of
the empty token.
For such accounts, RefreshTokenIsAvailable() and  RefreshTokenHasError()
now return true.

Change-Id: I0f206a183027162153b5a8de52e4c87c3462e84c
Reviewed-on: https://chromium-review.googlesource.com/1107979Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571429}

chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc

@@ -43,6 +43,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_downloader.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -60,17 +61,19 @@
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/user_manager/scoped_user_manager.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_image/user_image.h"
 #include "components/user_manager/user_manager.h"
 #include "crypto/rsa_private_key.h"
 #include "google_apis/gaia/gaia_oauth_client.h"
+#include "google_apis/gaia/gaia_urls.h"
 #include "google_apis/gaia/oauth2_token_service.h"
+#include "net/test/embedded_test_server/controllable_http_response.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
-#include "net/test/embedded_test_server/simple_connection_listener.h"
 #include "net/url_request/url_request_status.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -138,7 +141,7 @@ class UserImageChangeWaiter : public user_manager::UserManager::Observer {
 class UserImageManagerTest : public LoginManagerTest,
                              public user_manager::UserManager::Observer {
  public:
-  std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
+  std::unique_ptr<net::test_server::BasicHttpResponse> HandleRequest(
       const net::test_server::HttpRequest& request) {
     if (request.relative_url != "/avatar.jpg")
       return nullptr;
@@ -164,7 +167,7 @@ class UserImageManagerTest : public LoginManagerTest,
     response->set_content_type("image/jpeg");
     response->set_code(net::HTTP_OK);
     response->set_content(profile_image_data);
-    return std::move(response);
+    return response;
   }
 
  protected:
@@ -190,18 +193,24 @@ class UserImageManagerTest : public LoginManagerTest,
 
   void SetUpOnMainThread() override {
     // Set up the test server.
-    embedded_test_server()->RegisterRequestHandler(base::BindRepeating(
-        &UserImageManagerTest::HandleRequest, base::Unretained(this)));
-    connection_listener_ =
-        std::make_unique<net::test_server::SimpleConnectionListener>(
-            1, net::test_server::SimpleConnectionListener::
-                   ALLOW_ADDITIONAL_CONNECTIONS);
-    embedded_test_server()->SetConnectionListener(connection_listener_.get());
+    controllable_http_response_ =
+        std::make_unique<net::test_server::ControllableHttpResponse>(
+            embedded_test_server(), "/avatar.jpg",
+            true /*relative_url_is_prefix*/);
     ASSERT_TRUE(embedded_test_server()->Started());
 
     LoginManagerTest::SetUpOnMainThread();
     local_state_ = g_browser_process->local_state();
     user_manager::UserManager::Get()->AddObserver(this);
+
+    // FakeGaia authorizes requests for profile info.
+    FakeGaia::AccessTokenInfo token_info;
+    token_info.any_scope = true;
+    token_info.audience = GaiaUrls::GetInstance()->oauth2_chrome_client_id();
+    token_info.token = kRandomTokenStrForTesting;
+    token_info.email = test_account_id1_.GetUserEmail();
+    fake_gaia_.IssueOAuthToken(kRandomTokenStrForTesting, token_info);
+    fake_gaia_.MapEmailToGaiaId(kTestUser1, kTestUser1GaiaId);
   }
 
   void TearDownOnMainThread() override {
@@ -280,31 +289,16 @@ class UserImageManagerTest : public LoginManagerTest,
     AccountTrackerServiceFactory::GetForProfile(profile)->SeedAccountInfo(info);
   }
 
-  // Triggers the download of all non-image profile data for the user
-  // |account_id|. This method must only be called after a profile data
-  // download has been started.
-  // The |test_server::EmbeddedTestServer| instance installed on the caller
-  // side will capture the net::SimpleURLLoader created by the ProfileDownloader
-  // to download the profile image.
-  void CompleteProfileMetadataDownload(const AccountId& account_id) {
-    ProfileDownloader* profile_downloader =
-        reinterpret_cast<UserImageManagerImpl*>(
-            ChromeUserManager::Get()->GetUserImageManager(account_id))
-            ->profile_downloader_.get();
-    ASSERT_TRUE(profile_downloader);
-
-    static_cast<OAuth2TokenService::Consumer*>(profile_downloader)
-        ->OnGetTokenSuccess(NULL, kRandomTokenStrForTesting,
-                            base::Time::Now() + base::TimeDelta::FromDays(1));
-  }
-
   // Completes the download of the currently logged-in user's profile image.
   // This method must only be called after a profile data download including
-  // the profile image has been started, the download of all non-image data has
-  // been completed by calling CompleteProfileMetadataDownload() and the
-  // net::SimpleURLLoader created by the ProfileDownloader to download the
-  // profile image has been captured by |embedded_test_server()|.
+  // the profile image has been started.
   void CompleteProfileImageDownload() {
+    controllable_http_response_->WaitForRequest();
+    std::unique_ptr<net::test_server::BasicHttpResponse> response =
+        HandleRequest(*controllable_http_response_->http_request());
+    controllable_http_response_->Send(response->ToResponseString());
+    controllable_http_response_->Done();
+
     base::RunLoop run_loop;
     PrefChangeRegistrar pref_change_registrar;
     pref_change_registrar.Init(local_state_);
@@ -331,6 +325,9 @@ class UserImageManagerTest : public LoginManagerTest,
 
   std::unique_ptr<base::RunLoop> run_loop_;
 
+  std::unique_ptr<net::test_server::ControllableHttpResponse>
+      controllable_http_response_;
+
   const AccountId test_account_id1_ =
       AccountId::FromUserEmailGaiaId(kTestUser1, kTestUser1GaiaId);
   const AccountId test_account_id2_ =
@@ -340,9 +337,6 @@ class UserImageManagerTest : public LoginManagerTest,
   const cryptohome::Identification cryptohome_id_ =
       cryptohome::Identification(enterprise_account_id_);
 
-  std::unique_ptr<net::test_server::SimpleConnectionListener>
-      connection_listener_;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(UserImageManagerTest);
 };
@@ -532,6 +526,8 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveUserImageFromProfileImage) {
   UserImageManagerImpl::IgnoreProfileDataDownloadDelayForTesting();
   LoginUser(test_account_id1_);
   Profile* profile = ProfileHelper::Get()->GetProfileByUserUnsafe(user);
+  ProfileOAuth2TokenServiceFactory::GetForProfile(profile)->UpdateCredentials(
+      test_account_id1_.GetUserEmail(), kRandomTokenStrForTesting);
   SeedAccountTrackerService(test_account_id1_, profile);
 
   run_loop_.reset(new base::RunLoop);
@@ -540,8 +536,6 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, SaveUserImageFromProfileImage) {
   user_image_manager->SaveUserImageFromProfileImage();
   run_loop_->Run();
 
-  CompleteProfileMetadataDownload(test_account_id1_);
-  connection_listener_->WaitForConnections();
   CompleteProfileImageDownload();
 
   const gfx::ImageSkia& profile_image =
@@ -584,6 +578,8 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest,
   UserImageManagerImpl::IgnoreProfileDataDownloadDelayForTesting();
   LoginUser(test_account_id1_);
   Profile* profile = ProfileHelper::Get()->GetProfileByUserUnsafe(user);
+  ProfileOAuth2TokenServiceFactory::GetForProfile(profile)->UpdateCredentials(
+      test_account_id1_.GetUserEmail(), kRandomTokenStrForTesting);
   SeedAccountTrackerService(test_account_id1_, profile);
 
   run_loop_.reset(new base::RunLoop);
@@ -592,13 +588,9 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest,
   user_image_manager->SaveUserImageFromProfileImage();
   run_loop_->Run();
 
-  CompleteProfileMetadataDownload(test_account_id1_);
-
   user_image_manager->SaveUserDefaultImageIndex(
       default_user_image::kFirstDefaultImageIndex);
 
-  connection_listener_->WaitForConnections();
-
   CompleteProfileImageDownload();
 
   EXPECT_TRUE(user->HasDefaultImage());

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc

@@ -303,6 +303,7 @@ MutableProfileOAuth2TokenServiceDelegate::AccountStatus::AccountStatus(
       last_auth_error_(GoogleServiceAuthError::NONE) {
   DCHECK(signin_error_controller_);
   DCHECK(!account_id_.empty());
+  DCHECK(!refresh_token.empty());
 }
 
 void MutableProfileOAuth2TokenServiceDelegate::AccountStatus::Initialize() {
@@ -446,8 +447,11 @@ bool MutableProfileOAuth2TokenServiceDelegate::RefreshTokenIsAvailable(
 std::string MutableProfileOAuth2TokenServiceDelegate::GetRefreshToken(
     const std::string& account_id) const {
   AccountStatusMap::const_iterator iter = refresh_tokens_.find(account_id);
-  if (iter != refresh_tokens_.end())
-    return iter->second->refresh_token();
+  if (iter != refresh_tokens_.end()) {
+    const std::string refresh_token = iter->second->refresh_token();
+    DCHECK(!refresh_token.empty());
+    return refresh_token;
+  }
   return std::string();
 }
 
@@ -562,22 +566,18 @@ void MutableProfileOAuth2TokenServiceDelegate::OnWebDataServiceRequestDone(
       load_credentials_state_ =
           LOAD_CREDENTIALS_FINISHED_WITH_NO_TOKEN_FOR_PRIMARY_ACCOUNT;
     }
-    AddAccountStatus(loading_primary_account_id_, std::string(),
+    AddAccountStatus(loading_primary_account_id_, kInvalidRefreshToken,
                      GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
                          GoogleServiceAuthError::InvalidGaiaCredentialsReason::
                              CREDENTIALS_MISSING));
   }
 
-  // If we don't have a refresh token for a known account, signal an error.
+#ifndef NDEBUG
   for (auto& token : refresh_tokens_) {
-    if (!RefreshTokenIsAvailable(token.first)) {
-      UpdateAuthError(token.first,
-                      GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
-                          GoogleServiceAuthError::InvalidGaiaCredentialsReason::
-                              CREDENTIALS_MISSING));
-      break;
-    }
+    DCHECK(RefreshTokenIsAvailable(token.first))
+        << "Missing token for " << token.first;
   }
+#endif
 
   loading_primary_account_id_.clear();
   FireRefreshTokensLoaded();
@@ -784,6 +784,8 @@ void MutableProfileOAuth2TokenServiceDelegate::UpdateCredentialsInMemory(
 void MutableProfileOAuth2TokenServiceDelegate::PersistCredentials(
     const std::string& account_id,
     const std::string& refresh_token) {
+  DCHECK(!account_id.empty());
+  DCHECK(!refresh_token.empty());
   scoped_refptr<TokenWebData> token_web_data = client_->GetDatabase();
   if (token_web_data.get()) {
     VLOG(1) << "MutablePO2TS::PersistCredentials for account_id=" << account_id;
@@ -836,6 +838,7 @@ void MutableProfileOAuth2TokenServiceDelegate::RevokeCredentials(
 
 void MutableProfileOAuth2TokenServiceDelegate::ClearPersistedCredentials(
     const std::string& account_id) {
+  DCHECK(!account_id.empty());
   scoped_refptr<TokenWebData> token_web_data = client_->GetDatabase();
   if (token_web_data.get()) {
     VLOG(1) << "MutablePO2TS::ClearPersistedCredentials for account_id="
@@ -846,6 +849,8 @@ void MutableProfileOAuth2TokenServiceDelegate::ClearPersistedCredentials(
 
 void MutableProfileOAuth2TokenServiceDelegate::RevokeCredentialsOnServer(
     const std::string& refresh_token) {
+  DCHECK(!refresh_token.empty());
+
   if (refresh_token == kInvalidRefreshToken)
     return;
 

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc

@@ -385,9 +385,9 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   // LoadCredentials() guarantees that the account given to it as argument
   // is in the refresh_token map.
   EXPECT_EQ(1U, oauth2_service_delegate_->refresh_tokens_.size());
-  EXPECT_TRUE(oauth2_service_delegate_->refresh_tokens_["account_id"]
-                  ->refresh_token()
-                  .empty());
+  EXPECT_EQ(
+      MutableProfileOAuth2TokenServiceDelegate::kInvalidRefreshToken,
+      oauth2_service_delegate_->refresh_tokens_["account_id"]->refresh_token());
   // Setup a DB with tokens that don't require upgrade and clear memory.
   oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token");
   oauth2_service_delegate_->UpdateCredentials("account_id2", "refresh_token2");
@@ -671,8 +671,12 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   EXPECT_EQ(1, start_batch_changes_);
   EXPECT_EQ(1, end_batch_changes_);
   EXPECT_EQ(1, auth_error_changed_count_);
-  EXPECT_FALSE(oauth2_service_delegate_->RefreshTokenIsAvailable(
+  EXPECT_TRUE(oauth2_service_delegate_->RefreshTokenIsAvailable(
       primary_account.account_id));
+  EXPECT_EQ(
+      GoogleServiceAuthError::InvalidGaiaCredentialsReason::CREDENTIALS_MISSING,
+      oauth2_service_delegate_->GetAuthError(primary_account.account_id)
+          .GetInvalidGaiaCredentialsReason());
   EXPECT_EQ(OAuth2TokenServiceDelegate::
                 LOAD_CREDENTIALS_FINISHED_WITH_NO_TOKEN_FOR_PRIMARY_ACCOUNT,
             oauth2_service_delegate_->GetLoadCredentialsState());