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

components/user_manager/user_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/user_manager/user_manager_base.h"
 
 #include <cstddef>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 577 matching lines @@
                                     std::set<AccountId>* users_set) {
   users_vector->clear();
   users_set->clear();
   for (size_t i = 0; i < users_list.GetSize(); ++i) {
     std::string email;
     if (!users_list.GetString(i, &email) || email.empty()) {
       LOG(ERROR) << "Corrupt entry in user list at index " << i << ".";
       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) {
       LOG(ERROR) << "Duplicate user: " << email;
       continue;
     }
     users_vector->push_back(account_id);
   }
 }
 
@@ skipping 414 matching lines @@
 }
 
 bool UserManagerBase::FindKnownUserPrefs(
     const AccountId& account_id,
     const base::DictionaryValue** out_value) {
   PrefService* local_state = GetLocalState();
 
   // Local State may not be initialized in tests.
   if (!local_state)
     return false;
+
   if (IsUserNonCryptohomeDataEphemeral(account_id))
     return false;
 
   const base::ListValue* known_users = local_state->GetList(kKnownUsers);
   for (size_t i = 0; i < known_users->GetSize(); ++i) {
     const base::DictionaryValue* element = nullptr;
     if (known_users->GetDictionary(i, &element)) {
       if (UserMatches(account_id, *element)) {
         known_users->GetDictionary(i, out_value);
         return true;
@@ skipping 101 matching lines @@
   // Local State may not be initialized in tests.
   if (!local_state)
     return;
 
   ListPrefUpdate update(local_state, kKnownUsers);
   base::DictionaryValue dict;
   dict.SetInteger(path, in_value);
   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()));
 
-    *out_account_id = AccountId::FromUserEmailGaiaId(
-        canonical_email, authenticated_account_id.GetGaiaId());
-    return true;
+  // We can have several users with the same gaia_id but different e-mails.
+  // The opposite case is not possible.

[achuithb, 2015/11/20 18:46:51]

I think we need more comments for the various cases.

+  std::string old_gaia_id;
+  const std::string sanitized_email =
+      gaia::CanonicalizeEmail(gaia::SanitizeEmail(user_email));
+  if (!user_email.empty() &&

[achuithb, 2015/11/20 18:46:51]

!sanitized_email.empty() would be clearer

[Alexander Alekseev, 2015/11/21 01:06:29]

Done.

+      GetKnownUserStringPref(AccountId::FromUserEmail(sanitized_email),
+                             kGAIAIdKey, &old_gaia_id)) {
+    if (!gaia_id.empty() && gaia_id != old_gaia_id)
+      LOG(ERROR) << "User gaia id has changed. Sync will not work.";
+
+    return AccountId::FromUserEmailGaiaId(sanitized_email, old_gaia_id);

[achuithb, 2015/11/20 18:46:51]

Why is this right? SHouldn't we use the new 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;
+  std::string canonical_email;

[achuithb, 2015/11/20 18:46:51]

maybe stored_email?

[Alexander Alekseev, 2015/11/21 01:06:29]

Done.

+  if (!gaia_id.empty() &&
+      GetKnownUserStringPref(AccountId::FromGaiaId(gaia_id), kCanonicalEmail,
+                             &canonical_email)) {
+    return AccountId::FromUserEmailGaiaId(canonical_email, gaia_id);
+  }
+
+  return AccountId::FromUserEmailGaiaId(sanitized_email, gaia_id);
 }
 
 void UserManagerBase::UpdateGaiaID(const AccountId& account_id,
                                    const std::string& gaia_id) {
   SetKnownUserStringPref(account_id, kGAIAIdKey, gaia_id);
 }
 
 bool UserManagerBase::FindGaiaID(const AccountId& account_id,
                                  std::string* out_value) {
   return GetKnownUserStringPref(account_id, kGAIAIdKey, out_value);
@@ skipping 192 matching lines @@
 }
 
 void UserManagerBase::DeleteUser(User* user) {
   const bool is_active_user = (user == active_user_);
   delete user;
   if (is_active_user)
     active_user_ = nullptr;
 }
 
 }  // namespace user_manager