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

chrome/browser/sync/signin_manager.h

 // 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.
 //
 // The signin manager encapsulates some functionality tracking
 // which user is signed in. When a user is signed in, a ClientLogin
 // request is run on their behalf. Auth tokens are fetched from Google
 // and the results are stored in the TokenService.
+//
+// **NOTE** on semantics of SigninManager:
+//
+// Once a signin is successful, the username becomes "established" and will not
+// be cleared until a SignOut operation is performed (persists across
+// restarts). Until that happens, the signin manager can still be used to
+// refresh credentials, but changing the username is not permitted.
 
 #ifndef CHROME_BROWSER_SYNC_SIGNIN_MANAGER_H_
 #define CHROME_BROWSER_SYNC_SIGNIN_MANAGER_H_
 #pragma once
 
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 26 matching lines @@
   SigninManager();
   virtual ~SigninManager();
 
   // Call to register our prefs.
   static void RegisterUserPrefs(PrefService* user_prefs);
 
   // If user was signed in, load tokens from DB if available.
   void Initialize(Profile* profile);
   bool IsInitialized() const;
 
-  // If a user is signed in, this will return their name.
+  // If a user has previously established a username and SignOut has not been

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

To be clear, "established a username" == "has had a successful invocation of
StartSignIn OR is running CrOS which calls SetAuthenticatedUsername()"?

[tim (not reviewing), 2011/11/28 05:30:21]

Established means validated / authenticated.  It can be either of the cases you
mention, though I'd like to get rid of SetAuthenticatedUsername (see TODO) and
pass it on construction, if it fits the paradigm better for platforms where the
valid username is known a priori.

[Andrew T Wilson (Slow), 2011/11/28 17:28:58]

Is there a reason why cros can't just also use the pref instead of passing in on
construction?

+  // called, this will return the username.
   // Otherwise, it will return an empty string.
-  const std::string& GetUsername();
+  const std::string& GetAuthenticatedUsername();
 
-  // Sets the user name.  Used for migrating credentials from previous system.
-  void SetUsername(const std::string& username);
+  // Sets the user name.  Note: |username| should be already authenticated as
+  // this is a sticky operation (in contrast to StartSignIn).
+  // TODO(tim): Remove this in favor of passing username on construction by
+  // (by platform / depending on StartBehavior). Bug 88109.
+  void SetAuthenticatedUsername(const std::string& username);
 
   // Attempt to sign in this user with OAuth. If successful, set a preference
   // indicating the signed in user and send out a notification, then start
   // fetching tokens for the user.
   virtual void StartOAuthSignIn(const std::string& oauth1_request_token);
 
   // Attempt to sign in this user with ClientLogin. If successful, set a
   // preference indicating the signed in user and send out a notification,
   // then start fetching tokens for the user.
   // This is overridden for test subclasses that don't want to issue auth
   // requests.
   virtual void StartSignIn(const std::string& username,
                            const std::string& password,
                            const std::string& login_token,
                            const std::string& login_captcha);
 
   // Used when a second factor access code was required to complete a signin
   // attempt.
   void ProvideSecondFactorAccessCode(const std::string& access_code);
 
   // Sign a user out, removing the preference, erasing all keys
   // associated with the user, and canceling all auth in progress.
   void SignOut();
 
-  // Called when a new request to re-authenticate a user is in progress.
-  // Will clear in memory data but leaves the db as such so when the browser
-  // restarts we can use the old token(which might throw a password error).
-  void ClearInMemoryData();
-
   // GaiaAuthConsumer
   virtual void OnClientLoginSuccess(const ClientLoginResult& result) OVERRIDE;
   virtual void OnClientLoginFailure(
       const GoogleServiceAuthError& error) OVERRIDE;
   virtual void OnGetUserInfoSuccess(const std::string& key,
                                     const std::string& value) OVERRIDE;
   virtual void OnGetUserInfoKeyNotFound(const std::string& key) OVERRIDE;
   virtual void OnGetUserInfoFailure(
       const GoogleServiceAuthError& error) OVERRIDE;
   virtual void OnTokenAuthFailure(const GoogleServiceAuthError& error) OVERRIDE;
@@ skipping 11 matching lines @@
       const GoogleServiceAuthError& error) OVERRIDE;
   virtual void OnUserInfoSuccess(const std::string& email) OVERRIDE;
   virtual void OnUserInfoFailure(const GoogleServiceAuthError& error) OVERRIDE;
 
   // content::NotificationObserver
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(SigninManagerTest, ClearTransientSigninData);
+  FRIEND_TEST_ALL_PREFIXES(SigninManagerTest, ProvideSecondFactorSuccess);
+  FRIEND_TEST_ALL_PREFIXES(SigninManagerTest, ProvideSecondFactorFailure);
   void PrepareForSignin();
   void PrepareForOAuthSignin();
 
+  // Called when a new request to re-authenticate a user is in progress.
+  // Will clear in memory data but leaves the db as such so when the browser
+  // restarts we can use the old token(which might throw a password error).
+  void ClearTransientSigninData();
+
   Profile* profile_;
 
   // ClientLogin identity.
-  std::string username_;
+  std::string possibly_invalid_username_;
   std::string password_;  // This is kept empty whenever possible.
   bool had_two_factor_error_;
 
   // OAuth identity.
-  std::string oauth_username_;
   std::string oauth1_request_token_;
 
   void CleanupNotificationRegistration();
 
   // Result of the last client login, kept pending the lookup of the
   // canonical email.
   ClientLoginResult last_result_;
 
   // Actual client login handler.
   scoped_ptr<GaiaAuthFetcher> client_login_;
 
   // Actual OAuth login handler.
   scoped_ptr<GaiaOAuthFetcher> oauth_login_;
 
   // Register for notifications from the TokenService.
   content::NotificationRegistrar registrar_;
 
+  std::string authenticated_username_;

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

My big question with this CL is: why does SigninManager track the username,
given that we already take pains to ensure that authenticated_username_ always
matches what's stored under kGoogleServicesUsername in prefstore? Is this due to
cros, perhaps?

There are just so many different ways the username is abstracted now - through
SyncPrefs, through GetAuthenticatedUsername(), through setting the pref
directly, and there's no clear ownership of the pref (SigninManager clears it on
SignOut, but there are other places that set it). I'd like to see us either get
rid of all APIs that provide an interface for getting/setting the username (in
favor of having people just directly access the pref) or else have a single
interface that everyone uses (and so nobody touches the pref directly) - I
wonder if this CL would be a reasonable vehicle for a change like that? What's
your take on the situation?

[tim (not reviewing), 2011/11/28 05:30:21]

My motivation for this change (and the one before it) was to reduce the number
of different ways the username is abstracted. I removed GetAuthenticatedUsername
from the PSS, and am about to remove cros user, and the signin manager seems
like the best central / injectable thing to manage it.  If you notice my TODO in
SetAuthenticatedUsername, now that it's more obvious that this username maps to
the pref, we can probably remove this in the future and just use the pref.  i.e.
the todo is pretty much the same as your question above, I'm just trying to do
it piecewise.  Since the SigninManager will wind up setting the value as it
orchestrates the signin process, it seems like a reasonable place to build the
interface to the pref (either directly or wrapped in SyncPrefs, which can make
it easier to test than building a pref-service equipped profile). 

[Andrew T Wilson (Slow), 2011/11/28 17:28:58]

OK, sounds like we both agree on the long-term vision here, and I agree this is
a good intermediate step.

+
   DISALLOW_COPY_AND_ASSIGN(SigninManager);
 };
 
 #endif  // CHROME_BROWSER_SYNC_SIGNIN_MANAGER_H_