Store the information about leaked credentials on sign-in.

Bug: 1049193
Change-Id: I735369eef149185a7a7f7b946cce65d536d66bc0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089902
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747390}

components/password_manager/core/browser/leak_detection_delegate.cc

@@ -80,45 +80,31 @@ void LeakDetectionDelegate::OnLeakDetectionDone(bool is_leaked,
     logger.LogBoolean(Logger::STRING_LEAK_DETECTION_FINISHED, is_leaked);
   }
 
-  password_manager::PasswordStore* password_store =
-      client_->GetProfilePasswordStore();
-  if (base::FeatureList::IsEnabled(
-          password_manager::features::kPasswordCheck) &&
-      is_leaked) {
-    password_store->AddCompromisedCredentials({GetSignonRealm(url), username,
-                                               base::Time::Now(),
-                                               CompromiseType::kLeaked});
-  }
-
   if (is_leaked) {
-    if (!client_->GetPasswordFeatureManager()
-             ->ShouldCheckReuseOnLeakDetection()) {
-      // If leaked password reuse should not be checked, then the
-      // |CredentialLeakType| needed to show the correct notification is already
-      // determined.
-      OnShowLeakDetectionNotification(
-          CreateLeakType(IsSaved(false), IsReused(false), IsSyncing(false)),
-          std::move(url), std::move(username));
-    } else {
       // Otherwise query the helper to asynchronously determine the
       // |CredentialLeakType|.
-      helper_ = std::make_unique<LeakDetectionDelegateHelper>(base::BindOnce(
-          &LeakDetectionDelegate::OnShowLeakDetectionNotification,
-          base::Unretained(this)));
-      helper_->GetCredentialLeakType(password_store, std::move(url),
-                                     std::move(username), std::move(password));
-    }
+      helper_ = std::make_unique<LeakDetectionDelegateHelper>(
+          client_->GetProfilePasswordStore(),
+          base::BindOnce(
+              &LeakDetectionDelegate::OnShowLeakDetectionNotification,
+              base::Unretained(this)));
+      helper_->ProcessLeakedPassword(std::move(url), std::move(username),
+                                     std::move(password));
   }
 }
 
 void LeakDetectionDelegate::OnShowLeakDetectionNotification(
-    CredentialLeakType leak_type,
+    IsSaved is_saved,
+    IsReused is_reused,
     GURL url,
     base::string16 username) {
   DCHECK(is_leaked_timer_);
   base::UmaHistogramTimes("PasswordManager.LeakDetection.NotifyIsLeakedTime",
                           std::exchange(is_leaked_timer_, nullptr)->Elapsed());
   helper_.reset();
+  CredentialLeakType leak_type = CreateLeakType(
+      is_saved, is_reused,
+      IsSyncing(client_->GetPasswordSyncState() == SYNCING_NORMAL_ENCRYPTION));
   client_->NotifyUserCredentialsWereLeaked(leak_type, url);
 }
 

components/password_manager/core/browser/leak_detection_delegate.h

@@ -52,10 +52,10 @@ class LeakDetectionDelegate : public LeakDetectionDelegateInterface {
                            base::string16 username,
                            base::string16 password) override;
 
-  // Initiates the showing of the leak detection notification. If the account is
-  // synced, it is called by |helper_| after the |leak_type| was asynchronously
-  // determined.
-  void OnShowLeakDetectionNotification(CredentialLeakType leak_type,
+  // Initiates the showing of the leak detection notification. It is called by
+  // |helper_| after |is_saved|/|is_reused| was asynchronously determined.
+  void OnShowLeakDetectionNotification(IsSaved is_saved,
+                                       IsReused is_reused,
                                        GURL url,
                                        base::string16 username);
 

components/password_manager/core/browser/leak_detection_delegate_helper.cc

@@ -4,38 +4,53 @@
 
 #include "components/password_manager/core/browser/leak_detection_delegate_helper.h"
 
+#include "base/feature_list.h"
+#include "components/password_manager/core/browser/leak_detection/encryption_utils.h"
 #include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/common/password_manager_features.h"
 
 namespace password_manager {
 
-LeakDetectionDelegateHelper::LeakDetectionDelegateHelper(LeakTypeReply callback)
-    : callback_(std::move(callback)) {}
+LeakDetectionDelegateHelper::LeakDetectionDelegateHelper(
+    scoped_refptr<PasswordStore> store,
+    LeakTypeReply callback)
+    : store_(std::move(store)), callback_(std::move(callback)) {
+  DCHECK(store_);
+}
 
 LeakDetectionDelegateHelper::~LeakDetectionDelegateHelper() = default;
 
-void LeakDetectionDelegateHelper::GetCredentialLeakType(
-    PasswordStore* store,
+void LeakDetectionDelegateHelper::ProcessLeakedPassword(
     GURL url,
     base::string16 username,
     base::string16 password) {
-  DCHECK(store);
   url_ = std::move(url);
   username_ = std::move(username);
   password_ = std::move(password);
-  store->GetLoginsByPassword(password_, this);
+  store_->GetLoginsByPassword(password_, this);
 }
 
 void LeakDetectionDelegateHelper::OnGetPasswordStoreResults(
     std::vector<std::unique_ptr<autofill::PasswordForm>> results) {
+  if (base::FeatureList::IsEnabled(features::kPasswordCheck)) {
+    base::string16 canonicalized_username = CanonicalizeUsername(username_);
+    for (const auto& form : results) {
+      if (CanonicalizeUsername(form->username_value) ==
+          canonicalized_username) {
+        store_->AddCompromisedCredentials(
+            {form->signon_realm, form->username_value, base::Time::Now(),
+             CompromiseType::kLeaked});
+      }
+    }
+  }
+
   IsSaved is_saved(
       std::any_of(results.begin(), results.end(), [this](const auto& form) {
         return form->origin == url_ && form->username_value == username_;
       }));
 
   IsReused is_reused(results.size() > (is_saved ? 1 : 0));
-  CredentialLeakType leak_type =
-      CreateLeakType(is_saved, is_reused, IsSyncing(true));
-  std::move(callback_).Run(std::move(leak_type), std::move(url_),
+  std::move(callback_).Run(is_saved, is_reused, std::move(url_),
                            std::move(username_));
 }
 

components/password_manager/core/browser/leak_detection_delegate_helper.h

@@ -24,26 +24,29 @@ class LeakDetectionDelegateHelper : public PasswordStoreConsumer {
  public:
   // Type alias for |callback_|.
   using LeakTypeReply =
-      base::OnceCallback<void(CredentialLeakType, GURL, base::string16)>;
+      base::OnceCallback<void(IsSaved, IsReused, GURL, base::string16)>;
 
-  explicit LeakDetectionDelegateHelper(LeakTypeReply callback);
+  LeakDetectionDelegateHelper(scoped_refptr<PasswordStore> store,
+                              LeakTypeReply callback);
   ~LeakDetectionDelegateHelper() override;
 
-  // Request all credentials with |password| from |store|.
-  // Results are password to |OnGetPasswordStoreResults|.
-  void GetCredentialLeakType(PasswordStore* store,
-                             GURL url,
+  // Request all credentials with |password| from the store.
+  // Results are passed to |OnGetPasswordStoreResults|.
+  void ProcessLeakedPassword(GURL url,
                              base::string16 username,
                              base::string16 password);
 
  private:
   // PasswordStoreConsumer:
   // Is called by the |PasswordStore| once all credentials with the specific
-  // password are retrieved. Determine the |CredentialLeakType and invokes
+  // password are retrieved. Determine the credential type and invokes
   // |callback_| when done.
+  // All the saved credentials with the same username and password are stored to
+  // the database.
   void OnGetPasswordStoreResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results) override;
 
+  scoped_refptr<password_manager::PasswordStore> store_;
   LeakTypeReply callback_;
   GURL url_;
   base::string16 username_;

components/password_manager/core/browser/leak_detection_delegate_helper_unittest.cc

@@ -8,8 +8,12 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/mock_callback.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/task_environment.h"
+#include "components/password_manager/core/browser/form_parsing/form_parser.h"
 #include "components/password_manager/core/browser/leak_detection_dialog_utils.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/common/password_manager_features.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -27,17 +31,19 @@ namespace password_manager {
 namespace {
 constexpr char kLeakedPassword[] = "leaked_password";
 constexpr char kLeakedUsername[] = "leaked_username";
+constexpr char kLeakedUsernameNonCanonicalized[] = "Leaked_Username@gmail.com";
 constexpr char kOtherUsername[] = "other_username";
 constexpr char kLeakedOrigin[] = "https://www.leaked_origin.de/login";
 constexpr char kOtherOrigin[] = "https://www.other_origin.de/login";
 
-// Creates a |PasswordForm| with the supplied |origin| and |username|. The
-// password is always set to |kLeakedPassword|.
-PasswordForm CreateForm(base::StringPiece origin, base::StringPiece username) {
+// Creates a |PasswordForm| with the supplied |origin|, |username|, |password|.
+PasswordForm CreateForm(base::StringPiece origin,
+                        base::StringPiece username,
+                        base::StringPiece password = kLeakedPassword) {
   PasswordForm form;
   form.origin = GURL(ASCIIToUTF16(origin));
   form.username_value = ASCIIToUTF16(username);
-  form.password_value = ASCIIToUTF16(kLeakedPassword);
+  form.password_value = ASCIIToUTF16(password);
   form.signon_realm = form.origin.GetOrigin().spec();
   return form;
 }
@@ -45,7 +51,7 @@ PasswordForm CreateForm(base::StringPiece origin, base::StringPiece username) {
 // Used to mimic the callback of the |PasswordStore|.  Converts the vector of
 // |PasswordForm|s to a vector of unique pointers to |PasswordForm|s.
 ACTION_P(InvokeConsumerWithPasswordForms, forms) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> results;
+  std::vector<std::unique_ptr<PasswordForm>> results;
   for (const auto& form : forms) {
     results.push_back(std::make_unique<PasswordForm>(form));
   }
@@ -62,9 +68,10 @@ class LeakDetectionDelegateHelperTest : public testing::Test {
  protected:
   void SetUp() override {
     store_ = new testing::StrictMock<MockPasswordStore>;
+    CHECK(store_->Init(syncer::SyncableService::StartSyncFlare(), nullptr));
 
     delegate_helper_ =
-        std::make_unique<LeakDetectionDelegateHelper>(callback_.Get());
+        std::make_unique<LeakDetectionDelegateHelper>(store_, callback_.Get());
   }
 
   void TearDown() override {
@@ -74,7 +81,7 @@ class LeakDetectionDelegateHelperTest : public testing::Test {
 
   // Initiates determining the credential leak type.
   void InitiateGetCredentialLeakType() {
-    delegate_helper_->GetCredentialLeakType(store_.get(), GURL(kLeakedOrigin),
+    delegate_helper_->ProcessLeakedPassword(GURL(kLeakedOrigin),
                                             ASCIIToUTF16(kLeakedUsername),
                                             ASCIIToUTF16(kLeakedPassword));
   }
@@ -90,12 +97,13 @@ class LeakDetectionDelegateHelperTest : public testing::Test {
   // Set the expectation for the |CredentialLeakType| in the callback_.
   void SetOnShowLeakDetectionNotificationExpectation(IsSaved is_saved,
                                                      IsReused is_reused) {
-    EXPECT_CALL(callback_,
-                Run(CreateLeakType(is_saved, is_reused, IsSyncing(true)),
-                    GURL(kLeakedOrigin), ASCIIToUTF16(kLeakedUsername)))
+    EXPECT_CALL(callback_, Run(is_saved, is_reused, GURL(kLeakedOrigin),
+                               ASCIIToUTF16(kLeakedUsername)))
         .Times(1);
   }
 
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   MockCallback<LeakDetectionDelegateHelper::LeakTypeReply> callback_;
   scoped_refptr<MockPasswordStore> store_;
   std::unique_ptr<LeakDetectionDelegateHelper> delegate_helper_;
@@ -177,4 +185,43 @@ TEST_F(LeakDetectionDelegateHelperTest, ReusedPassword) {
   InitiateGetCredentialLeakType();
 }
 
+// All the credentials with the same username/password are marked as leaked.
+TEST_F(LeakDetectionDelegateHelperTest, SaveLeakedCredentials) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kPasswordCheck);
+
+  SetGetLoginByPasswordConsumerInvocation(
+      {CreateForm(kLeakedOrigin, kLeakedUsername, kLeakedPassword),
+       CreateForm(kOtherOrigin, kLeakedUsername, kLeakedPassword),
+       CreateForm(kLeakedOrigin, kOtherUsername, kLeakedPassword)});
+  SetOnShowLeakDetectionNotificationExpectation(IsSaved(true), IsReused(true));
+  EXPECT_CALL(*store_, AddCompromisedCredentialsImpl(CompromisedCredentials{
+                           GetSignonRealm(GURL(kLeakedOrigin)),
+                           ASCIIToUTF16(kLeakedUsername), base::Time::Now(),
+                           CompromiseType::kLeaked}));
+  EXPECT_CALL(*store_, AddCompromisedCredentialsImpl(CompromisedCredentials{
+                           GetSignonRealm(GURL(kOtherOrigin)),
+                           ASCIIToUTF16(kLeakedUsername), base::Time::Now(),
+                           CompromiseType::kLeaked}));
+  InitiateGetCredentialLeakType();
+  task_environment_.RunUntilIdle();
+}
+
+// Credential with the same canonicalized username marked as leaked.
+TEST_F(LeakDetectionDelegateHelperTest, SaveLeakedCredentialsCanonicalized) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kPasswordCheck);
+
+  SetGetLoginByPasswordConsumerInvocation({CreateForm(
+      kOtherOrigin, kLeakedUsernameNonCanonicalized, kLeakedPassword)});
+  SetOnShowLeakDetectionNotificationExpectation(IsSaved(false), IsReused(true));
+
+  EXPECT_CALL(*store_, AddCompromisedCredentialsImpl(CompromisedCredentials{
+                           GetSignonRealm(GURL(kOtherOrigin)),
+                           ASCIIToUTF16(kLeakedUsernameNonCanonicalized),
+                           base::Time::Now(), CompromiseType::kLeaked}));
+  InitiateGetCredentialLeakType();
+  task_environment_.RunUntilIdle();
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/leak_detection_delegate_unittest.cc

@@ -14,6 +14,7 @@
 #include "components/password_manager/core/browser/leak_detection/mock_leak_detection_check_factory.h"
 #include "components/password_manager/core/browser/leak_detection_delegate.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
@@ -32,7 +33,17 @@ using base::ASCIIToUTF16;
 using testing::_;
 using testing::ByMove;
 using testing::Eq;
+using testing::NiceMock;
 using testing::Return;
+using testing::WithArg;
+
+ACTION_P(InvokeConsumerWithPasswordForms, forms) {
+  std::vector<std::unique_ptr<autofill::PasswordForm>> results;
+  for (const auto& form : forms) {
+    results.push_back(std::make_unique<autofill::PasswordForm>(form));
+  }
+  arg0->OnGetPasswordStoreResults(std::move(results));
+}
 
 autofill::PasswordForm CreateTestForm() {
   autofill::PasswordForm form;
@@ -161,7 +172,8 @@ TEST_F(LeakDetectionDelegateTest, LeakDetectionDoneWithFalseResult) {
   const autofill::PasswordForm form = CreateTestForm();
 
   EXPECT_CALL(factory(), TryCreateLeakCheck)
-      .WillOnce(Return(ByMove(std::make_unique<MockLeakDetectionCheck>())));
+      .WillOnce(
+          Return(ByMove(std::make_unique<NiceMock<MockLeakDetectionCheck>>())));
   delegate().StartLeakCheck(form);
 
   EXPECT_CALL(client(), NotifyUserCredentialsWereLeaked).Times(0);
@@ -177,8 +189,14 @@ TEST_F(LeakDetectionDelegateTest, LeakDetectionDoneWithTrueResult) {
   LeakDetectionDelegateInterface* delegate_interface = &delegate();
   const autofill::PasswordForm form = CreateTestForm();
 
+  EXPECT_CALL(client(), GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store()));
+  EXPECT_CALL(*store(), GetLoginsByPassword)
+      .WillRepeatedly(WithArg<1>(InvokeConsumerWithPasswordForms(
+          std::vector<autofill::PasswordForm>())));
   EXPECT_CALL(factory(), TryCreateLeakCheck)
-      .WillOnce(Return(ByMove(std::make_unique<MockLeakDetectionCheck>())));
+      .WillOnce(
+          Return(ByMove(std::make_unique<NiceMock<MockLeakDetectionCheck>>())));
   delegate().StartLeakCheck(form);
 
   EXPECT_CALL(client(),
@@ -201,8 +219,12 @@ TEST_F(LeakDetectionDelegateTest, LeakHistoryAddCredentials) {
 
   EXPECT_CALL(client(), GetProfilePasswordStore())
       .WillRepeatedly(testing::Return(store()));
+  EXPECT_CALL(*store(), GetLoginsByPassword)
+      .WillRepeatedly(WithArg<1>(InvokeConsumerWithPasswordForms(
+          std::vector<autofill::PasswordForm>{form})));
   EXPECT_CALL(factory(), TryCreateLeakCheck)
-      .WillOnce(Return(ByMove(std::make_unique<MockLeakDetectionCheck>())));
+      .WillOnce(
+          Return(ByMove(std::make_unique<NiceMock<MockLeakDetectionCheck>>())));
   delegate().StartLeakCheck(form);
 
   EXPECT_CALL(client(), NotifyUserCredentialsWereLeaked(_, form.origin));

components/password_manager/core/browser/mock_password_feature_manager.h

@@ -16,7 +16,6 @@ class MockPasswordFeatureManager : public PasswordFeatureManager {
   ~MockPasswordFeatureManager() override;
 
   MOCK_CONST_METHOD0(IsGenerationEnabled, bool());
-  MOCK_CONST_METHOD0(ShouldCheckReuseOnLeakDetection, bool());
 
   MOCK_CONST_METHOD0(IsOptedInForAccountStorage, bool());
   MOCK_CONST_METHOD0(ShouldShowAccountStorageOptIn, bool());

components/password_manager/core/browser/password_feature_manager.h

@@ -18,11 +18,6 @@ class PasswordFeatureManager {
 
   virtual bool IsGenerationEnabled() const = 0;
 
-  // Whether we should, upon the detection of a leaked password, check if the
-  // same password is reused on other website. That's used only for the UI
-  // string.
-  virtual bool ShouldCheckReuseOnLeakDetection() const = 0;
-
   // Whether the current signed-in user (aka unconsented primary account) has
   // opted in to use the Google account storage for passwords (as opposed to
   // local/profile storage).

components/password_manager/core/browser/password_feature_manager_impl.cc

@@ -29,20 +29,6 @@ bool PasswordFeatureManagerImpl::IsGenerationEnabled() const {
   }
 }
 
-bool PasswordFeatureManagerImpl::ShouldCheckReuseOnLeakDetection() const {
-  switch (password_manager_util::GetPasswordSyncState(sync_service_)) {
-    // We currently check the reuse of the leaked password only for users who
-    // can access passwords.google.com. Therefore, if the credentials are not
-    // synced, no need to check for password use.
-    case NOT_SYNCING:
-    case SYNCING_WITH_CUSTOM_PASSPHRASE:
-      return false;
-    case SYNCING_NORMAL_ENCRYPTION:
-    case ACCOUNT_PASSWORDS_ACTIVE_NORMAL_ENCRYPTION:
-      return true;
-  }
-}
-
 bool PasswordFeatureManagerImpl::IsOptedInForAccountStorage() const {
   return password_manager_util::IsOptedInForAccountStorage(pref_service_,
                                                            sync_service_);

components/password_manager/core/browser/password_feature_manager_impl.h

@@ -26,8 +26,6 @@ class PasswordFeatureManagerImpl : public PasswordFeatureManager {
 
   bool IsGenerationEnabled() const override;
 
-  bool ShouldCheckReuseOnLeakDetection() const override;
-
   bool IsOptedInForAccountStorage() const override;
   bool ShouldShowAccountStorageOptIn() const override;
   void SetAccountStorageOptIn(bool opt_in) override;

ios/web_view/internal/passwords/web_view_password_feature_manager.h

@@ -18,8 +18,6 @@ class WebViewPasswordFeatureManager
 
   bool IsGenerationEnabled() const override;
 
-  bool ShouldCheckReuseOnLeakDetection() const override;
-
   bool IsOptedInForAccountStorage() const override;
   bool ShouldShowAccountStorageOptIn() const override;
   void SetAccountStorageOptIn(bool opt_in) override;

ios/web_view/internal/passwords/web_view_password_feature_manager.mm

@@ -16,10 +16,6 @@ bool WebViewPasswordFeatureManager::IsGenerationEnabled() const {
   return false;
 }
 
-bool WebViewPasswordFeatureManager::ShouldCheckReuseOnLeakDetection() const {
-  return false;
-}
-
 bool WebViewPasswordFeatureManager::IsOptedInForAccountStorage() const {
   return false;
 }