Replace SetAuthenticatedUsername with SetAuthenticatedAccountId.

components/signin/core/browser/signin_manager_base.cc

 // Copyright 2014 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 "components/signin/core/browser/signin_manager_base.h"
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/signin_client.h"
 #include "components/signin/core/common/signin_pref_names.h"
 #include "components/signin/core/common/signin_switches.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/gaia_urls.h"
 
 using namespace signin_internals_util;
 
-SigninManagerBase::SigninManagerBase(SigninClient* client)
-    : client_(client), initialized_(false), weak_pointer_factory_(this) {}
+SigninManagerBase::SigninManagerBase(
+    SigninClient* client,
+    AccountTrackerService* account_tracker_service)
+    : client_(client),
+      account_tracker_service_(account_tracker_service),
+      initialized_(false),
+      weak_pointer_factory_(this) {
+  DCHECK(client_);
+  DCHECK(account_tracker_service_);
+}
 
 SigninManagerBase::~SigninManagerBase() {}
 
 void SigninManagerBase::Initialize(PrefService* local_state) {
   // Should never call Initialize() twice.
   DCHECK(!IsInitialized());
   initialized_ = true;
 
   // If the user is clearing the token service from the command line, then
   // clear their login info also (not valid to be logged in without any
   // tokens).
   base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
-  if (cmd_line->HasSwitch(switches::kClearTokenService))
+  if (cmd_line->HasSwitch(switches::kClearTokenService)) {
+    client_->GetPrefs()->ClearPref(prefs::kGoogleServicesAccountId);
     client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
+    client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUserAccountId);

[Mike Lerman, 2015/04/08 14:45:27]

Also clear prefs::kGoogleServicesUserAccountId?

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

Already done.  Did you mean some other pref?

[Mike Lerman, 2015/04/08 20:49:28]

I did, I meeant kGoogleServicesLastAccountId.

[Roger Tawa OOO till Jul 10th, 2015/04/09 15:59:47]

Good question.  The *Last* prefs are for remembering the last account that
signed in to this profile.  I think that is probably worth keeping even if we
force clearing the existing tokens.  The old code also did not clear the
LastUsername, so functionality is preserved.

+  }
 
-  std::string user =
-      client_->GetPrefs()->GetString(prefs::kGoogleServicesUsername);
-  if (!user.empty())
-    SetAuthenticatedUsername(user);
+  std::string account_id =
+      client_->GetPrefs()->GetString(prefs::kGoogleServicesAccountId);
+
+  // Handle backward compatibility: if kGoogleServicesAccountId is empty, but
+  // kGoogleServicesUsername is not, then this is an old profile that needs to

[Mike Lerman, 2015/04/08 14:45:27]

comment nit: There's no "profile" within components. Can call it a user or, more
correctly, a context or browser context.

Or ignore this nit, since really, it's a profile ;)

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

I think I'll it as is.  I'm not sure "user" or "browser context" is more
readable :-)

+  // be updated.  kGoogleServicesUserAccountId should not be empty, and contains
+  // the gaia_id.  Use both properties to prime the account tracker before
+  // proceeding.
+  if (account_id.empty()) {
+    std::string pref_account_username =
+        client_->GetPrefs()->GetString(prefs::kGoogleServicesUsername);
+    std::string pref_gaia_id =
+        client_->GetPrefs()->GetString(prefs::kGoogleServicesUserAccountId);
+    DCHECK(pref_account_username.empty() || !pref_gaia_id.empty());

[Mike Lerman, 2015/04/08 14:45:27]

Is this supposed to read !pref_account_username.empty() - I assume you're trying
to DCHECK you have either the gaia_id or the account_username.

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

I'm trying to DCHECK that if I have a username, then I have a gaiaid.  So:

    have-email implies have-gaiaid

The implication p->q can be written as !p||q, so I think the condition is
correct.

+    if (!pref_account_username.empty() && !pref_gaia_id.empty()) {
+      account_tracker_service_->SeedAccountInfo(

[Mike Lerman, 2015/04/08 14:45:27]

Should lines 69-72 instead be:
account_id = SetAuthenticatedAccountInfo(pref_gaia_id, pref_account_username)?
That way we let the AccountTrackerService pick which is going to be that
account_id, and then have a common call to that pick.

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

I did it this way because this is the "upgrade" case, so for sure account_id ==
username.  But SeedAccountInfo() handles that correctly so changed.

+          pref_gaia_id, pref_account_username);
+
+      account_id = pref_account_username;
+      client_->GetPrefs()->SetString(prefs::kGoogleServicesLastAccountId,
+                                     account_id);
+
+      // Now remove obsolete preferences.
+      client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
+
+      // TODO(rogerta): once migration to gaia id is complete, remove
+      // kGoogleServicesUserAccountId and change all uses of that pref to
+      // kGoogleServicesAccountId.
+    }
+  }
+
+  if (!account_id.empty())
+    SetAuthenticatedAccountId(account_id);
 }
 
 bool SigninManagerBase::IsInitialized() const { return initialized_; }
 
 bool SigninManagerBase::IsSigninAllowed() const {
   return client_->GetPrefs()->GetBoolean(prefs::kSigninAllowed);
 }
 
-const std::string& SigninManagerBase::GetAuthenticatedUsername() const {
-  return authenticated_username_;
+std::string SigninManagerBase::GetAuthenticatedUsername() const {
+  return account_tracker_service_->GetAccountInfo(

[Mike Lerman, 2015/04/08 14:45:27]

Hmm. Should this method be deleted, and users of this method should instead
reference the AccountTrackerService to access the email?

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

It could, but there is just so much code that assumes you can get the username
directly from SM that this would be a very large change.  Something to consider
for a later CL maybe, but conceptually it makes sense for signin manager to
expose GetAuthenticatedUsername().

+      GetAuthenticatedAccountId()).email;
 }
 
 const std::string& SigninManagerBase::GetAuthenticatedAccountId() const {
   return authenticated_account_id_;
 }
 
-void SigninManagerBase::SetAuthenticatedUsername(const std::string& username) {
-  DCHECK(!username.empty());
-  if (!authenticated_username_.empty()) {
-    DLOG_IF(ERROR, !gaia::AreEmailsSame(username, authenticated_username_))
-        << "Tried to change the authenticated username to something different: "
-        << "Current: " << authenticated_username_ << ", New: " << username;
+void SigninManagerBase::SetAuthenticatedAccountInfo(const std::string& gaia_id,
+                                                    const std::string& email) {
+  std::string account_id =

[Mike Lerman, 2015/04/08 14:45:27]

Can this be a const std::string&?

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

SeedAccountInfo returns an AccountInfo by value.  Changing this to a const&
means we might reference a variable that goes out of scope at the ;

+      account_tracker_service_->SeedAccountInfo(gaia_id, email);
+  SetAuthenticatedAccountId(account_id);
+}
 
-#if defined(OS_IOS)
-    // Prior to M26, chrome on iOS did not normalize the email before setting
-    // it in SigninManager.  If the emails are the same as given by
-    // gaia::AreEmailsSame() but not the same as given by std::string::op==(),
-    // make sure to set the authenticated name below.
-    if (!gaia::AreEmailsSame(username, authenticated_username_) ||
-        username == authenticated_username_) {
-      return;
-    }
-#else
+void SigninManagerBase::SetAuthenticatedAccountId(
+    const std::string& account_id) {
+  DCHECK(!account_id.empty());
+  if (!authenticated_account_id_.empty()) {
+    DLOG_IF(ERROR, account_id != authenticated_account_id_)
+        << "Tried to change the authenticated id to something different: "
+        << "Current: " << authenticated_account_id_ << ", New: " << account_id;
     return;
-#endif
   }
-  std::string pref_username =
-      client_->GetPrefs()->GetString(prefs::kGoogleServicesUsername);
-  DCHECK(pref_username.empty() || gaia::AreEmailsSame(username, pref_username))
-      << "username: " << username << "; pref_username: " << pref_username;
-  authenticated_username_ = username;
 
-  // TODO(rogerta): remove this DCHECK when migration work is started.
-  DCHECK_EQ(AccountTrackerService::MIGRATION_NOT_STARTED,
-            AccountTrackerService::GetMigrationState(client_->GetPrefs()));
-  authenticated_account_id_ =
-      AccountTrackerService::PickAccountIdForAccount(client_->GetPrefs(),
-                                                     username,
-                                                     username);
-  client_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, username);
+  std::string pref_account_id =
+      client_->GetPrefs()->GetString(prefs::kGoogleServicesAccountId);
 
-  // Go ahead and update the last signed in username here as well. Once a
+  DCHECK(pref_account_id.empty() || pref_account_id == account_id)
+      << "account_id=" << account_id
+      << " pref_account_id=" << pref_account_id;
+  authenticated_account_id_ = account_id;
+  client_->GetPrefs()->SetString(prefs::kGoogleServicesAccountId, account_id);
+
+  // This preference is set so that code on I/O thread has access to the
+  // Gaia id of the signed in user.
+  AccountTrackerService::AccountInfo info =
+      account_tracker_service_->GetAccountInfo(account_id);
+  DCHECK(!info.gaia.empty());
+  client_->GetPrefs()->SetString(prefs::kGoogleServicesUserAccountId,
+                                 info.gaia);
+
+  // Go ahead and update the last signed in account info here as well. Once a
   // user is signed in the two preferences should match. Doing it here as
   // opposed to on signin allows us to catch the upgrade scenario.
-  client_->GetPrefs()->SetString(prefs::kGoogleServicesLastUsername, username);
-}
-
-void SigninManagerBase::ClearAuthenticatedUsername() {
-  authenticated_username_.clear();
-  authenticated_account_id_.clear();
+  client_->GetPrefs()->SetString(prefs::kGoogleServicesLastAccountId,

[Mike Lerman, 2015/04/08 14:45:27]

Umm -you never seem to read the kGoogleServicesLastAccountId anywhere. Why is it
set?

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

kGoogleServicesLastUsername was used to know the if an account was previously
signed in to this profile, see  inline_login_handler_impl.cc.  So I changed this
to be an account_id instead, hence this new pref.  But I realized that
inline_login_handler_impl.cc still needs the email string because it needs to
show it in the UI, and we can't depend on AccountTrackerService to map from id
to email since the account is no longer in the profile.  So I had to keep
kGoogleServicesLastUsername anyway.

I decided to leave this kGoogleServicesLastAccountId here because for non-UI
purposes, it is cleaner to check the id than the username.  But its true no uses
it right now.  I could remove this pref and then add it later if necessary, if
you think that is better.

[Mike Lerman, 2015/04/08 20:49:28]

Since we'll be keeping kGoogleServicesLastUsername for the long haul to show it
in the UI, and that field can meet all our needs, I'd vote for the slightly
trimmer solution and nix the kGoogleServicesLastAccountID.

[Roger Tawa OOO till Jul 10th, 2015/04/09 15:59:47]

Done.

+                                 account_id);
+  client_->GetPrefs()->SetString(prefs::kGoogleServicesLastUsername,

[Mike Lerman, 2015/04/08 14:45:27]

You never seem to read the kGoogleServicesLastUsername anywhere.

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

Used in inline_login_handler_impl.cc:405

+                                 info.email);
 }
 
 bool SigninManagerBase::IsAuthenticated() const {
   return !authenticated_account_id_.empty();
 }
 
+void SigninManagerBase::clear_authenticated_user() {
+  authenticated_account_id_.clear();

[Mike Lerman, 2015/04/08 14:45:27]

Should this also clear the kGoogleServicesAccountId pref?

[Roger Tawa OOO till Jul 10th, 2015/04/08 20:24:21]

I don't think so.

[Mike Lerman, 2015/04/08 20:49:28]

Then should the implementation of the hacker-style function be in the header
file?

[Roger Tawa OOO till Jul 10th, 2015/04/09 15:59:47]

Done.

+}
+
 bool SigninManagerBase::AuthInProgress() const {
   // SigninManagerBase never kicks off auth processes itself.
   return false;
 }
 
 void SigninManagerBase::Shutdown() {}
 
 void SigninManagerBase::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }
@@ skipping 12 matching lines @@
   signin_diagnostics_observers_.RemoveObserver(observer);
 }
 
 void SigninManagerBase::NotifyDiagnosticsObservers(
     const TimedSigninStatusField& field,
     const std::string& value) {
   FOR_EACH_OBSERVER(SigninDiagnosticsObserver,
                     signin_diagnostics_observers_,
                     NotifySigninValueChanged(field, value));
 }