[Password Manager] Use SavedPasswordsPresenter in password imports.

This cl also introduced a new type (Imported) to distinguish imported passwords in the store.

Bug: 1325290
Change-Id: I5b0fa4df933d23b997da23752423bfc9c020aa16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3782625
Commit-Queue: Elias Khsheibun <eliaskh@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028385}

chrome/browser/ui/passwords/settings/password_manager_porter.cc

@@ -4,10 +4,7 @@
 
 #include "chrome/browser/ui/passwords/settings/password_manager_porter.h"
 
-#include <iterator>
-#include <memory>
 #include <string>
-#include <utility>
 #include <vector>
 
 #include "base/auto_reset.h"
@@ -17,19 +14,15 @@
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
-#include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/password_manager/core/browser/export/password_manager_exporter.h"
 #include "components/password_manager/core/browser/import/csv_password_sequence.h"
-#include "components/password_manager/core/browser/password_store_interface.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/web_contents.h"
-#include "net/base/filename_util.h"
 #include "ui/base/l10n/l10n_util.h"
-#include "url/gurl.h"
 
 #if BUILDFLAG(IS_WIN)
 #endif
@@ -69,7 +62,9 @@ base::FilePath GetDefaultFilepathForPasswordFile(
 // A helper class for reading the passwords that have been imported.
 class PasswordImportConsumer {
  public:
-  explicit PasswordImportConsumer(Profile* profile);
+  explicit PasswordImportConsumer(
+      Profile* profile,
+      raw_ptr<password_manager::SavedPasswordsPresenter> presenter);
 
   PasswordImportConsumer(const PasswordImportConsumer&) = delete;
   PasswordImportConsumer& operator=(const PasswordImportConsumer&) = delete;
@@ -78,26 +73,25 @@ class PasswordImportConsumer {
 
  private:
   raw_ptr<Profile> profile_;
+  const raw_ptr<password_manager::SavedPasswordsPresenter> presenter_;
   SEQUENCE_CHECKER(sequence_checker_);
 };
 
-PasswordImportConsumer::PasswordImportConsumer(Profile* profile)
-    : profile_(profile) {}
+PasswordImportConsumer::PasswordImportConsumer(
+    Profile* profile,
+    raw_ptr<password_manager::SavedPasswordsPresenter> presenter)
+    : profile_(profile), presenter_(presenter) {}
 
 void PasswordImportConsumer::ConsumePasswords(
     password_manager::mojom::CSVPasswordSequencePtr seq) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!seq)
     return;
 
-  scoped_refptr<password_manager::PasswordStoreInterface> store(
-      PasswordStoreFactory::GetForProfile(profile_,
-                                          ServiceAccessType::EXPLICIT_ACCESS));
-  if (!store)
-    return;
-
-  for (const auto& pwd : seq->csv_passwords)
-    store->AddLogin(pwd.ToPasswordForm());
+  for (const auto& pwd : seq->csv_passwords) {
+    presenter_->AddCredential(password_manager::CredentialUIEntry(pwd),
+                              password_manager::PasswordForm::Type::kImported);
+  }
 
   UMA_HISTOGRAM_COUNTS_1M("PasswordManager.ImportedPasswordsPerUserInCSV",
                           seq->csv_passwords.size());
@@ -240,7 +234,8 @@ void PasswordManagerPorter::FileSelectionCanceled(void* params) {
 void PasswordManagerPorter::ImportPasswordsFromPath(
     const base::FilePath& path) {
   // Set up a |PasswordImportConsumer| to process each password entry.
-  auto form_consumer = std::make_unique<PasswordImportConsumer>(profile_);
+  auto form_consumer =
+      std::make_unique<PasswordImportConsumer>(profile_, presenter_);
   importer_->Import(path,
                     base::BindOnce(&PasswordImportConsumer::ConsumePasswords,
                                    std::move(form_consumer)));

chrome/browser/ui/passwords/settings/password_manager_porter.h

@@ -11,14 +11,14 @@
 #include "base/memory/raw_ptr.h"
 #include "components/password_manager/core/browser/import/password_importer.h"
 #include "components/password_manager/core/browser/ui/export_progress_status.h"
+#include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 
 namespace content {
 class WebContents;
 }
 
 namespace password_manager {
-class SavedPasswordsPresenter;
 class PasswordManagerExporter;
 }  // namespace password_manager
 
@@ -92,7 +92,7 @@ class PasswordManagerPorter : public ui::SelectFileDialog::Listener {
   // We store |presenter_| and
   // |on_export_progress_callback_| to use them to create a new
   // PasswordManagerExporter instance for each export.
-  raw_ptr<password_manager::SavedPasswordsPresenter> presenter_;
+  const raw_ptr<password_manager::SavedPasswordsPresenter> presenter_;
   ProgressCallback on_export_progress_callback_;
 };
 

chrome/browser/ui/passwords/settings/password_manager_porter_unittest.cc

@@ -321,11 +321,10 @@ TEST_P(PasswordManagerPorterStoreTest, Import) {
   ASSERT_TRUE(base::CreateTemporaryFile(&temp_file_path));
   ASSERT_TRUE(base::WriteFile(temp_file_path, tc.csv));
 
-  // No credential provider needed, because this |porter| won't be used for
-  // exporting. No progress callback needed, because UI interaction will be
-  // skipped.
-  PasswordManagerPorter porter(profile.get(),
-                               /*presenter=*/nullptr,
+  password_manager::SavedPasswordsPresenter presenter{test_password_store};
+
+  // No progress callback needed, because UI interaction will be skipped.
+  PasswordManagerPorter porter(profile.get(), &presenter,
                                PasswordManagerPorter::ProgressCallback());
 
   std::unique_ptr<password_manager::PasswordImporter> importer =

components/password_manager/core/browser/export/password_csv_writer_unittest.cc

@@ -51,7 +51,7 @@ TEST(PasswordCSVWriterTest, SerializePasswords_SinglePassword) {
 
   std::vector<CredentialUIEntry> pwds;
   for (const auto& pwd : seq) {
-    pwds.emplace_back(pwd.ToPasswordForm());
+    pwds.emplace_back(pwd);
   }
   EXPECT_THAT(pwds, ElementsAre(FormHasOriginUsernamePassword(
                         "http://example.com/", "Someone", "Secret")));
@@ -74,7 +74,7 @@ TEST(PasswordCSVWriterTest, SerializePasswords_TwoPasswords) {
 
   std::vector<CredentialUIEntry> pwds;
   for (const auto& pwd : seq) {
-    pwds.emplace_back(pwd.ToPasswordForm());
+    pwds.emplace_back(pwd);
   }
   EXPECT_THAT(pwds, ElementsAre(FormHasOriginUsernamePassword(
                                     "http://example.com/", "Someone", "Secret"),

components/password_manager/core/browser/import/csv_password.cc

@@ -29,11 +29,6 @@ std::string ConvertUTF8(base::StringPiece str) {
   return str_copy;
 }
 
-// Convert() converts the result to a 16-bit string.
-std::u16string Convert(const std::string& str) {
-  return base::UTF8ToUTF16(str);
-}
-
 }  // namespace
 
 CSVPassword::CSVPassword() : status_(Status::kSemanticError) {}
@@ -93,27 +88,14 @@ CSVPassword::CSVPassword(CSVPassword&&) = default;
 CSVPassword& CSVPassword::operator=(const CSVPassword&) = default;
 CSVPassword& CSVPassword::operator=(CSVPassword&&) = default;
 
-CSVPassword::Status CSVPassword::GetParseStatus() const {
-  return status_;
+bool operator==(const CSVPassword& lhs, const CSVPassword& rhs) {
+  return lhs.GetParseStatus() == rhs.GetParseStatus() &&
+         lhs.GetPassword() == rhs.GetPassword() &&
+         lhs.GetUsername() == rhs.GetUsername() && lhs.GetURL() == rhs.GetURL();
 }
 
-PasswordForm CSVPassword::ToPasswordForm() const {
-  // Only valid PasswordForms are allowed to be created.
-  DCHECK_EQ(this->GetParseStatus(), Status::kOK);
-  // There is currently no way to import non-HTML credentials.
-  PasswordForm form;
-  form.scheme = PasswordForm::Scheme::kHtml;
-  // Android credentials have a non-standard scheme ("android://"). Hence the
-  // following explicit check is necessary to set |signon_realm| correctly for
-  // both regular and Android credentials.
-  form.signon_realm =
-      IsValidAndroidFacetURI(url_.spec()) ? url_.spec() : GetSignonRealm(url_);
-  form.url = url_;
-  form.username_value = Convert(username_);
-  form.password_value = Convert(password_);
-  form.date_created = base::Time::Now();
-  form.date_password_modified = form.date_created;
-  return form;
+CSVPassword::Status CSVPassword::GetParseStatus() const {
+  return status_;
 }
 
 const std::string& CSVPassword::GetPassword() const {

components/password_manager/core/browser/import/csv_password.h

@@ -9,7 +9,8 @@
 
 #include "base/containers/flat_map.h"
 #include "base/strings/string_piece.h"
-#include "components/password_manager/core/browser/password_form.h"
+
+#include "url/gurl.h"
 
 namespace password_manager {
 
@@ -47,10 +48,6 @@ class CSVPassword {
   // Returns the URL.
   const GURL& GetURL() const;
 
-  // Returns PasswordForm populated with parsed data, if initial parsing
-  // completed successfully.
-  PasswordForm ToPasswordForm() const;
-
  private:
   GURL url_;
   std::string username_;
@@ -59,6 +56,9 @@ class CSVPassword {
   Status status_;
 };
 
+// An exact equality comparison of all the fields is only used for tests.
+bool operator==(const CSVPassword& lhs, const CSVPassword& rhs);
+
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_IMPORT_CSV_PASSWORD_H_

components/password_manager/core/browser/import/csv_password_iterator_unittest.cc

@@ -36,8 +36,7 @@ TEST(CSVPasswordIteratorTest, Operations) {
   {
     base::test::SingleThreadTaskEnvironment env(
         base::test::TaskEnvironment::TimeSource::MOCK_TIME);
-    EXPECT_EQ(iter->ToPasswordForm(),
-              CSVPassword(kColMap, kCSV).ToPasswordForm());
+    EXPECT_EQ(*iter, CSVPassword(kColMap, kCSV));
   }
 
   // Copy.
@@ -95,12 +94,12 @@ TEST(CSVPasswordIteratorTest, MostRowsCorrect) {
   EXPECT_NE(CSVPassword::Status::kOK, check->GetParseStatus());
 
   for (const base::StringPiece& expected_username : kExpectedUsernames) {
-    PasswordForm result = (iter++)->ToPasswordForm();
+    CSVPassword result = *(iter++);
     // Detailed checks of the parsed result are made in the test for
     // CSVPassword. Here only the last field (username) is checked to (1) ensure
     // that lines are processed in the expected sequence, and (2) line breaks
     // are handled as expected (in particular, '\r' alone is not a line break).
-    EXPECT_EQ(base::ASCIIToUTF16(expected_username), result.username_value);
+    EXPECT_EQ(expected_username, result.GetUsername());
   }
 }
 
@@ -121,8 +120,7 @@ TEST(CSVPasswordIteratorTest, LastRowCorrect) {
 
   // The iterator should skip all the faulty rows and land on the last one.
   EXPECT_EQ(CSVPassword::Status::kOK, iter->GetParseStatus());
-  PasswordForm pf = iter->ToPasswordForm();
-  EXPECT_EQ("http://no-failure.example.com/", pf.signon_realm);
+  EXPECT_EQ("http://no-failure.example.com/", iter->GetURL());
 
   iter++;
   // After iterating over all lines, there is no more data to parse.

components/password_manager/core/browser/import/csv_password_sequence_unittest.cc

@@ -9,7 +9,6 @@
 #include <utility>
 
 #include "base/strings/utf_string_conversions.h"
-#include "components/password_manager/core/browser/password_form.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -108,11 +107,10 @@ TEST(CSVPasswordSequenceTest, Iteration) {
   size_t order = 0;
   for (const CSVPassword& pwd : seq) {
     ASSERT_LT(order, std::size(kExpectedCredentials));
-    PasswordForm parsed = pwd.ToPasswordForm();
     const auto& expected = kExpectedCredentials[order];
-    EXPECT_EQ(GURL(expected.url), parsed.url);
-    EXPECT_EQ(base::ASCIIToUTF16(expected.username), parsed.username_value);
-    EXPECT_EQ(base::ASCIIToUTF16(expected.password), parsed.password_value);
+    EXPECT_EQ(GURL(expected.url), pwd.GetURL());
+    EXPECT_EQ(expected.username, pwd.GetUsername());
+    EXPECT_EQ(expected.password, pwd.GetPassword());
     ++order;
   }
 }
@@ -125,10 +123,10 @@ TEST(CSVPasswordSequenceTest, MissingEolAtEof) {
   EXPECT_EQ(CSVPassword::Status::kOK, seq.result());
 
   ASSERT_EQ(1, std::distance(seq.begin(), seq.end()));
-  PasswordForm parsed = seq.begin()->ToPasswordForm();
-  EXPECT_EQ(GURL("http://a.com"), parsed.url);
-  EXPECT_EQ(u"l", parsed.username_value);
-  EXPECT_EQ(u"p", parsed.password_value);
+  CSVPassword pwd = *seq.begin();
+  EXPECT_EQ(GURL("http://a.com"), pwd.GetURL());
+  EXPECT_EQ("l", pwd.GetUsername());
+  EXPECT_EQ("p", pwd.GetPassword());
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/import/csv_password_unittest.cc

@@ -28,15 +28,10 @@ TEST(CSVPasswordTest, Construction) {
   };
   // Use const to check that ParseValid does not mutate the CSVPassword.
   const CSVPassword csv_pwd(kColMap, "http://example.com,user,password");
-  const PasswordForm result = csv_pwd.ToPasswordForm();
   const GURL expected_origin("http://example.com");
-  EXPECT_EQ(expected_origin, result.url);
-  EXPECT_EQ(expected_origin.DeprecatedGetOriginAsURL().spec(),
-            result.signon_realm);
-  EXPECT_EQ(u"user", result.username_value);
-  EXPECT_EQ(u"password", result.password_value);
-  EXPECT_EQ(base::Time::Now(), result.date_created);
-  EXPECT_EQ(base::Time::Now(), result.date_password_modified);
+  EXPECT_EQ(expected_origin, csv_pwd.GetURL());
+  EXPECT_EQ("user", csv_pwd.GetUsername());
+  EXPECT_EQ("password", csv_pwd.GetPassword());
 }
 
 struct TestCase {
@@ -107,21 +102,17 @@ class CSVPasswordTestSuccess : public ::testing::TestWithParam<TestCase> {
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 };
 
-TEST_P(CSVPasswordTestSuccess, ShouldParseToPasswordForm) {
+TEST_P(CSVPasswordTestSuccess, ShouldParse) {
   const TestCase& test_case = GetParam();
   SCOPED_TRACE(test_case.name);
   const CSVPassword csv_pwd(test_case.map, test_case.csv);
   EXPECT_EQ(Status::kOK, csv_pwd.GetParseStatus());
 
-  const PasswordForm result = csv_pwd.ToPasswordForm();
-
   const GURL expected_origin(test_case.origin);
-  EXPECT_EQ(expected_origin, result.url);
-  EXPECT_EQ(test_case.signon_realm, result.signon_realm);
 
-  EXPECT_EQ(base::UTF8ToUTF16(test_case.username), result.username_value);
-  EXPECT_EQ(base::UTF8ToUTF16(test_case.password), result.password_value);
-  EXPECT_EQ(base::Time::Now(), result.date_created);
+  EXPECT_EQ(expected_origin, csv_pwd.GetURL());
+  EXPECT_EQ(test_case.username, csv_pwd.GetUsername());
+  EXPECT_EQ(test_case.password, csv_pwd.GetPassword());
 }
 
 INSTANTIATE_TEST_SUITE_P(

components/password_manager/core/browser/import/password_csv_reader_fuzzer.cc

@@ -10,7 +10,6 @@
 #include "base/at_exit.h"
 #include "base/i18n/icu_util.h"
 #include "components/password_manager/core/browser/import/csv_password_sequence.h"
-#include "components/password_manager/core/browser/password_form.h"
 
 namespace password_manager {
 
@@ -42,15 +41,15 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   CHECK(IsValid(seq.result()))
       << "Invalid parsing result of the whole sequence: "
       << static_cast<int>(seq.result());
-  PasswordForm copy;
+  CSVPassword copy;
   for (const auto& pwd : seq) {
     const CSVPassword::Status status = pwd.GetParseStatus();
     CHECK(IsValid(status)) << "Invalid parsing result of one row: "
                            << static_cast<int>(status);
     if (status == CSVPassword::Status::kOK) {
       // Copy the parsed password to access all its data members and allow the
       // ASAN to detect any corrupted memory inside.
-      copy = pwd.ToPasswordForm();
+      copy = pwd;
     }
   }
   return 0;