[go: nahoru, domu]

[Passwords] Don't do password leak check if it can't be muted

The filed bug report contained several problems:

1) Password leak check ignored broken `PasswordStore`. `PasswordStore`
stored if any leak notification was already shown to the user and
allowed to show the leak notification at most once. Since
`PasswordStore` was broken, the user was prompted with
password leak every time they login to the website.

2) If the saving was disabled, form manager was not cleared and this
meant that every website navigation was form submission.

This lead to Password Manager prompting leaked password dialog on
every navigation.

Move password leak check after checking that password store is
available for write and clear the submitted form even if saving is
disabled.

(cherry picked from commit 01f4347a31d0cabfe12e00717f757c64c5d49aa7)

Bug: b/327621222
Change-Id: Ic43c34f33b0315ae645affee3b5398ecc221007a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5317876
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Din Nezametdinov <shaikhitdin@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1264635}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5340376
Auto-Submit: Din Nezametdinov <shaikhitdin@google.com>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/branch-heads/6312@{#387}
Cr-Branched-From: 6711dcdae48edaf98cbc6964f90fac85b7d9986e-refs/heads/main@{#1262506}
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
index f7c44e3..2483866 100644
--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -1131,17 +1131,6 @@
       submitted_manager->GetPendingCredentials().username_value);
   client_->NotifyOnSuccessfulLogin(submitted_form->username_value);
 
-  // Check for leaks only if there are no muted credentials and it is not a
-  // single username submission (a leak warning may offer an automated password
-  // change, which requires a user to be logged in).
-  if (!HasMutedCredentials(
-          submitted_manager->GetInsecureCredentials(),
-          submitted_manager->GetSubmittedForm()->username_value) &&
-      !IsSingleUsernameSubmission(*submitted_manager->GetSubmittedForm())) {
-    leak_delegate_.StartLeakCheck(LeakDetectionInitiator::kSignInCheck,
-                                  submitted_manager->GetPendingCredentials());
-  }
-
   auto submission_event =
       submitted_manager->GetSubmittedForm()->submission_event;
   metrics_util::LogPasswordSuccessfulSubmissionIndicatorEvent(submission_event);
@@ -1155,6 +1144,17 @@
   if (!able_to_save_passwords)
     return;
 
+  // Check for leaks only if there are no muted credentials and it is not a
+  // single username submission (a leak warning may offer an automated password
+  // change, which requires a user to be logged in).
+  if (!HasMutedCredentials(
+          submitted_manager->GetInsecureCredentials(),
+          submitted_manager->GetSubmittedForm()->username_value) &&
+      !IsSingleUsernameSubmission(*submitted_manager->GetSubmittedForm())) {
+    leak_delegate_.StartLeakCheck(LeakDetectionInitiator::kSignInCheck,
+                                  submitted_manager->GetPendingCredentials());
+  }
+
   password_manager::PasswordReuseManager* reuse_manager =
       client_->GetPasswordReuseManager();
   // May be null in tests.
@@ -1189,6 +1189,9 @@
   CHECK(!submitted_manager->GetPendingCredentials().only_for_fallback);
 
   if (!submitted_manager->IsSavingAllowed()) {
+    // Stop tracking the form if it was not allowed to save credentials at the
+    // submission time.
+    ResetSubmittedManager();
     return;
   }
 
diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
index 0bfb7e9..46d7665 100644
--- a/components/password_manager/core/browser/password_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -1282,6 +1282,69 @@
   store->ShutdownOnUIThread();
 }
 
+// Checks that credentials on the submitted form are not checked for leak when
+// the password store is broken. Broken password store makes it unable to mute
+// the leak notification.
+TEST_P(PasswordManagerTest, BrokenPasswordStorePreventsMutingCredentials) {
+  auto mock_factory =
+      std::make_unique<testing::StrictMock<MockLeakDetectionCheckFactory>>();
+  MockLeakDetectionCheckFactory* weak_factory = mock_factory.get();
+  manager()->set_leak_factory(std::move(mock_factory));
+  auto store = base::MakeRefCounted<PasswordStore>(
+      std::make_unique<FailingPasswordStoreBackend>());
+  store->Init(/*prefs=*/nullptr, /*affiliated_match_helper=*/nullptr);
+  ON_CALL(client_, GetProfilePasswordStore())
+      .WillByDefault(Return(store.get()));
+
+  const FormData form_data = MakeSimpleFormData();
+  std::vector<FormData> observed = {form_data};
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed);
+  task_environment_.RunUntilIdle();
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabled).WillRepeatedly(Return(true));
+  OnPasswordFormSubmitted(form_data);
+
+  // Expect no automatic save/update prompt and no leak check if password store
+  // is not available.
+  EXPECT_CALL(client_, PromptUserToSaveOrUpdatePassword).Times(0);
+  EXPECT_CALL(*weak_factory, TryCreateLeakCheck).Times(0);
+
+  // Now the password manager waits for the navigation to complete.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed);
+  task_environment_.RunUntilIdle();
+  // Objects owned by the manager may keep references to the store - therefore
+  // destroy the manager prior to store destruction.
+  manager_.reset();
+  store->ShutdownOnUIThread();
+}
+
+// Checks that submitted form manager is cleared when saving was disabled at the
+// time of submission.
+TEST_P(PasswordManagerTest, ClearSubmittedManagerIfSavingIsDisabled) {
+  const FormData form_data = MakeSimpleFormData();
+  std::vector<FormData> observed = {form_data};
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed);
+  task_environment_.RunUntilIdle();
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabled).WillRepeatedly(Return(false));
+  OnPasswordFormSubmitted(form_data);
+
+  // Expect no automatic save/update prompt because saving is disabled.
+  EXPECT_CALL(client_, PromptUserToSaveOrUpdatePassword).Times(0);
+
+  // Now the password manager waits for the navigation to complete.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed);
+
+  // Form manager is deleted after successful form submission.
+  EXPECT_TRUE(manager()->form_managers().empty());
+}
+
 // This test verifies a fix for http://crbug.com/236673
 TEST_P(PasswordManagerTest, FormSubmitWithFormOnPreviousPage) {
   PasswordForm first_form(MakeSimpleForm());