Using NewPasswordFormManager for saving manual fallbacks.

Manual fallback with NPFM is implemented similar to PFM, namely:
1.The user is typing
2.FormData is extracted and sent to the browser.
3.PasswordManager::ShowManualFallbackForSaving is called
 a.NPFM is cloned
 b.Received FormData is considered to be submitted form
 c.Cloned NPFM is moved to UI

Along the way, GetSignonRealm was extracted to the util file.

Bug: 831123
Change-Id: Ibbb30ccd1a1e8084e378480cd2ceccca4eaa93d4
Reviewed-on: https://chromium-review.googlesource.com/1238896
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594686}

components/password_manager/core/browser/form_parsing/form_parser.cc

@@ -665,7 +665,7 @@ std::unique_ptr<PasswordForm> AssemblePasswordForm(
   // Create the PasswordForm and set data not related to specific fields.
   auto result = std::make_unique<PasswordForm>();
   result->origin = form_data.origin;
-  result->signon_realm = form_data.origin.GetOrigin().spec();
+  result->signon_realm = GetSignonRealm(form_data.origin);
   result->action = form_data.action;
   result->form_data = form_data;
   result->all_possible_passwords = std::move(all_possible_passwords);
@@ -769,4 +769,14 @@ std::unique_ptr<PasswordForm> ParseFormData(
       std::move(all_possible_usernames), form_predictions);
 }
 
+std::string GetSignonRealm(const GURL& url) {
+  GURL::Replacements rep;
+  rep.ClearUsername();
+  rep.ClearPassword();
+  rep.ClearQuery();
+  rep.ClearRef();
+  rep.SetPathStr(std::string());
+  return url.ReplaceComponents(rep).spec();
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/form_parsing/form_parser.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "components/password_manager/core/browser/form_parsing/password_field_prediction.h"
+#include "url/gurl.h"
 
 namespace autofill {
 struct FormData;
@@ -40,6 +41,10 @@ std::unique_ptr<autofill::PasswordForm> ParseFormData(
     const FormPredictions* form_predictions,
     FormParsingMode mode);
 
+// Returns the value of PasswordForm::signon_realm for an HTML form with the
+// origin |url|.
+std::string GetSignonRealm(const GURL& url);
+
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_FORM_PARSING_IOS_FORM_PARSER_H_

components/password_manager/core/browser/form_parsing/form_parser_unittest.cc

@@ -1439,6 +1439,25 @@ TEST(FormParserTest, HistogramsForUsernameDetectionMethod) {
   }
 }
 
+TEST(FormParserTest, GetSignonRealm) {
+  struct TestCase {
+    const char* input;
+    const char* expected_output;
+  } test_cases[]{
+      {"http://example.com/", "http://example.com/"},
+      {"http://example.com/signup", "http://example.com/"},
+      {"https://google.com/auth?a=1#b", "https://google.com/"},
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    SCOPED_TRACE(testing::Message("Input: ")
+                 << test_case.input << " "
+                 << "Expected output: " << test_case.expected_output);
+    GURL input(test_case.input);
+    EXPECT_EQ(test_case.expected_output, GetSignonRealm(input));
+  }
+}
+
 }  // namespace
 
 }  // namespace password_manager

components/password_manager/core/browser/new_password_form_manager.cc

@@ -160,6 +160,7 @@ bool NewPasswordFormManager::DoesManage(
     const PasswordManagerDriver* driver) const {
   if (driver != driver_.get())
     return false;
+
   if (observed_form_.is_form_tag != form.is_form_tag)
     return false;
   // All unowned input elements are considered as one synthetic form.
@@ -305,8 +306,7 @@ void NewPasswordFormManager::PermanentlyBlacklist() {
   if (!new_blacklisted_) {
     new_blacklisted_ = std::make_unique<PasswordForm>();
     new_blacklisted_->origin = observed_form_.origin;
-    // The following method of finding |signon_realm| is correct for HTML forms.
-    new_blacklisted_->signon_realm = observed_form_.origin.GetOrigin().spec();
+    new_blacklisted_->signon_realm = GetSignonRealm(observed_form_.origin);
     blacklisted_matches_.push_back(new_blacklisted_.get());
   }
   form_saver_->PermanentlyBlacklist(new_blacklisted_.get());
@@ -342,6 +342,40 @@ NewPasswordFormManager::GetDrivers() const {
   return {driver_};
 }
 
+std::unique_ptr<NewPasswordFormManager> NewPasswordFormManager::Clone() {
+  // Fetcher is cloned to avoid re-fetching data from PasswordStore.
+  std::unique_ptr<FormFetcher> fetcher = form_fetcher_->Clone();
+
+  // Some data is filled through the constructor. No PasswordManagerDriver is
+  // needed, because the UI does not need any functionality related to the
+  // renderer process, to which the driver serves as an interface. The full
+  // |observed_form_| needs to be copied, because it is used to create the
+  // blacklisting entry if needed.
+  auto result = std::make_unique<NewPasswordFormManager>(
+      client_, base::WeakPtr<PasswordManagerDriver>(), observed_form_,
+      fetcher.get(), form_saver_->Clone());
+
+  result->metrics_recorder_ = metrics_recorder_;
+
+  // The constructor only can take a weak pointer to the fetcher, so moving the
+  // owning one needs to happen explicitly.
+  result->owned_form_fetcher_ = std::move(fetcher);
+
+  // These data members all satisfy:
+  //   (1) They could have been changed by |*this| between its construction and
+  //       calling Clone().
+  //   (2) They are potentially used in the clone as the clone is used in the UI
+  //       code.
+  //   (3) They are not changed during ProcessMatches, triggered at some point
+  //       by the cloned FormFetcher.
+  result->has_generated_password_ = has_generated_password_;
+  result->user_action_ = user_action_;
+  result->votes_uploader_ = votes_uploader_;
+  result->predictions_ = predictions_;
+
+  return result;
+}
+
 void NewPasswordFormManager::ProcessMatches(
     const std::vector<const PasswordForm*>& non_federated,
     size_t filtered_count) {

components/password_manager/core/browser/new_password_form_manager.h

@@ -122,13 +122,19 @@ class NewPasswordFormManager : public PasswordFormManagerInterface,
   bool RetryPasswordFormPasswordUpdate() const override;
   std::vector<base::WeakPtr<PasswordManagerDriver>> GetDrivers() const override;
 
+  // Create a copy of |*this| which can be passed to the code handling
+  // save-password related UI. This omits some parts of the internal data, so
+  // the result is not identical to the original.
+  // TODO(crbug.com/739366): Replace with translating one appropriate class into
+  // another one.
+  std::unique_ptr<NewPasswordFormManager> Clone();
+
 #if defined(UNIT_TEST)
   static void set_wait_for_server_predictions_for_filling(bool value) {
     wait_for_server_predictions_for_filling_ = value;
   }
 
   FormSaver* form_saver() { return form_saver_.get(); }
-
 #endif
 
   // TODO(https://crbug.com/831123): Remove it when the old form parsing is

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -828,4 +828,20 @@ TEST_F(NewPasswordFormManagerTest, PermanentlyBlacklist) {
             new_blacklisted_form->signon_realm);
 }
 
+TEST_F(NewPasswordFormManagerTest, Clone) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
+  fetcher_->SetNonFederated({}, 0u);
+
+  std::unique_ptr<NewPasswordFormManager> cloned_manager =
+      form_manager_->Clone();
+
+  EXPECT_TRUE(cloned_manager->DoesManage(observed_form_, nullptr));
+  EXPECT_TRUE(cloned_manager->GetFormFetcher());
+  // Check that |form_fetcher| was cloned.
+  EXPECT_NE(form_manager_->GetFormFetcher(), cloned_manager->GetFormFetcher());
+
+  EXPECT_EQ(form_manager_->metrics_recorder(),
+            cloned_manager->metrics_recorder());
+}
+
 }  // namespace  password_manager

components/password_manager/core/browser/password_manager.cc

@@ -228,6 +228,29 @@ PasswordFormManager* FindMatchedManager(
              : matched_manager_it->get();
 }
 
+std::unique_ptr<PasswordFormManager> FindAndCloneMatchedPasswordFormManager(
+    const PasswordForm& password_form,
+    const std::vector<std::unique_ptr<PasswordFormManager>>&
+        pending_login_managers,
+    const password_manager::PasswordManagerDriver* driver) {
+  PasswordFormManager* matched_manager = FindMatchedManager(
+      password_form, pending_login_managers, driver, nullptr);
+  if (!matched_manager)
+    return nullptr;
+  // TODO(crbug.com/741537): Process manual saving request even if there is
+  // still no response from the store.
+  if (matched_manager->GetFormFetcher()->GetState() ==
+      FormFetcher::State::WAITING) {
+    return nullptr;
+  }
+
+  std::unique_ptr<PasswordFormManager> manager = matched_manager->Clone();
+  PasswordForm form(password_form);
+  form.preferred = true;
+  manager->ProvisionallySave(form);
+  return manager;
+}
+
 // Returns true if the user needs to be prompted before a password can be
 // saved (instead of automatically saving the password), based on inspecting
 // the state of |manager|.
@@ -260,6 +283,47 @@ bool IsThereVisiblePasswordField(const FormData& form) {
   return false;
 }
 
+// Finds the matched form manager for |form| in |form_managers|.
+NewPasswordFormManager* FindMatchedManager(
+    const FormData& form,
+    const std::vector<std::unique_ptr<NewPasswordFormManager>>& form_managers,
+    const PasswordManagerDriver* driver) {
+  for (const auto& form_manager : form_managers) {
+    if (form_manager->DoesManage(form, driver))
+      return form_manager.get();
+  }
+  return nullptr;
+}
+
+// Returns a form manager that is ready to save/update credentials, provided
+// that |form| is submitted form. Namely 1. Finds form manager from
+// |form_managers| that manages |form| 2. Clones it. 3. Passes |form| as
+// submitted form to the cloned form manager.
+std::unique_ptr<NewPasswordFormManager>
+FindAndCloneMatchedNewPasswordFormManager(
+    const FormData& form,
+    const std::vector<std::unique_ptr<NewPasswordFormManager>>& form_managers,
+    const PasswordManagerDriver* driver) {
+  NewPasswordFormManager* matched_manager =
+      FindMatchedManager(form, form_managers, driver);
+  if (!matched_manager)
+    return nullptr;
+  // TODO(crbug.com/741537): Process manual saving request even if there is
+  // still no response from the store.
+  if (matched_manager->GetFormFetcher()->GetState() ==
+      FormFetcher::State::WAITING) {
+    return nullptr;
+  }
+
+  std::unique_ptr<NewPasswordFormManager> manager = matched_manager->Clone();
+  // Cloned NewPasswordFormManager doesn't have |driver|, so nullptr must be
+  // passed to ensure that the |form| is managed.
+  if (manager->SetSubmittedFormIfIsManaged(form, nullptr))
+    return manager;
+
+  return nullptr;
+}
+
 }  // namespace
 
 // static
@@ -308,7 +372,7 @@ void PasswordManager::RegisterLocalPrefs(PrefRegistrySimple* registry) {
 PasswordManager::PasswordManager(PasswordManagerClient* client)
     : client_(client),
       is_new_form_parsing_for_saving_enabled_(base::FeatureList::IsEnabled(
-          password_manager::features::kNewPasswordFormParsingForSaving)) {
+          features::kNewPasswordFormParsingForSaving)) {
   DCHECK(client_);
 }
 
@@ -462,8 +526,7 @@ void PasswordManager::ProvisionallySavePassword(
 
 void PasswordManager::UpdateFormManagers() {
   std::vector<PasswordFormManagerInterface*> form_managers;
-  if (base::FeatureList::IsEnabled(
-          password_manager::features::kNewPasswordFormParsing)) {
+  if (base::FeatureList::IsEnabled(features::kNewPasswordFormParsing)) {
     for (const auto& form_manager : form_managers_)
       form_managers.push_back(form_manager.get());
   } else {
@@ -586,30 +649,25 @@ void PasswordManager::ShowManualFallbackForSaving(
       !client_->GetStoreResultFilter()->ShouldSave(password_form))
     return;
 
-  PasswordFormManager* matched_manager = FindMatchedManager(
-      password_form, pending_login_managers_, driver, nullptr);
-  if (!matched_manager)
-    return;
-  // TODO(crbug.com/741537): Process manual saving request even if there is
-  // still no response from the store.
-  if (matched_manager->GetFormFetcher()->GetState() ==
-      FormFetcher::State::WAITING) {
-    return;
+  std::unique_ptr<PasswordFormManagerInterface> manager = nullptr;
+  if (is_new_form_parsing_for_saving_enabled_) {
+    manager = FindAndCloneMatchedNewPasswordFormManager(password_form.form_data,
+                                                        form_managers_, driver);
+  } else {
+    manager = FindAndCloneMatchedPasswordFormManager(
+        password_form, pending_login_managers_, driver);
   }
-
-  std::unique_ptr<PasswordFormManager> manager = matched_manager->Clone();
-  PasswordForm form(password_form);
-  form.preferred = true;
-  manager->ProvisionallySave(form);
+  if (!manager)
+    return;
 
   // Show the fallback if a prompt or a confirmation bubble should be available.
   bool has_generated_password = manager->HasGeneratedPassword();
   if (ShouldPromptUserToSavePassword(*manager) || has_generated_password) {
     bool is_update = IsPasswordUpdate(*manager);
+    manager->GetMetricsRecorder()->RecordShowManualFallbackForSaving(
+        has_generated_password, is_update);
     client_->ShowManualFallbackForSaving(std::move(manager),
                                          has_generated_password, is_update);
-    matched_manager->GetMetricsRecorder()->RecordShowManualFallbackForSaving(
-        has_generated_password, is_update);
   } else {
     HideManualFallbackForSaving();
   }
@@ -642,8 +700,7 @@ void PasswordManager::CreatePendingLoginManagers(
     logger->LogMessage(Logger::STRING_CREATE_LOGIN_MANAGERS_METHOD);
   }
 
-  if (base::FeatureList::IsEnabled(
-          password_manager::features::kNewPasswordFormParsing)) {
+  if (base::FeatureList::IsEnabled(features::kNewPasswordFormParsing)) {
     CreateFormManagers(driver, forms);
   }
 
@@ -758,20 +815,18 @@ void PasswordManager::CreateFormManagers(
     // NewPasswordFormManger instance.
     if (form.is_gaia_with_skip_save_password_form)
       continue;
-    auto form_it =
-        std::find_if(form_managers_.begin(), form_managers_.end(),
-                     [&form, driver](const auto& form_manager) {
-                       return form_manager->DoesManage(form.form_data, driver);
-                     });
-    if (form_it == form_managers_.end()) {
-      new_forms.push_back(&form);
-    } else {
+    NewPasswordFormManager* manager =
+        FindMatchedManager(form.form_data, form_managers_, driver);
+
+    if (manager) {
       // This extra filling is just duplicating redundancy that was in
       // PasswordFormManager, that helps to fix cases when the site overrides
       // filled values.
       // TODO(https://crbug.com/831123): Implement more robust filling and
       // remove the next line.
-      (*form_it)->Fill();
+      manager->Fill();
+    } else {
+      new_forms.push_back(&form);
     }
   }
 

components/password_manager/core/browser/password_manager_unittest.cc

@@ -198,7 +198,7 @@ class PasswordManagerTest : public testing::Test {
     form.username_value = ASCIIToUTF16("googleuser");
     form.password_value = ASCIIToUTF16("p4ssword");
     form.submit_element = ASCIIToUTF16("signIn");
-    form.signon_realm = "http://www.google.com";
+    form.signon_realm = "http://www.google.com/";
 
     // Fill |form.form_data|.
     form.form_data.origin = form.origin;
@@ -209,12 +209,14 @@ class PasswordManagerTest : public testing::Test {
     autofill::FormFieldData field;
     field.name = ASCIIToUTF16("Email");
     field.id = field.name;
+    field.value = ASCIIToUTF16("googleuser");
     field.form_control_type = "text";
     field.unique_renderer_id = 2;
     form.form_data.fields.push_back(field);
 
     field.name = ASCIIToUTF16("Passwd");
     field.id = field.name;
+    field.value = ASCIIToUTF16("p4ssword");
     field.form_control_type = "password";
     field.unique_renderer_id = 3;
     form.form_data.fields.push_back(field);
@@ -271,7 +273,7 @@ class PasswordManagerTest : public testing::Test {
     form.username_value = ASCIIToUTF16("twitter");
     form.password_value = ASCIIToUTF16("password");
     form.submit_element = ASCIIToUTF16("signIn");
-    form.signon_realm = "https://twitter.com";
+    form.signon_realm = "https://twitter.com/";
     return form;
   }
 
@@ -285,7 +287,7 @@ class PasswordManagerTest : public testing::Test {
     form.username_value = ASCIIToUTF16("twitter");
     form.password_value = ASCIIToUTF16("password");
     form.submit_element = ASCIIToUTF16("signIn");
-    form.signon_realm = "https://twitter.com";
+    form.signon_realm = "https://twitter.com/";
     return form;
   }
 
@@ -328,8 +330,7 @@ MATCHER_P(FormMatches, form, "") {
          form.username_value == arg.username_value &&
          form.password_element == arg.password_element &&
          form.password_value == arg.password_value &&
-         form.new_password_element == arg.new_password_element &&
-         form.submit_element == arg.submit_element;
+         form.new_password_element == arg.new_password_element;
 }
 
 TEST_F(PasswordManagerTest, FormSubmitWithOnlyNewPasswordField) {
@@ -2713,4 +2714,47 @@ TEST_F(PasswordManagerTest, MetricForSchemeOfSuccessfulLogins) {
   }
 }
 
+TEST_F(PasswordManagerTest, ManualFallbackForSavingNewParser) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  TurnOnNewParsingForSaving(&scoped_feature_list);
+  NewPasswordFormManager::set_wait_for_server_predictions_for_filling(false);
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm());
+  observed.push_back(form);
+  PasswordForm stored_form = form;
+  stored_form.password_value = ASCIIToUTF16("old_password");
+  EXPECT_CALL(*store_, GetLogins(_, _))
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(2);
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  // The username of the stored form is the same, there should be update bubble.
+  std::unique_ptr<PasswordFormManagerForUI> form_manager_to_save;
+  EXPECT_CALL(client_, ShowManualFallbackForSavingPtr(_, false, true))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save)));
+  manager()->ShowManualFallbackForSaving(&driver_, form);
+  ASSERT_TRUE(form_manager_to_save);
+  EXPECT_THAT(form_manager_to_save->GetPendingCredentials(), FormMatches(form));
+
+  // The username of the stored form is different, there should be save bubble.
+  PasswordForm new_form = form;
+  new_form.username_value = ASCIIToUTF16("another_username");
+  new_form.form_data.fields[0].value = new_form.username_value;
+  EXPECT_CALL(client_, ShowManualFallbackForSavingPtr(_, false, false))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save)));
+  manager()->ShowManualFallbackForSaving(&driver_, new_form);
+  ASSERT_TRUE(form_manager_to_save);
+  EXPECT_THAT(form_manager_to_save->GetPendingCredentials(),
+              FormMatches(new_form));
+
+  // Hide the manual fallback.
+  EXPECT_CALL(client_, HideManualFallbackForSaving());
+  manager()->HideManualFallbackForSaving();
+}
+
 }  // namespace password_manager