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

chrome/browser/sync/signin_manager.h

Index: chrome/browser/sync/signin_manager.h
diff --git a/chrome/browser/sync/signin_manager.h b/chrome/browser/sync/signin_manager.h
index 240036928856c8de4bee646978ec5bf1cc658132..ff05bdcdf7fb171ced8a2ba856b4217cf2a548cf 100644
--- a/chrome/browser/sync/signin_manager.h
+++ b/chrome/browser/sync/signin_manager.h
@@ -6,6 +6,13 @@
 // 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_
@@ -52,12 +59,16 @@ class SigninManager : public GaiaAuthConsumer,
   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
@@ -82,11 +93,6 @@ class SigninManager : public GaiaAuthConsumer,
   // 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(
@@ -118,18 +124,25 @@ class SigninManager : public GaiaAuthConsumer,
                        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();
@@ -147,6 +160,8 @@ class SigninManager : public GaiaAuthConsumer,
   // 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);
 };