Make CSVPassword::Parse differentiate failures

Parsing a single CSV row inside CSVPassword::Parse can fail for two
reasons: bad syntax, or incorrect semantics. So far, both cases have
been reported simply as an error. Soon, the distinction will be needed
to correctly report from CSVPasswordSequence.

Therefore this CL changes the return type of CSVPassword::Parse from
Boolean (whether parsing failed) to a new status enum (how it
failed): OK, Syntax error or Semantic error. This enum describes
a subset of ParserState::Result, but ParserState::Result was not
directly used here, because the CSV logic should not know about
its own client code.

Bug: 934326
Change-Id: I621e2faa38241541d684357b2f54d9f5fb8ed923
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1908528
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714521}

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

@@ -35,12 +35,12 @@ CSVPassword::CSVPassword(const ColumnMap& map, base::StringPiece csv_row)
 
 CSVPassword::~CSVPassword() = default;
 
-bool CSVPassword::Parse(PasswordForm* form) const {
+CSVPassword::Status CSVPassword::Parse(PasswordForm* form) const {
   // |map_| must be an (1) injective and (2) surjective (3) partial map. (3) is
   // enforced by its type, (2) is checked later in the code and (1) follows from
   // (2) and the following size() check.
   if (map_.size() != kLabelCount)
-    return false;
+    return Status::kSemanticError;
 
   size_t field_idx = 0;
   CSVFieldParser parser(row_);
@@ -51,14 +51,14 @@ bool CSVPassword::Parse(PasswordForm* form) const {
   while (parser.HasMoreFields()) {
     base::StringPiece field;
     if (!parser.NextField(&field))
-      return false;
+      return Status::kSyntaxError;
     auto meaning_it = map_.find(field_idx++);
     if (meaning_it == map_.end())
       continue;
     switch (meaning_it->second) {
       case Label::kOrigin:
         if (!base::IsStringASCII(field))
-          return false;
+          return Status::kSyntaxError;
         origin = GURL(field);
         break;
       case Label::kUsername:
@@ -74,9 +74,9 @@ bool CSVPassword::Parse(PasswordForm* form) const {
   // username is permitted to be an empty string, while password and origin are
   // not.
   if (!origin.is_valid() || !username_set || password.empty())
-    return false;
+    return Status::kSemanticError;
   if (!form)
-    return true;
+    return Status::kOK;
   // There is currently no way to import non-HTML credentials.
   form->scheme = PasswordForm::Scheme::kHtml;
   // GURL::GetOrigin() returns an empty GURL for Android credentials due
@@ -89,13 +89,13 @@ bool CSVPassword::Parse(PasswordForm* form) const {
   form->origin = std::move(origin);
   form->username_value = Convert(username);
   form->password_value = Convert(password);
-  return true;
+  return Status::kOK;
 }
 
 PasswordForm CSVPassword::ParseValid() const {
   PasswordForm result;
-  bool success = Parse(&result);
-  DCHECK(success);
+  Status status = Parse(&result);
+  DCHECK_EQ(Status::kOK, status);
   return result;
 }
 

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

@@ -21,6 +21,9 @@ class CSVPassword {
   enum class Label { kOrigin, kUsername, kPassword };
   using ColumnMap = base::flat_map<size_t, Label>;
 
+  // Status describes parsing errors.
+  enum class Status { kOK, kSyntaxError, kSemanticError };
+
   // Number of values in the Label enum.
   static constexpr size_t kLabelCount = 3;
 
@@ -32,10 +35,10 @@ class CSVPassword {
   ~CSVPassword();
 
   // Returns whether the associated CSV row can be parsed successfully.
-  // If returning true and |form| is not null, it also stores the parsed result
-  // in |*form|. It does not return base::Optional<PasswordForm> for efficiency
-  // reasons in cases when the parsed form is not needed.
-  bool Parse(autofill::PasswordForm* form) const;
+  // If returning success and |form| is not null, it also stores the parsed
+  // result in |*form|. It does not return base::Optional<PasswordForm> for
+  // efficiency reasons in cases when the parsed form is not needed.
+  Status Parse(autofill::PasswordForm* form) const;
   // Convenience wrapper around Parse() for cases known to be correctly
   // parseable.
   autofill::PasswordForm ParseValid() const;

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

@@ -9,6 +9,7 @@
 
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/password_manager/core/browser/import/csv_password.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace password_manager {
@@ -75,9 +76,10 @@ TEST(CSVPasswordIteratorTest, Success) {
 
   CSVPasswordIterator check = iter;
   for (size_t i = 0; i < base::size(kExpectedPasswords); ++i) {
-    EXPECT_TRUE((check++)->Parse(nullptr)) << "on line " << i;
+    EXPECT_EQ(CSVPassword::Status::kOK, (check++)->Parse(nullptr))
+        << "on line " << i;
   }
-  EXPECT_FALSE(check->Parse(nullptr));
+  EXPECT_NE(CSVPassword::Status::kOK, check->Parse(nullptr));
 
   for (const base::StringPiece& expected_password : kExpectedPasswords) {
     PasswordForm result = (iter++)->ParseValid();
@@ -107,12 +109,13 @@ TEST(CSVPasswordIteratorTest, Failure) {
 
   CSVPasswordIterator check = iter;
   for (size_t i = 0; i + 1 < kLinesInBlob; ++i) {
-    EXPECT_FALSE((check++)->Parse(nullptr)) << "on line " << i;
+    EXPECT_NE(CSVPassword::Status::kOK, (check++)->Parse(nullptr))
+        << "on line " << i;
   }
   // Last line was not a failure.
-  EXPECT_TRUE((check++)->Parse(nullptr));
+  EXPECT_EQ(CSVPassword::Status::kOK, (check++)->Parse(nullptr));
   // After iterating over all lines, there is no more data to parse.
-  EXPECT_FALSE(check->Parse(nullptr));
+  EXPECT_NE(CSVPassword::Status::kOK, check->Parse(nullptr));
 }
 
 }  // namespace password_manager

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

@@ -14,6 +14,8 @@ namespace password_manager {
 
 using ::autofill::PasswordForm;
 
+using Status = CSVPassword::Status;
+
 TEST(CSVPassword, Construction) {
   CSVPassword::ColumnMap col_map = {
       {0, CSVPassword::Label::kOrigin},
@@ -39,6 +41,7 @@ struct TestCase {
   std::string signon_realm;
   std::string username;
   std::string password;
+  Status status = Status::kOK;
 };
 
 class TestCaseBuilder {
@@ -77,6 +80,11 @@ class TestCaseBuilder {
     return *this;
   }
 
+  TestCaseBuilder& Status(Status status) {
+    test_case_.status = status;
+    return *this;
+  }
+
   TestCase Build() { return std::move(test_case_); }
 
  private:
@@ -89,7 +97,7 @@ TEST_P(CSVPasswordTestSuccess, Parse) {
   const TestCase& test_case = GetParam();
   SCOPED_TRACE(test_case.name);
   CSVPassword csv_pwd(test_case.map, test_case.csv);
-  EXPECT_TRUE(csv_pwd.Parse(nullptr));
+  EXPECT_EQ(Status::kOK, csv_pwd.Parse(nullptr));
 
   PasswordForm result = csv_pwd.ParseValid();
 
@@ -173,7 +181,8 @@ class CSVPasswordTestFailure : public ::testing::TestWithParam<TestCase> {};
 TEST_P(CSVPasswordTestFailure, Parse) {
   const TestCase& test_case = GetParam();
   SCOPED_TRACE(test_case.name);
-  EXPECT_FALSE(CSVPassword(test_case.map, test_case.csv).Parse(nullptr));
+  EXPECT_EQ(test_case.status,
+            CSVPassword(test_case.map, test_case.csv).Parse(nullptr));
 }
 
 INSTANTIATE_TEST_SUITE_P(
@@ -182,51 +191,60 @@ INSTANTIATE_TEST_SUITE_P(
     ::testing::Values(TestCaseBuilder("no columns specified")
                           .Map({})
                           .CSV("http://example.com,user,password")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("not ASCII")
                           .Map({{0, CSVPassword::Label::kOrigin},
                                 {1, CSVPassword::Label::kUsername},
                                 {2, CSVPassword::Label::kPassword}})
                           .CSV("http://example.com/ř,user,password")
+                          .Status(Status::kSyntaxError)
                           .Build(),
                       TestCaseBuilder("no origin in map")
                           .Map({{1, CSVPassword::Label::kUsername},
                                 {2, CSVPassword::Label::kPassword}})
                           .CSV("http://example.com,user,password")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("no username in map")
                           .Map({{0, CSVPassword::Label::kOrigin},
                                 {2, CSVPassword::Label::kPassword}})
                           .CSV("http://example.com,user,password")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("no password in map")
                           .Map({{0, CSVPassword::Label::kOrigin},
                                 {1, CSVPassword::Label::kUsername}})
                           .CSV("http://example.com,user,password")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("no origin in CSV")
                           .Map({{0, CSVPassword::Label::kUsername},
                                 {1, CSVPassword::Label::kPassword},
                                 {2, CSVPassword::Label::kOrigin}})
                           .CSV("user,password")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("no username in CSV")
                           .Map({{0, CSVPassword::Label::kOrigin},
                                 {1, CSVPassword::Label::kPassword},
                                 {2, CSVPassword::Label::kUsername}})
                           .CSV("http://example.com,password")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("no password in CSV")
                           .Map({{0, CSVPassword::Label::kOrigin},
                                 {1, CSVPassword::Label::kUsername},
                                 {2, CSVPassword::Label::kPassword}})
                           .CSV("http://example.com,user")
+                          .Status(Status::kSemanticError)
                           .Build(),
                       TestCaseBuilder("malformed CSV")
                           .Map({{0, CSVPassword::Label::kOrigin},
                                 {1, CSVPassword::Label::kUsername},
                                 {2, CSVPassword::Label::kPassword}})
                           .CSV("\"")
+                          .Status(Status::kSyntaxError)
                           .Build(),
                       TestCaseBuilder("map not injective")
                           .Map({{0, CSVPassword::Label::kOrigin},
@@ -234,6 +252,7 @@ INSTANTIATE_TEST_SUITE_P(
                                 {2, CSVPassword::Label::kPassword},
                                 {3, CSVPassword::Label::kUsername}})
                           .CSV("http://example.com,user,pwd,user2")
+                          .Status(Status::kSemanticError)
                           .Build()));
 
 }  // namespace password_manager