Canonicalize username when save PasswordHashData

Canonicalize username when we save PasswordHashData. For example,
user.name@gmail.com should always saved as username@gmail.com.  Since
some users might have already had non-canonicalized username saved. This
change also makes sure when we retrieve or clear saved PasswordHashData,
HashPasswordManager always compares canonicalized usernames.

Bug: 858865
Change-Id: Iac6df28a5660dcf5f77e9ee29c5b81805a02245f
Reviewed-on: https://chromium-review.googlesource.com/1123102Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573380}

components/password_manager/core/browser/BUILD.gn

@@ -243,6 +243,7 @@ static_library("password_hash_data") {
   deps = [
     "//base",
     "//crypto",
+    "//google_apis",
   ]
 }
 

components/password_manager/core/browser/hash_password_manager.cc

@@ -166,7 +166,9 @@ bool HashPasswordManager::SavePasswordHash(const std::string username,
   // sign in timestamp.
   ListPrefUpdate update(prefs_, prefs::kPasswordHashDataList);
   for (base::Value& password_hash_data : update.Get()->GetList()) {
-    if (GetAndDecryptField(password_hash_data, kUsernameFieldKey) == username) {
+    if (AreUsernamesSame(
+            GetAndDecryptField(password_hash_data, kUsernameFieldKey),
+            username)) {
       base::Optional<PasswordHashData> existing_password_hash =
           ConvertToPasswordHashData(password_hash_data);
       if (existing_password_hash && existing_password_hash->MatchesPassword(
@@ -213,13 +215,14 @@ void HashPasswordManager::ClearSavedPasswordHash(const std::string& username,
                                                  bool is_gaia_password) {
   if (prefs_) {
     ListPrefUpdate update(prefs_, prefs::kPasswordHashDataList);
-    for (auto it = update->GetList().begin(); it != update->GetList().end();
-         it++) {
-      if (GetAndDecryptField(*it, kUsernameFieldKey) == username &&
+    for (auto it = update->GetList().begin(); it != update->GetList().end();) {
+      if (AreUsernamesSame(GetAndDecryptField(*it, kUsernameFieldKey),
+                           username) &&
           GetAndDecryptField(*it, kIsGaiaFieldKey) ==
               BooleanToString(is_gaia_password)) {
-        update->GetList().erase(it);
-        return;
+        it = update->GetList().erase(it);
+      } else {
+        it++;
       }
     }
   }
@@ -283,7 +286,8 @@ base::Optional<PasswordHashData> HashPasswordManager::RetrievePasswordHash(
 
   for (const base::Value& entry :
        prefs_->GetList(prefs::kPasswordHashDataList)->GetList()) {
-    if (GetAndDecryptField(entry, kUsernameFieldKey) == username &&
+    if (AreUsernamesSame(GetAndDecryptField(entry, kUsernameFieldKey),
+                         username) &&
         GetAndDecryptField(entry, kIsGaiaFieldKey) ==
             BooleanToString(is_gaia_password)) {
       return ConvertToPasswordHashData(entry);
@@ -306,7 +310,8 @@ bool HashPasswordManager::HasPasswordHash(const std::string& username,
 
   for (const base::Value& entry :
        prefs_->GetList(prefs::kPasswordHashDataList)->GetList()) {
-    if (username == GetAndDecryptField(entry, kUsernameFieldKey) &&
+    if (AreUsernamesSame(GetAndDecryptField(entry, kUsernameFieldKey),
+                         username) &&
         BooleanToString(is_gaia_password) ==
             GetAndDecryptField(entry, kIsGaiaFieldKey)) {
       return true;
@@ -392,7 +397,8 @@ bool HashPasswordManager::EncryptAndSave(
     return false;
   }
 
-  std::string encrypted_username = EncryptString(password_hash_data.username);
+  std::string encrypted_username =
+      EncryptString(CanonicalizeUsername(password_hash_data.username));
   if (encrypted_username.empty())
     return false;
 
@@ -423,17 +429,23 @@ bool HashPasswordManager::EncryptAndSave(
   encrypted_password_hash_entry.SetKey(
       kLastSignInTimeFieldKey, base::Value(base::Time::Now().ToDoubleT()));
   ListPrefUpdate update(prefs_, prefs::kPasswordHashDataList);
-  for (auto it = update->GetList().begin(); it != update->GetList().end();
-       it++) {
-    if (GetAndDecryptField(*it, kUsernameFieldKey) ==
-            password_hash_data.username &&
+  bool replace_old_entry = false;
+  for (auto it = update->GetList().begin(); it != update->GetList().end();) {
+    if (AreUsernamesSame(GetAndDecryptField(*it, kUsernameFieldKey),
+                         password_hash_data.username) &&
         GetAndDecryptField(*it, kIsGaiaFieldKey) ==
             BooleanToString(password_hash_data.is_gaia_password)) {
-      update->GetList().erase(it);
-      update->GetList().push_back(std::move(encrypted_password_hash_entry));
-      return true;
+      it = update->GetList().erase(it);
+      replace_old_entry = true;
+    } else {
+      it++;
     }
   }
+  if (replace_old_entry) {
+    update->GetList().push_back(std::move(encrypted_password_hash_entry));
+    return true;
+  }
+
   if (update->GetList().size() >= kMaxPasswordHashDataDictSize)
     RemoveOldestSignInPasswordHashData(&update->GetList());
 

components/password_manager/core/browser/hash_password_manager_unittest.cc

@@ -75,7 +75,7 @@ TEST_F(HashPasswordManagerTest, SavingPasswordHashData) {
   // Verify |SavePasswordHash(const std::string,const base::string16&)|
   // behavior.
   hash_password_manager.SavePasswordHash(username, password,
-                                         /*force_update=*/true);
+                                         /*is_gaia_password=*/true);
   EXPECT_TRUE(prefs_.HasPrefPath(prefs::kPasswordHashDataList));
 
   // Saves the same password again won't change password hash, length or salt.
@@ -83,7 +83,7 @@ TEST_F(HashPasswordManagerTest, SavingPasswordHashData) {
       hash_password_manager.RetrievePasswordHash(username,
                                                  /*is_gaia_password=*/true);
   hash_password_manager.SavePasswordHash(username, password,
-                                         /*force_update=*/true);
+                                         /*is_gaia_password=*/true);
   base::Optional<PasswordHashData> existing_password_data =
       hash_password_manager.RetrievePasswordHash(username,
                                                  /*is_gaia_password=*/true);
@@ -95,7 +95,7 @@ TEST_F(HashPasswordManagerTest, SavingPasswordHashData) {
   // Verify |SavePasswordHash(const PasswordHashData&)| behavior.
   base::string16 new_password(base::UTF8ToUTF16("new_password"));
   PasswordHashData new_password_data(username, new_password,
-                                     /*force_update=*/true);
+                                     /*is_gaia_password=*/true);
   EXPECT_TRUE(hash_password_manager.SavePasswordHash(new_password_data));
   EXPECT_NE(current_password_hash_data->hash,
             hash_password_manager
@@ -103,6 +103,44 @@ TEST_F(HashPasswordManagerTest, SavingPasswordHashData) {
                 ->hash);
 }
 
+TEST_F(HashPasswordManagerTest, SavingPasswordHashDataNotCanonicalized) {
+  ASSERT_FALSE(prefs_.HasPrefPath(prefs::kPasswordHashDataList));
+  HashPasswordManager hash_password_manager;
+  hash_password_manager.set_prefs(&prefs_);
+  base::string16 password(base::UTF8ToUTF16("password"));
+  std::string canonical_username("user@gmail.com");
+  std::string username("US.ER@gmail.com");
+
+  // Verify |SavePasswordHash(const std::string,const base::string16&)|
+  // behavior.
+  hash_password_manager.SavePasswordHash(canonical_username, password,
+                                         /*is_gaia_password=*/true);
+  ASSERT_TRUE(prefs_.HasPrefPath(prefs::kPasswordHashDataList));
+  EXPECT_EQ(1u, prefs_.GetList(prefs::kPasswordHashDataList)->GetList().size());
+  EXPECT_EQ(
+      canonical_username,
+      hash_password_manager
+          .RetrievePasswordHash(canonical_username, /*is_gaia_password=*/true)
+          ->username);
+
+  // Saves the same password with not canonicalized username should not change
+  // password hash.
+  base::Optional<PasswordHashData> current_password_hash_data =
+      hash_password_manager.RetrievePasswordHash(username,
+                                                 /*is_gaia_password=*/true);
+  hash_password_manager.SavePasswordHash(username, password,
+                                         /*force_update=*/true);
+  base::Optional<PasswordHashData> existing_password_data =
+      hash_password_manager.RetrievePasswordHash(username,
+                                                 /*is_gaia_password=*/true);
+  EXPECT_EQ(current_password_hash_data->hash, existing_password_data->hash);
+  EXPECT_EQ(1u, prefs_.GetList(prefs::kPasswordHashDataList)->GetList().size());
+  EXPECT_EQ(canonical_username,
+            hash_password_manager
+                .RetrievePasswordHash(username, /*is_gaia_password=*/true)
+                ->username);
+}
+
 TEST_F(HashPasswordManagerTest, SavingGaiaPasswordAndNonGaiaPassword) {
   ASSERT_FALSE(prefs_.HasPrefPath(prefs::kPasswordHashDataList));
   HashPasswordManager hash_password_manager;
@@ -239,17 +277,17 @@ TEST_F(HashPasswordManagerTest, RetrievingPasswordHashData) {
   ASSERT_FALSE(prefs_.HasPrefPath(prefs::kPasswordHashDataList));
   HashPasswordManager hash_password_manager;
   hash_password_manager.set_prefs(&prefs_);
-  hash_password_manager.SavePasswordHash("username",
+  hash_password_manager.SavePasswordHash("username@gmail.com",
                                          base::UTF8ToUTF16("password"),
                                          /*is_gaia_password=*/true);
   EXPECT_EQ(1u, hash_password_manager.RetrieveAllPasswordHashes().size());
 
   base::Optional<PasswordHashData> password_hash_data =
-      hash_password_manager.RetrievePasswordHash("username",
+      hash_password_manager.RetrievePasswordHash("username@gmail.com",
                                                  /*is_gaia_password=*/false);
   ASSERT_FALSE(password_hash_data);
   password_hash_data = hash_password_manager.RetrievePasswordHash(
-      "username", /*is_gaia_password=*/true);
+      "username@gmail.com", /*is_gaia_password=*/true);
   ASSERT_TRUE(password_hash_data);
   EXPECT_EQ(8u, password_hash_data->length);
   EXPECT_EQ(16u, password_hash_data->salt.size());
@@ -257,6 +295,15 @@ TEST_F(HashPasswordManagerTest, RetrievingPasswordHashData) {
                                                  password_hash_data->salt);
   EXPECT_EQ(expected_hash, password_hash_data->hash);
 
+  // Retrieve not canonicalized version of "username@gmail.com" should return
+  // the same result.
+  EXPECT_TRUE(
+      hash_password_manager.RetrievePasswordHash("user.name@gmail.com",
+                                                 /*is_gaia_password=*/true));
+  EXPECT_TRUE(
+      hash_password_manager.RetrievePasswordHash("USER.NAME@gmail.com",
+                                                 /*is_gaia_password=*/true));
+
   base::Optional<PasswordHashData> non_existing_data =
       hash_password_manager.RetrievePasswordHash("non_existing_user", true);
   ASSERT_FALSE(non_existing_data);

components/password_manager/core/browser/password_hash_data.cc

@@ -5,8 +5,11 @@
 #include "components/password_manager/core/browser/password_hash_data.h"
 
 #include "base/strings/string_piece.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "crypto/openssl_util.h"
 #include "crypto/random.h"
+#include "google_apis/gaia/gaia_auth_util.h"
 #include "third_party/boringssl/src/include/openssl/evp.h"
 
 namespace password_manager {
@@ -44,7 +47,8 @@ PasswordHashData::PasswordHashData(const std::string& username,
 bool PasswordHashData::MatchesPassword(const std::string& username,
                                        const base::string16& password,
                                        bool is_gaia_password) const {
-  if (password.size() != this->length || username != this->username ||
+  if (password.size() != this->length ||
+      !AreUsernamesSame(username, this->username) ||
       is_gaia_password != this->is_gaia_password) {
     return false;
   }
@@ -99,4 +103,15 @@ uint64_t CalculatePasswordHash(const base::StringPiece16& text,
   return hash37;
 }
 
+std::string CanonicalizeUsername(const std::string& username) {
+  std::vector<std::string> parts = base::SplitString(
+      username, "@", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  return parts.size() != 2U ? username : gaia::CanonicalizeEmail(username);
+}
+
+bool AreUsernamesSame(const std::string& username1,
+                      const std::string& username2) {
+  return CanonicalizeUsername(username1) == CanonicalizeUsername(username2);
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/password_hash_data.h

@@ -56,6 +56,14 @@ struct SyncPasswordData {
 uint64_t CalculatePasswordHash(const base::StringPiece16& text,
                                const std::string& salt);
 
+// If username is an email address, canonicalizes this email. Otherwise, returns
+// |username|.
+std::string CanonicalizeUsername(const std::string& username);
+
+// Returns true if the two usernames the same after canonicalization.
+bool AreUsernamesSame(const std::string& username1,
+                      const std::string& username2);
+
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_HASH_DATA_H_