ChromeOS: This CL fixes bug in UserManager::GetKnownUserAccountId .

The bug in GetKnownUserAccountId led to crash on user log on.

BUG=462823
TEST=manual

Review URL: https://codereview.chromium.org/1463753002

Cr-Commit-Position: refs/heads/master@{#361286}

chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc

@@ -77,7 +77,9 @@ void ChromeUserSelectionScreen::OnDeviceLocalAccountsChanged() {
 
 void ChromeUserSelectionScreen::CheckForPublicSessionDisplayNameChange(
     policy::DeviceLocalAccountPolicyBroker* broker) {
-  const AccountId& account_id = GetAccountIdOfKnownUser(broker->user_id());
+  const AccountId& account_id =
+      user_manager::UserManager::GetKnownUserAccountId(broker->user_id(),
+                                                       std::string());
   DCHECK(account_id.is_valid());
   const std::string& display_name = broker->GetDisplayName();
   if (display_name == public_session_display_names_[account_id])
@@ -107,7 +109,9 @@ void ChromeUserSelectionScreen::CheckForPublicSessionDisplayNameChange(
 
 void ChromeUserSelectionScreen::CheckForPublicSessionLocalePolicyChange(
     policy::DeviceLocalAccountPolicyBroker* broker) {
-  const AccountId& account_id = GetAccountIdOfKnownUser(broker->user_id());
+  const AccountId& account_id =
+      user_manager::UserManager::GetKnownUserAccountId(broker->user_id(),
+                                                       std::string());
   DCHECK(account_id.is_valid());
   const policy::PolicyMap::Entry* entry =
       broker->core()->store()->policy_map().Get(policy::key::kSessionLocales);

chrome/browser/chromeos/login/screens/user_selection_screen.cc

@@ -246,26 +246,6 @@ bool UserSelectionScreen::ShouldForceOnlineSignIn(
          (token_status == user_manager::User::OAUTH_TOKEN_STATUS_UNKNOWN);
 }
 
-// static
-AccountId UserSelectionScreen::GetAccountIdOfKnownUser(
-    const std::string& user_id) {
-  if (user_id.empty())
-    return EmptyAccountId();
-
-  const AccountId initial_account_id = AccountId::FromUserEmail(user_id);
-
-  user_manager::UserManager* user_manager = user_manager::UserManager::Get();
-  if (!user_manager)
-    return initial_account_id;
-
-  AccountId known_account_id(EmptyAccountId());
-  if (user_manager->GetKnownUserAccountId(initial_account_id,
-                                          &known_account_id))
-    return known_account_id;
-
-  return initial_account_id;
-}
-
 void UserSelectionScreen::SetHandler(LoginDisplayWebUIHandler* handler) {
   handler_ = handler;
 }
@@ -369,7 +349,8 @@ void UserSelectionScreen::SendUserList() {
   std::string owner_email;
   chromeos::CrosSettings::Get()->GetString(chromeos::kDeviceOwner,
                                            &owner_email);
-  const AccountId owner(GetAccountIdOfKnownUser(owner_email));
+  const AccountId owner = user_manager::UserManager::GetKnownUserAccountId(
+      owner_email, std::string());
 
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
@@ -457,7 +438,8 @@ void UserSelectionScreen::OnUserStatusChecked(
 void UserSelectionScreen::SetAuthType(const std::string& user_id,
                                       AuthType auth_type,
                                       const base::string16& initial_value) {
-  const AccountId& account_id = GetAccountIdOfKnownUser(user_id);
+  const AccountId& account_id =
+      user_manager::UserManager::GetKnownUserAccountId(user_id, std::string());
   if (GetAuthType(account_id.GetUserEmail()) == FORCE_OFFLINE_PASSWORD)
     return;
   DCHECK(GetAuthType(account_id.GetUserEmail()) != FORCE_OFFLINE_PASSWORD ||
@@ -468,7 +450,8 @@ void UserSelectionScreen::SetAuthType(const std::string& user_id,
 
 proximity_auth::ScreenlockBridge::LockHandler::AuthType
 UserSelectionScreen::GetAuthType(const std::string& username) const {
-  const AccountId& account_id = GetAccountIdOfKnownUser(username);
+  const AccountId& account_id =
+      user_manager::UserManager::GetKnownUserAccountId(username, std::string());
   if (user_auth_type_map_.find(account_id) == user_auth_type_map_.end())
     return OFFLINE_PASSWORD;
   return user_auth_type_map_.find(account_id)->second;
@@ -496,12 +479,14 @@ void UserSelectionScreen::ShowUserPodCustomIcon(
   scoped_ptr<base::DictionaryValue> icon = icon_options.ToDictionaryValue();
   if (!icon || icon->empty())
     return;
-  const AccountId account_id = GetAccountIdOfKnownUser(user_id);
+  const AccountId account_id =
+      user_manager::UserManager::GetKnownUserAccountId(user_id, std::string());
   view_->ShowUserPodCustomIcon(account_id, *icon);
 }
 
 void UserSelectionScreen::HideUserPodCustomIcon(const std::string& user_id) {
-  const AccountId account_id = GetAccountIdOfKnownUser(user_id);
+  const AccountId account_id =
+      user_manager::UserManager::GetKnownUserAccountId(user_id, std::string());
   view_->HideUserPodCustomIcon(account_id);
 }
 
@@ -523,7 +508,8 @@ void UserSelectionScreen::AttemptEasySignin(const std::string& user_id,
                                             const std::string& key_label) {
   DCHECK_EQ(GetScreenType(), SIGNIN_SCREEN);
 
-  UserContext user_context(GetAccountIdOfKnownUser(user_id));
+  UserContext user_context(
+      user_manager::UserManager::GetKnownUserAccountId(user_id, std::string()));
   user_context.SetAuthFlow(UserContext::AUTH_FLOW_EASY_UNLOCK);
   user_context.SetKey(Key(secret));
   user_context.GetKey()->SetLabel(key_label);

chrome/browser/chromeos/login/screens/user_selection_screen.h

@@ -109,10 +109,6 @@ class UserSelectionScreen
   static bool ShouldForceOnlineSignIn(const user_manager::User* user);
 
  protected:
-  // This call forms full account id of a known user by email.
-  // This is a temporary call while migrating to AccountId.
-  static AccountId GetAccountIdOfKnownUser(const std::string& user_id);
-
   LoginDisplayWebUIHandler* handler_;
   LoginDisplay::Delegate* login_display_delegate_;
   UserBoardView* view_;

chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc

@@ -449,30 +449,16 @@ AccountId GaiaScreenHandler::GetAccountId(
     const std::string& gaia_id) const {
   const std::string canonicalized_email =
       gaia::CanonicalizeEmail(gaia::SanitizeEmail(authenticated_email));
-  const AccountId authenticated_account_id(
-      AccountId::FromUserEmailGaiaId(canonicalized_email, gaia_id));
-
-  // If we don't have UserManager instance (i.e. we are in unit test),
-  // or a known user has authenticated, just log in.
-  user_manager::UserManager* user_manager = user_manager::UserManager::Get();
-  if (!user_manager || user_manager->IsKnownUser(authenticated_account_id))
-    return authenticated_account_id;
-
-  // If [part of] user id has changed, update stored data and connect user
-  // to existing home directory.
-  AccountId old_account_id(EmptyAccountId());
-  if (!user_manager->GetKnownUserAccountId(authenticated_account_id,
-                                           &old_account_id)) {
-    return authenticated_account_id;
-  }
 
-  if (old_account_id.GetUserEmail() != canonicalized_email) {
-    LOG(WARNING) << "Existing user '" << old_account_id.GetUserEmail()
+  const AccountId account_id = user_manager::UserManager::GetKnownUserAccountId(
+      authenticated_email, gaia_id);
+
+  if (account_id.GetUserEmail() != canonicalized_email) {
+    LOG(WARNING) << "Existing user '" << account_id.GetUserEmail()
                  << "' authenticated by alias '" << canonicalized_email << "'.";
-    return old_account_id;
   }
 
-  return authenticated_account_id;
+  return account_id;
 }
 
 void GaiaScreenHandler::HandleCompleteAuthentication(

components/user_manager/user_manager.cc

@@ -5,6 +5,8 @@
 #include "components/user_manager/user_manager.h"
 
 #include "base/logging.h"
+#include "chromeos/login/user_names.h"
+#include "components/signin/core/account_id/account_id.h"
 
 namespace user_manager {
 
@@ -85,4 +87,27 @@ UserManager* UserManager::SetForTesting(UserManager* user_manager) {
   return previous_instance;
 }
 
+// static
+AccountId UserManager::GetKnownUserAccountId(const std::string& user_email,
+                                             const std::string& gaia_id) {
+  // In tests empty accounts are possible.
+  if (user_email.empty() && gaia_id.empty())
+    return EmptyAccountId();
+
+  if (user_email == chromeos::login::kStubUser)
+    return chromeos::login::StubAccountId();
+
+  if (user_email == chromeos::login::kGuestUserName)
+    return chromeos::login::GuestAccountId();
+
+  UserManager* user_manager = Get();
+  if (user_manager)
+    return user_manager->GetKnownUserAccountIdImpl(user_email, gaia_id);
+
+  // This is fallback for tests.
+  return (gaia_id.empty()
+              ? AccountId::FromUserEmail(user_email)
+              : AccountId::FromUserEmailGaiaId(user_email, gaia_id));
+}
+
 }  // namespace user_manager

components/user_manager/user_manager.h

@@ -363,10 +363,16 @@ class USER_MANAGER_EXPORT UserManager {
                                        const std::string& path,
                                        const int in_value) = 0;
 
-  // Returns true if user's AccountId was found.
-  // Returns it in |out_account_id|.
-  virtual bool GetKnownUserAccountId(const AccountId& authenticated_account_id,
-                                     AccountId* out_account_id) = 0;
+  // This call forms full account id of a known user by email and (optionally)
+  // gaia_id.
+  // This is a temporary call while migrating to AccountId.
+  virtual AccountId GetKnownUserAccountIdImpl(const std::string& user_email,
+                                              const std::string& gaia_id) = 0;
+
+  // The same as above, but doesn't crash in unit_tests when Usermanager
+  // doesn't exist.
+  static AccountId GetKnownUserAccountId(const std::string& user_email,
+                                         const std::string& gaia_id);
 
   // Updates |gaia_id| for user with |account_id|.
   // TODO(alemate): Update this once AccountId contains GAIA ID

components/user_manager/user_manager_base.cc

@@ -595,19 +595,7 @@ void UserManagerBase::ParseUserList(const base::ListValue& users_list,
       continue;
     }
 
-    const AccountId partial_account_id = AccountId::FromUserEmail(email);
-    AccountId account_id = EmptyAccountId();
-
-    const bool lookup_result =
-        GetKnownUserAccountId(partial_account_id, &account_id);
-    // TODO(alemate):
-    // DCHECK(lookup_result) << "KnownUser lookup falied for '" << email << "'";
-    // (tests do not initialize KnownUserData)
-
-    if (!lookup_result) {
-      account_id = partial_account_id;
-      LOG(WARNING) << "KnownUser lookup falied for '" << email << "'";
-    }
+    const AccountId account_id = GetKnownUserAccountId(email, std::string());
 
     if (existing_users.find(account_id) != existing_users.end() ||
         !users_set->insert(account_id).second) {
@@ -1042,6 +1030,7 @@ bool UserManagerBase::FindKnownUserPrefs(
   // Local State may not be initialized in tests.
   if (!local_state)
     return false;
+
   if (IsUserNonCryptohomeDataEphemeral(account_id))
     return false;
 
@@ -1163,29 +1152,38 @@ void UserManagerBase::SetKnownUserIntegerPref(const AccountId& account_id,
   UpdateKnownUserPrefs(account_id, dict, false);
 }
 
-bool UserManagerBase::GetKnownUserAccountId(
-    const AccountId& authenticated_account_id,
-    AccountId* out_account_id) {
-  if (!authenticated_account_id.GetGaiaId().empty()) {
-    std::string canonical_email;
-    if (!GetKnownUserStringPref(
-            AccountId::FromGaiaId(authenticated_account_id.GetGaiaId()),
-            kCanonicalEmail, &canonical_email)) {
-      return false;
-    }
+AccountId UserManagerBase::GetKnownUserAccountIdImpl(
+    const std::string& user_email,
+    const std::string& gaia_id) {
+  DCHECK(!(user_email.empty() && gaia_id.empty()));
+
+  // We can have several users with the same gaia_id but different e-mails.
+  // The opposite case is not possible.
+  std::string stored_gaia_id;
+  const std::string sanitized_email =
+      user_email.empty()
+          ? std::string()
+          : gaia::CanonicalizeEmail(gaia::SanitizeEmail(user_email));
+  if (!sanitized_email.empty() &&
+      GetKnownUserStringPref(AccountId::FromUserEmail(sanitized_email),
+                             kGAIAIdKey, &stored_gaia_id)) {
+    if (!gaia_id.empty() && gaia_id != stored_gaia_id)
+      LOG(ERROR) << "User gaia id has changed. Sync will not work.";
+
+    // gaia_id is associated with cryptohome.
+    return AccountId::FromUserEmailGaiaId(sanitized_email, stored_gaia_id);
+  }
 
-    *out_account_id = AccountId::FromUserEmailGaiaId(
-        canonical_email, authenticated_account_id.GetGaiaId());
-    return true;
+  std::string stored_email;
+  // GetKnownUserStringPref() returns the first user record that matches
+  // given ID. So we will get the first one if there are multiples.
+  if (!gaia_id.empty() &&
+      GetKnownUserStringPref(AccountId::FromGaiaId(gaia_id), kCanonicalEmail,
+                             &stored_email)) {
+    return AccountId::FromUserEmailGaiaId(stored_email, gaia_id);
   }
-  DCHECK(!authenticated_account_id.GetUserEmail().empty());
-  std::string gaia_id;
-  if (!GetKnownUserStringPref(authenticated_account_id, kGAIAIdKey, &gaia_id))
-    return false;
 
-  *out_account_id = AccountId::FromUserEmailGaiaId(
-      authenticated_account_id.GetUserEmail(), gaia_id);
-  return true;
+  return AccountId::FromUserEmailGaiaId(sanitized_email, gaia_id);
 }
 
 void UserManagerBase::UpdateGaiaID(const AccountId& account_id,

components/user_manager/user_manager_base.h

@@ -129,8 +129,8 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
   void SetKnownUserIntegerPref(const AccountId& account_id,
                                const std::string& path,
                                int in_value) override;
-  bool GetKnownUserAccountId(const AccountId& authenticated_account_id,
-                             AccountId* out_account_id) override;
+  AccountId GetKnownUserAccountIdImpl(const std::string& user_email,
+                                      const std::string& gaia_id) override;
   void UpdateGaiaID(const AccountId& account_id,
                     const std::string& gaia_id) override;
   bool FindGaiaID(const AccountId& account_id, std::string* out_value) override;