sync: change semantics (and name) of SigninManager::GetUsername

chrome/browser/sync/signin_manager.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/signin_manager.h"
 
 #include "base/command_line.h"
 #include "base/string_util.h"
 #include "chrome/browser/net/gaia/token_service.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ skipping 21 matching lines @@
   user_prefs->RegisterStringPref(prefs::kGoogleServicesUsername,
                                  "",
                                  PrefService::UNSYNCABLE_PREF);
   user_prefs->RegisterBooleanPref(prefs::kAutologinEnabled,
                                   true,
                                   PrefService::UNSYNCABLE_PREF);
 }
 
 void SigninManager::Initialize(Profile* profile) {
   profile_ = profile;
-  SetUsername(profile_->GetPrefs()->GetString(prefs::kGoogleServicesUsername));
+
+  std::string user = profile_->GetPrefs()->GetString(
+      prefs::kGoogleServicesUsername);
+  if (!user.empty())
+    SetAuthenticatedUsername(user);
   profile_->GetTokenService()->Initialize(
       GaiaConstants::kChromeSource, profile_);
-  if (!GetUsername().empty()) {
+  if (!authenticated_username_.empty() &&

[Andrew T Wilson (Slow), 2011/11/28 04:54:02]

Would the token service ever actually have a token for the user prior to
Initialize() being called here? I'm curious about what this new if clause is
for.

[tim (not reviewing), 2011/12/05 21:24:50]

Some tests were catching this since I think they add tokens before calling setup
to satisfy some AreCredentialsAvaialable checks.  I'll see if that makes sense,
if they should change in this CL or I should file a bug here.

+      !profile_->GetTokenService()->HasTokenForService(

[lipalani1, 2011/12/05 21:56:57]

I heard other services also depend on the tokens obtained from here. So if sync
is eventually disabled by the user the other tokens will not load. We probably
dont want that.

[tim (not reviewing), 2011/12/06 02:18:22]

Those services shouldn't depend on sync calling LoadTokens though, right?  It
should probably self-check internally, I believe what happens is some tests that
use a WebDatabase (e.g. PSSAutofill) wind up seeing tasks get posted that aren't
expected and hence run after the test exits, which causes a crash.  Looking to
see if I can avoid that another way

+          browser_sync::SyncServiceName())) {
     profile_->GetTokenService()->LoadTokensFromDB();
   }
 }
 
 bool SigninManager::IsInitialized() const {
   return profile_ != NULL;
 }
 
 void SigninManager::CleanupNotificationRegistration() {
 #if !defined(OS_CHROMEOS)
   content::Source<TokenService> token_service(profile_->GetTokenService());
   if (registrar_.IsRegistered(this,
                               chrome::NOTIFICATION_TOKEN_AVAILABLE,
                               token_service)) {
     registrar_.Remove(this,
                       chrome::NOTIFICATION_TOKEN_AVAILABLE,
                       token_service);
   }
 #endif
 }
 
-// If a username already exists, the user is logged in.
-const std::string& SigninManager::GetUsername() {
-  return browser_sync::IsUsingOAuth() ? oauth_username_ : username_;
+const std::string& SigninManager::GetAuthenticatedUsername() {
+  return authenticated_username_;
 }
 
-void SigninManager::SetUsername(const std::string& username) {
-  if (browser_sync::IsUsingOAuth())
-    oauth_username_ = username;
-  else
-    username_ = username;
+void SigninManager::SetAuthenticatedUsername(const std::string& username) {
+  DCHECK(authenticated_username_.empty());
+  authenticated_username_ = username;
+  // TODO(tim): We could go further in ensuring kGoogleServicesUsername and
+  // authenticated_username_ are consistent once established (e.g. remove
+  // authenticated_username_ altogether).
 }
 
-// static
 void SigninManager::PrepareForSignin() {
   DCHECK(!browser_sync::IsUsingOAuth());
-  DCHECK(username_.empty());
+  DCHECK(possibly_invalid_username_.empty());
+  // This attempt is either 1) the user trying to establish initial sync, or
+  // 2) trying to refresh credentials for an existing username.  If it is 2, we
+  // need to try again, but take care to leave state around tracking that the
+  // user has successfully signed in once before with this username, so that on
+  // restart we don't think sync setup has never completed.
+  ClearTransientSigninData();
 }
 
-// static
 void SigninManager::PrepareForOAuthSignin() {
   DCHECK(browser_sync::IsUsingOAuth());
-  DCHECK(oauth_username_.empty());
 #if !defined(OS_CHROMEOS)
   // The Sign out should clear the token service credentials.
   // Note: In CHROMEOS we might have valid credentials but still need to
   // set up 2-factor authentication.
   DCHECK(!profile_->GetTokenService()->HasOAuthCredentials());
 #endif
+  ClearTransientSigninData();
 }
 
 // Users must always sign out before they sign in again.
 void SigninManager::StartOAuthSignIn(const std::string& oauth1_request_token) {
   DCHECK(browser_sync::IsUsingOAuth());
   PrepareForOAuthSignin();
   oauth1_request_token_.assign(oauth1_request_token);
   oauth_login_.reset(new GaiaOAuthFetcher(this,
                                           profile_->GetRequestContext(),
                                           profile_,
                                           GaiaConstants::kSyncServiceOAuth));
   oauth_login_->StartOAuthGetAccessToken(oauth1_request_token_);
   // TODO(rogerta?): Bug 92325: Expand Autologin to include OAuth signin
 }
 
 // Users must always sign out before they sign in again.
 void SigninManager::StartSignIn(const std::string& username,
                                 const std::string& password,
                                 const std::string& login_token,
                                 const std::string& login_captcha) {
   DCHECK(!browser_sync::IsUsingOAuth());
+  DCHECK(authenticated_username_.empty() ||
+         username == authenticated_username_);
   PrepareForSignin();
-  username_.assign(username);
+  possibly_invalid_username_.assign(username);
   password_.assign(password);
 
   client_login_.reset(new GaiaAuthFetcher(this,
                                           GaiaConstants::kChromeSource,
                                           profile_->GetRequestContext()));
   client_login_->StartClientLogin(username,
                                   password,
                                   "",
                                   login_token,
                                   login_captcha,
                                   GaiaAuthFetcher::HostedAccountsNotAllowed);
 
   // Register for token availability.  The signin manager will pre-login the
   // user when the GAIA service token is ready for use.  Only do this if we
   // are not running in ChomiumOS, since it handles pre-login itself.
 #if !defined(OS_CHROMEOS)
   registrar_.Add(this,
                  chrome::NOTIFICATION_TOKEN_AVAILABLE,
                  content::Source<TokenService>(profile_->GetTokenService()));
 #endif
 }
 
 void SigninManager::ProvideSecondFactorAccessCode(
     const std::string& access_code) {
   DCHECK(!browser_sync::IsUsingOAuth());
-  DCHECK(!username_.empty() && !password_.empty() &&
+  DCHECK(!possibly_invalid_username_.empty() && !password_.empty() &&
       last_result_.data.empty());
 
   client_login_.reset(new GaiaAuthFetcher(this,
                                           GaiaConstants::kChromeSource,
                                           profile_->GetRequestContext()));
-  client_login_->StartClientLogin(username_,
+  client_login_->StartClientLogin(possibly_invalid_username_,
                                   access_code,
                                   "",
                                   std::string(),
                                   std::string(),
                                   GaiaAuthFetcher::HostedAccountsNotAllowed);
 }
 
-void SigninManager::ClearInMemoryData() {
+void SigninManager::ClearTransientSigninData() {
   if (!profile_)
     return;
 
   CleanupNotificationRegistration();
   client_login_.reset();
   last_result_ = ClientLoginResult();
-  username_.clear();
-  oauth_username_.clear();
+  possibly_invalid_username_.clear();

[lipalani1, 2011/12/05 21:56:57]

Without clearing the AuthenticateduserName the PSS' AreCredentialsAvailable will
return true even if there is an auth error.

Which is probably fine but worth pointing out to make sure you have done the due
diligence it is fine. Also may be we can add a comment the method
AreCredentialsAvailable to say this does not mean the credentials are current.

   password_.clear();
   had_two_factor_error_ = false;
 }
 
 void SigninManager::SignOut() {
   if (!profile_)
     return;
 
-  ClearInMemoryData();
+  ClearTransientSigninData();
+  authenticated_username_.clear();
   profile_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
   profile_->GetPrefs()->ClearPref(prefs::kSyncUsingOAuth);
   profile_->GetPrefs()->ScheduleSavePersistentPrefs();
   profile_->GetTokenService()->ResetCredentialsInMemory();
   profile_->GetTokenService()->EraseTokensFromDB();
 }
 
 void SigninManager::OnClientLoginSuccess(const ClientLoginResult& result) {
   DCHECK(!browser_sync::IsUsingOAuth());
   last_result_ = result;
   // Make a request for the canonical email address.
   client_login_->StartGetUserInfo(result.lsid, kGetInfoEmailKey);
 }
 
 // NOTE: GetUserInfo is a ClientLogin request similar to OAuth's userinfo
 void SigninManager::OnGetUserInfoSuccess(const std::string& key,
                                          const std::string& value) {
   DCHECK(!browser_sync::IsUsingOAuth());
   DCHECK(key == kGetInfoEmailKey);
+  DCHECK(authenticated_username_.empty() || authenticated_username_ == value);
 
-  username_ = value;
-  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, username_);
+  authenticated_username_ = value;
+  possibly_invalid_username_.clear();
+  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername,
+                                  authenticated_username_);
   profile_->GetPrefs()->SetBoolean(prefs::kSyncUsingOAuth, false);
   profile_->GetPrefs()->ScheduleSavePersistentPrefs();
 
-  GoogleServiceSigninSuccessDetails details(username_, password_);
+  GoogleServiceSigninSuccessDetails details(authenticated_username_,
+                                            password_);
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL,
       content::Source<Profile>(profile_),
       content::Details<const GoogleServiceSigninSuccessDetails>(&details));
 
   password_.clear();  // Don't need it anymore.
 
   profile_->GetTokenService()->UpdateCredentials(last_result_);
   DCHECK(profile_->GetTokenService()->AreCredentialsValid());
   profile_->GetTokenService()->StartFetchingTokens();
@@ skipping 33 matching lines @@
   // a second factor error, and we don't sign out if we're dealing with
   // an invalid access code (again, because the password was valid).
   bool invalid_gaia = error.state() ==
       GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS;
   if (error.state() == GoogleServiceAuthError::TWO_FACTOR ||
       (had_two_factor_error_ && invalid_gaia)) {
     had_two_factor_error_ = true;
     return;
   }
 
-  ClearInMemoryData();
+  ClearTransientSigninData();
 }
 
 void SigninManager::OnOAuthGetAccessTokenSuccess(const std::string& token,
                                                  const std::string& secret) {
   DCHECK(browser_sync::IsUsingOAuth());
   VLOG(1) << "SigninManager::OnOAuthGetAccessTokenSuccess";
   profile_->GetTokenService()->UpdateOAuthCredentials(token, secret);
 }
 
 void SigninManager::OnOAuthGetAccessTokenFailure(
@@ skipping 22 matching lines @@
 
   TokenService* token_service = profile_->GetTokenService();
   CHECK(token_service);
 
   // If |SignOut()| was called between the login start and |OnUserInfoSucess()|,
   // then the OAuth credentials would have been cleared.
   if (!token_service->HasOAuthCredentials())
     return;
 
   VLOG(1) << "Sync signin for " << email << " is complete.";
-  oauth_username_ = email;
+  authenticated_username_ = email;
   profile_->GetPrefs()->SetString(
-      prefs::kGoogleServicesUsername, oauth_username_);
+      prefs::kGoogleServicesUsername, authenticated_username_);
   profile_->GetPrefs()->SetBoolean(prefs::kSyncUsingOAuth, true);
   profile_->GetPrefs()->ScheduleSavePersistentPrefs();
 
   DCHECK(password_.empty());
-  GoogleServiceSigninSuccessDetails details(oauth_username_, "");
+  GoogleServiceSigninSuccessDetails details(authenticated_username_, "");
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL,
       content::Source<Profile>(profile_),
       content::Details<const GoogleServiceSigninSuccessDetails>(&details));
 
   DCHECK(token_service->HasOAuthCredentials());
   token_service->StartFetchingOAuthTokens();
 }
 
 void SigninManager::OnUserInfoFailure(const GoogleServiceAuthError& error) {
@@ skipping 19 matching lines @@
                                               profile_->GetRequestContext()));
     }
 
     client_login_->StartMergeSession(tok_details->token());
 
     // We only want to do this once per sign-in.
     CleanupNotificationRegistration();
   }
 #endif
 }