Reland "Refactor HashPasswordManager to accept multiple password hashes"

This is a reland of 3c6230c

Previous revert is caused by merge conflict with another CL that
happened to land around the same time.

This reland is a simply a rebase of the original CL.

Original change's description:
> Refactor HashPasswordManager to accept multiple password hashes
>
> This CL doesn't change existing behavior, but only refactor
> HashPasswordManager class such that it can keep track of multiple
> password hashes. This CL introduces a new struct PasswordHashData to
> replace SyncPassswordData.
>
> Migration from SyncPasswordData to PasswordHashData is in place so that
> we'll not lose already captured sync password. IsSyncPasswordHashSaved
> histogram enum is added to track this migration. SyncPasswordData
> will be deprecated after the transition period.
>
> Bug: 830998
> Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
> Change-Id: I50b52adcd1cf14a6bfddc74838019f3c7d287a99
>
> TBR=jochen@chromium.org
>
> Change-Id: I50b52adcd1cf14a6bfddc74838019f3c7d287a99
> Reviewed-on: https://chromium-review.googlesource.com/1036464
> Commit-Queue: Jialiu Lin <jialiul@chromium.org>
> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557646}

TBR=dvadym@chromium.org, jochen@chromium.org

Bug: 830998
Change-Id: I195fbea0c53f2b3a2000e53b37f3eeaa33d5b846
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/1054354
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557730}

ash/login/login_screen_controller.cc

@@ -396,8 +396,9 @@ void LoginScreenController::DoAuthenticateUser(const AccountId& account_id,
       password, system_salt, chromeos::Key::KEY_TYPE_SALTED_SHA256_TOP_HALF);
 
   // Used for GAIA password reuse detection.
-  password_manager::SyncPasswordData sync_password_data(
-      base::UTF8ToUTF16(password), false /*force_update*/);
+  password_manager::PasswordHashData sync_password_hash_data(
+      account_id.GetUserEmail(), base::UTF8ToUTF16(password),
+      false /*force_update*/);
 
   PrefService* prefs =
       Shell::Get()->session_controller()->GetLastActiveUserPrefService();
@@ -421,7 +422,7 @@ void LoginScreenController::DoAuthenticateUser(const AccountId& account_id,
       is_pin ? LoginMetricsRecorder::AuthMethod::kPin
              : LoginMetricsRecorder::AuthMethod::kPassword);
   login_screen_client_->AuthenticateUser(
-      account_id, hashed_password, sync_password_data, is_pin,
+      account_id, hashed_password, sync_password_hash_data, is_pin,
       base::BindOnce(&LoginScreenController::OnAuthenticateComplete,
                      weak_factory_.GetWeakPtr(), base::Passed(&callback)));
 }

ash/login/mock_login_screen_client.cc

@@ -24,10 +24,10 @@ mojom::LoginScreenClientPtr MockLoginScreenClient::CreateInterfacePtrAndBind() {
 void MockLoginScreenClient::AuthenticateUser(
     const AccountId& account_id,
     const std::string& password,
-    const password_manager::SyncPasswordData& sync_password_data,
+    const password_manager::PasswordHashData& sync_password_hash_data,
     bool authenticated_by_pin,
     AuthenticateUserCallback callback) {
-  AuthenticateUser_(account_id, password, sync_password_data,
+  AuthenticateUser_(account_id, password, sync_password_hash_data,
                     authenticated_by_pin, callback);
   if (authenticate_user_callback_storage_)
     *authenticate_user_callback_storage_ = std::move(callback);

ash/login/mock_login_screen_client.h

@@ -19,13 +19,13 @@ class MockLoginScreenClient : public mojom::LoginScreenClient {
 
   mojom::LoginScreenClientPtr CreateInterfacePtrAndBind();
 
-  MOCK_METHOD5(
-      AuthenticateUser_,
-      void(const AccountId& account_id,
-           const std::string& password,
-           const password_manager::SyncPasswordData& sync_password_data_unused,
-           bool authenticated_by_pin,
-           AuthenticateUserCallback& callback));
+  MOCK_METHOD5(AuthenticateUser_,
+               void(const AccountId& account_id,
+                    const std::string& password,
+                    const password_manager::PasswordHashData&
+                        sync_password_hash_data_unused,
+                    bool authenticated_by_pin,
+                    AuthenticateUserCallback& callback));
 
   // Set the result that should be passed to |callback| in |AuthenticateUser|.
   void set_authenticate_user_callback_result(bool value) {
@@ -43,7 +43,7 @@ class MockLoginScreenClient : public mojom::LoginScreenClient {
   void AuthenticateUser(
       const AccountId& account_id,
       const std::string& password,
-      const password_manager::SyncPasswordData& sync_password_data,
+      const password_manager::PasswordHashData& sync_password_hash_data,
       bool authenticated_by_pin,
       AuthenticateUserCallback callback) override;
   MOCK_METHOD1(AttemptUnlock, void(const AccountId& account_id));

ash/public/interfaces/login_screen.mojom

@@ -7,7 +7,7 @@ module ash.mojom;
 import "ash/public/interfaces/user_info.mojom";
 import "ash/public/interfaces/login_user_info.mojom";
 import "chromeos/components/proximity_auth/public/interfaces/auth_type.mojom";
-import "components/password_manager/public/interfaces/sync_password_data.mojom";
+import "components/password_manager/public/interfaces/password_hash_data.mojom";
 import "components/account_id/interfaces/account_id.mojom";
 import "mojo/public/mojom/base/string16.mojom";
 import "mojo/public/mojom/base/values.mojom";
@@ -124,15 +124,16 @@ interface LoginScreenClient {
   // chrome will request lock screen to show error messages.
   // |account_id|:           The account id of the user we are authenticating.
   // |hashed_password|:      The hashed password of the user.
-  // |sync_password_data|:   Sync password related data used for password
-  //                         reuse detection.
+  // |sync_password_hash_data|: Password hash data of sync password, used for
+  //                            password reuse detection.
   // |authenticated_by_pin|: True if we are using pin to authenticate.
   //
   // The result will be set to true if auth was successful, false if not.
-  AuthenticateUser(signin.mojom.AccountId account_id,
-                   string hashed_password,
-                   password_manager.mojom.SyncPasswordData sync_password_data,
-                   bool authenticated_by_pin) => (bool auth_success);
+  AuthenticateUser(
+      signin.mojom.AccountId account_id,
+      string hashed_password,
+      password_manager.mojom.PasswordHashData sync_password_hash_data,
+      bool authenticated_by_pin) => (bool auth_success);
 
   // Request to attempt easy unlock in chrome.
   // |account_id|:   The account id of the user we are authenticating.

chrome/browser/chromeos/login/existing_user_controller.cc

@@ -611,8 +611,9 @@ void ExistingUserController::PerformLogin(
   if (user_context.GetKey()->GetKeyType() == Key::KEY_TYPE_PASSWORD_PLAIN) {
     base::string16 password(
         base::UTF8ToUTF16(new_user_context.GetKey()->GetSecret()));
-    new_user_context.SetSyncPasswordData(password_manager::SyncPasswordData(
-        password, auth_mode == LoginPerformer::AUTH_MODE_EXTENSION));
+    new_user_context.SetSyncPasswordData(password_manager::PasswordHashData(
+        user_context.GetAccountId().GetUserEmail(), password,
+        auth_mode == LoginPerformer::AUTH_MODE_EXTENSION));
   }
 
   if (user_manager::UserManager::Get()->IsSupervisedAccountId(

chrome/browser/chromeos/login/existing_user_controller_browsertest.cc

@@ -1059,9 +1059,7 @@ IN_PROC_BROWSER_TEST_F(ExistingUserControllerSavePasswordHashTest,
   Profile* profile =
       content::Details<Profile>(profile_prepared_observer.details()).ptr();
   EXPECT_TRUE(profile->GetPrefs()->HasPrefPath(
-      password_manager::prefs::kSyncPasswordHash));
-  EXPECT_TRUE(profile->GetPrefs()->HasPrefPath(
-      password_manager::prefs::kSyncPasswordLengthAndHashSalt));
+      password_manager::prefs::kPasswordHashDataList));
 }
 
 // Tests that successful offline login saves SyncPasswordData to user profile
@@ -1083,9 +1081,7 @@ IN_PROC_BROWSER_TEST_F(ExistingUserControllerSavePasswordHashTest,
   Profile* profile =
       content::Details<Profile>(profile_prepared_observer.details()).ptr();
   EXPECT_TRUE(profile->GetPrefs()->HasPrefPath(
-      password_manager::prefs::kSyncPasswordHash));
-  EXPECT_TRUE(profile->GetPrefs()->HasPrefPath(
-      password_manager::prefs::kSyncPasswordLengthAndHashSalt));
+      password_manager::prefs::kPasswordHashDataList));
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/lock/views_screen_locker.cc

@@ -160,7 +160,7 @@ content::WebContents* ViewsScreenLocker::GetWebContents() {
 void ViewsScreenLocker::HandleAuthenticateUser(
     const AccountId& account_id,
     const std::string& hashed_password,
-    const password_manager::SyncPasswordData& sync_password_data,
+    const password_manager::PasswordHashData& sync_password_hash_data,
     bool authenticated_by_pin,
     AuthenticateUserCallback callback) {
   DCHECK_EQ(account_id.GetUserEmail(),
@@ -181,7 +181,7 @@ void ViewsScreenLocker::HandleAuthenticateUser(
                            : chromeos::Key::KEY_TYPE_SALTED_SHA256_TOP_HALF;
   user_context.SetKey(Key(key_type, std::string(), hashed_password));
   user_context.SetIsUsingPin(authenticated_by_pin);
-  user_context.SetSyncPasswordData(sync_password_data);
+  user_context.SetSyncPasswordData(sync_password_hash_data);
   if (account_id.GetAccountType() == AccountType::ACTIVE_DIRECTORY &&
       (user_context.GetUserType() !=
        user_manager::UserType::USER_TYPE_ACTIVE_DIRECTORY)) {

chrome/browser/chromeos/login/lock/views_screen_locker.h

@@ -52,7 +52,7 @@ class ViewsScreenLocker : public LoginScreenClient::Delegate,
   void HandleAuthenticateUser(
       const AccountId& account_id,
       const std::string& hashed_password,
-      const password_manager::SyncPasswordData& sync_password_data,
+      const password_manager::PasswordHashData& sync_password_hash_data,
       bool authenticated_by_pin,
       AuthenticateUserCallback callback) override;
   void HandleAttemptUnlock(const AccountId& account_id) override;

chrome/browser/chromeos/login/ui/login_display_host_mojo.cc

@@ -206,7 +206,7 @@ const user_manager::UserList LoginDisplayHostMojo::GetUsers() {
 void LoginDisplayHostMojo::HandleAuthenticateUser(
     const AccountId& account_id,
     const std::string& hashed_password,
-    const password_manager::SyncPasswordData& sync_password_data,
+    const password_manager::PasswordHashData& sync_password_hash_data,
     bool authenticated_by_pin,
     AuthenticateUserCallback callback) {
   DCHECK(!authenticated_by_pin);
@@ -221,7 +221,7 @@ void LoginDisplayHostMojo::HandleAuthenticateUser(
   UserContext user_context(*user);
   user_context.SetKey(Key(chromeos::Key::KEY_TYPE_SALTED_SHA256_TOP_HALF,
                           std::string(), hashed_password));
-  user_context.SetSyncPasswordData(sync_password_data);
+  user_context.SetSyncPasswordData(sync_password_hash_data);
   if (account_id.GetAccountType() == AccountType::ACTIVE_DIRECTORY &&
       (user_context.GetUserType() !=
        user_manager::UserType::USER_TYPE_ACTIVE_DIRECTORY)) {

chrome/browser/chromeos/login/ui/login_display_host_mojo.h

@@ -69,7 +69,7 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
   void HandleAuthenticateUser(
       const AccountId& account_id,
       const std::string& hashed_password,
-      const password_manager::SyncPasswordData& sync_password_data,
+      const password_manager::PasswordHashData& sync_password_hash_data,
       bool authenticated_by_pin,
       AuthenticateUserCallback callback) override;
   void HandleAttemptUnlock(const AccountId& account_id) override;

chrome/browser/password_manager/password_store_factory.cc

@@ -32,6 +32,7 @@
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
+#include "components/signin/core/browser/signin_manager.h"
 
 #if defined(OS_WIN)
 #include "chrome/browser/password_manager/password_manager_util_win.h"
@@ -66,6 +67,15 @@ const LocalProfileId kInvalidLocalProfileId =
     static_cast<LocalProfileId>(0);
 #endif
 
+#if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
+std::string GetSyncUsername(Profile* profile) {
+  SigninManagerBase* signin_manager =
+      SigninManagerFactory::GetForProfileIfExists(profile);
+  return signin_manager ? signin_manager->GetAuthenticatedAccountInfo().email
+                        : std::string();
+}
+#endif
+
 }  // namespace
 
 // static
@@ -260,6 +270,13 @@ PasswordStoreFactory::BuildServiceInstanceFor(
     return nullptr;
   }
 
+#if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
+  // Prepare sync password hash data for reuse detection.
+  std::string sync_username = GetSyncUsername(profile);
+  if (!sync_username.empty())
+    ps->PrepareSyncPasswordHashData(sync_username);
+#endif
+
   // TODO(https://crbug.com/817754): remove the code once majority of the users
   // executed it.
   password_manager_util::CleanUserDataInBlacklistedCredentials(

chrome/browser/password_manager/password_store_signin_notifier_impl.cc

@@ -32,13 +32,13 @@ void PasswordStoreSigninNotifierImpl::GoogleSigninSucceededWithPassword(
     const std::string& account_id,
     const std::string& username,
     const std::string& password) {
-  NotifySignin(password);
+  NotifySignin(username, password);
 }
 
 void PasswordStoreSigninNotifierImpl::GoogleSignedOut(
     const std::string& account_id,
     const std::string& username) {
-  NotifySignedOut();
+  NotifySignedOut(username);
 }
 
 }  // namespace password_manager

chrome/browser/password_manager/password_store_signin_notifier_impl_unittest.cc

@@ -47,11 +47,11 @@ TEST_F(PasswordStoreSigninNotifierImplTest, Subscribed) {
   EXPECT_CALL(
       *store_,
       SaveSyncPasswordHash(
-          base::ASCIIToUTF16("password"),
+          "username", base::ASCIIToUTF16("password"),
           metrics_util::SyncPasswordHashChange::SAVED_ON_CHROME_SIGNIN));
   fake_signin_manager_->SignIn("accountid", "username", "password");
   testing::Mock::VerifyAndClearExpectations(store_.get());
-  EXPECT_CALL(*store_, ClearSyncPasswordHash());
+  EXPECT_CALL(*store_, ClearPasswordHash(_));
   fake_signin_manager_->ForceSignOut();
   notifier.UnsubscribeFromSigninEvents();
 }
@@ -63,8 +63,8 @@ TEST_F(PasswordStoreSigninNotifierImplTest, Unsubscribed) {
   notifier.SubscribeToSigninEvents(store_.get());
 
   notifier.UnsubscribeFromSigninEvents();
-  EXPECT_CALL(*store_, SaveSyncPasswordHash(_, _)).Times(0);
-  EXPECT_CALL(*store_, ClearSyncPasswordHash()).Times(0);
+  EXPECT_CALL(*store_, SaveSyncPasswordHash(_, _, _)).Times(0);
+  EXPECT_CALL(*store_, ClearPasswordHash(_)).Times(0);
   fake_signin_manager_->SignIn("accountid", "username", "secret");
   fake_signin_manager_->ForceSignOut();
 }

chrome/browser/safe_browsing/chrome_password_protection_service.cc

@@ -30,6 +30,7 @@
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/google/core/browser/google_util.h"
+#include "components/password_manager/core/browser/hash_password_manager.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
@@ -177,11 +178,16 @@ ChromePasswordProtectionService::ChromePasswordProtectionService(
       pref_change_registrar_(new PrefChangeRegistrar) {
   pref_change_registrar_->Init(profile_->GetPrefs());
   pref_change_registrar_->Add(
-      password_manager::prefs::kSyncPasswordHash,
+      password_manager::prefs::kPasswordHashDataList,
       base::Bind(&ChromePasswordProtectionService::CheckGaiaPasswordChange,
                  base::Unretained(this)));
-  gaia_password_hash_ = profile_->GetPrefs()->GetString(
-      password_manager::prefs::kSyncPasswordHash);
+  password_manager::HashPasswordManager hash_password_manager;
+  hash_password_manager.set_prefs(profile->GetPrefs());
+  base::Optional<password_manager::PasswordHashData> sync_hash_data =
+      hash_password_manager.RetrievePasswordHash(GetAccountInfo().email);
+  sync_password_hash_ = sync_hash_data
+                            ? base::NumberToString(sync_hash_data->hash)
+                            : std::string();
 }
 
 ChromePasswordProtectionService::~ChromePasswordProtectionService() {
@@ -727,10 +733,17 @@ void ChromePasswordProtectionService::
 }
 
 void ChromePasswordProtectionService::CheckGaiaPasswordChange() {
-  std::string new_gaia_password_hash = profile_->GetPrefs()->GetString(
-      password_manager::prefs::kSyncPasswordHash);
-  if (gaia_password_hash_ != new_gaia_password_hash) {
-    gaia_password_hash_ = new_gaia_password_hash;
+  password_manager::HashPasswordManager hash_password_manager;
+  hash_password_manager.set_prefs(profile_->GetPrefs());
+  base::Optional<password_manager::PasswordHashData>
+      new_sync_password_hash_data =
+          hash_password_manager.RetrievePasswordHash(GetAccountInfo().email);
+  std::string new_sync_password_hash =
+      new_sync_password_hash_data
+          ? base::NumberToString(new_sync_password_hash_data->hash)
+          : std::string();
+  if (sync_password_hash_ != new_sync_password_hash) {
+    sync_password_hash_ = new_sync_password_hash;
     OnGaiaPasswordChanged();
   }
 }

chrome/browser/safe_browsing/chrome_password_protection_service.h

@@ -224,7 +224,7 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
       PasswordProtectionService::WarningAction action);
 
   void SetGaiaPasswordHashForTesting(const std::string& new_password_hash) {
-    gaia_password_hash_ = new_password_hash;
+    sync_password_hash_ = new_password_hash;
   }
 
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
@@ -302,8 +302,8 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   TriggerManager* trigger_manager_;
   // Profile associated with this instance.
   Profile* profile_;
-  // Current Gaia password hash.
-  std::string gaia_password_hash_;
+  // Current sync password hash.
+  std::string sync_password_hash_;
   scoped_refptr<SafeBrowsingNavigationObserverManager>
       navigation_observer_manager_;
   base::ObserverList<Observer> observer_list_;