[Passwords] Require matching username for UsernameFirstFlow

This change modifies `IsPossibleUsernameValid` to require that the
possible username is part of an already saved credential for the same
site. This will be later extended to only require a match with a local
username saved on any site. For robustness the username and the set of
allowed usernames are canonicalized, that is case and email providers
are irrelevant.

The existing requirements to only be ASCII and to not contain whitespace
are dropped.

Bug: 959776, 1167042
Change-Id: Ic50c954ddd7a4fd969c56295825ae717c870b915
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2627278
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Maria Kazinova <kazinova@google.com>
Cr-Commit-Position: refs/heads/master@{#844013}

components/password_manager/core/browser/password_form_manager.cc

@@ -1054,9 +1054,17 @@ bool PasswordFormManager::UsePossibleUsername(
     }
   }
 
+  // TODO(crbug.com/959776): This currently only considers a possible username
+  // valid if a credential with the same username already exists for the same
+  // site. This is too conservative, and we should allow any possible username
+  // that matches a credential on any site in the user's password store.
+  std::vector<base::string16> usernames;
+  usernames.reserve(GetBestMatches().size());
+  base::ranges::transform(GetBestMatches(), std::back_inserter(usernames),
+                          &PasswordForm::username_value);
+
   bool is_possible_username_valid = IsPossibleUsernameValid(
-      *possible_username, parsed_submitted_form_->signon_realm,
-      base::Time::Now());
+      *possible_username, parsed_submitted_form_->signon_realm, usernames);
   LogUsingPossibleUsername(client_, /*is_used*/ is_possible_username_valid,
                            "Local heuristics");
   return is_possible_username_valid;

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -2098,8 +2098,8 @@ TEST_P(PasswordFormManagerTest, UsernameFirstFlow) {
   feature_list.InitAndEnableFeature(features::kUsernameFirstFlow);
 
   CreateFormManager(observed_form_only_password_fields_);
-  fetcher_->NotifyFetchCompleted();
-  const base::string16 possible_username = ASCIIToUTF16("possible_username");
+  SetNonFederatedAndNotifyFetchCompleted({&saved_match_});
+  const base::string16 possible_username = ASCIIToUTF16("test@example.com");
   PossibleUsernameData possible_username_data(
       saved_match_.signon_realm, autofill::FieldRendererId(1),
       possible_username, base::Time::Now(), 0 /* driver_id */);
@@ -2117,7 +2117,8 @@ TEST_P(PasswordFormManagerTest, UsernameFirstFlow) {
 #else
   // Local heuristics on Android for username first flow are not supported, so
   // the username should not be taken from the username form.
-  EXPECT_TRUE(form_manager_->GetPendingCredentials().username_value.empty());
+  EXPECT_EQ(saved_match_.username_value,
+            form_manager_->GetPendingCredentials().username_value);
 #endif  // !defined(OS_ANDROID)
 }
 
@@ -2808,8 +2809,8 @@ TEST_F(PasswordFormManagerTestWithMockedSaver, UsernameFirstFlow) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(features::kUsernameFirstFlow);
   CreateFormManager(observed_form_only_password_fields_);
-  fetcher_->NotifyFetchCompleted();
-  const base::string16 possible_username = ASCIIToUTF16("possible_username");
+  SetNonFederatedAndNotifyFetchCompleted({&saved_match_});
+  const base::string16 possible_username = ASCIIToUTF16("test@example.org");
   PossibleUsernameData possible_username_data(
       saved_match_.signon_realm, autofill::FieldRendererId(1u),
       possible_username, base::Time::Now(), 0 /* driver_id */);

components/password_manager/core/browser/password_manager_unittest.cc

@@ -74,8 +74,10 @@ using base::ASCIIToUTF16;
 using base::Feature;
 using base::TestMockTimeTaskRunner;
 using testing::_;
+using testing::AllOf;
 using testing::AnyNumber;
 using testing::ByMove;
+using testing::Field;
 using testing::Invoke;
 using testing::IsNull;
 using testing::Mock;
@@ -3410,11 +3412,12 @@ TEST_P(PasswordManagerTest, UsernameFirstFlow) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(features::kUsernameFirstFlow);
   EXPECT_CALL(*store_, GetLogins(_, _))
-      .WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms(store_.get())));
+      .WillRepeatedly(
+          WithArg<1>(InvokeConsumer(store_.get(), MakeSavedForm())));
 
   PasswordForm form(MakeSimpleFormWithOnlyPasswordField());
   // Simulate the user typed a username in username form.
-  const base::string16 username = ASCIIToUTF16("username1");
+  const base::string16 username = ASCIIToUTF16("googleuser@gmail.com");
   EXPECT_CALL(driver_, GetLastCommittedURL()).WillOnce(ReturnRef(form.url));
   manager()->OnUserModifiedNonPasswordField(&driver_, FieldRendererId(1001),
                                             username /* value */);
@@ -3438,21 +3441,24 @@ TEST_P(PasswordManagerTest, UsernameFirstFlow) {
   // Simulates successful submission.
   manager()->OnPasswordFormsRendered(&driver_, {} /* observed */, true);
 
-  // Simulate saving the form, as if the info bar was accepted.
-  PasswordForm saved_form;
-  EXPECT_CALL(*store_, AddLogin(_)).WillOnce(SaveArg<0>(&saved_form));
   ASSERT_TRUE(form_manager_to_save);
-  form_manager_to_save->Save();
-
-#if defined(OS_ANDROID)
-  // Local heuristics on Android for username first flow are not supported, so
-  // the username should not be taken from the username form.
-  EXPECT_TRUE(saved_form.username_value.empty());
+#if !defined(OS_ANDROID)
+  EXPECT_CALL(*store_,
+              AddLogin(AllOf(Field(&PasswordForm::username_value, username),
+                             Field(&PasswordForm::password_value, password))));
 #else
-  EXPECT_EQ(username, saved_form.username_value);
+  // Local heuristics on Android for username first flow are not supported, so
+  // the username should not be taken from the username form. Furthermore, since
+  // there is no change to the already saved username, this should trigger an
+  // update flow, rather than a save flow.
+  EXPECT_CALL(
+      *store_,
+      UpdateLogin(AllOf(
+          Field(&PasswordForm::username_value, MakeSavedForm().username_value),
+          Field(&PasswordForm::password_value, password))));
 #endif  // defined(OS_ANDROID)
-
-  EXPECT_EQ(password, saved_form.password_value);
+  // Simulate saving the form, as if the info bar was accepted.
+  form_manager_to_save->Save();
 }
 
 #if !defined(OS_IOS)

components/password_manager/core/browser/possible_username_data.cc

@@ -4,7 +4,12 @@
 
 #include "components/password_manager/core/browser/possible_username_data.h"
 
-#include "base/strings/string_util.h"
+#include <vector>
+
+#include "base/containers/contains.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_piece.h"
+#include "components/password_manager/core/browser/leak_detection/encryption_utils.h"
 
 using base::char16;
 using base::TimeDelta;
@@ -26,29 +31,25 @@ PossibleUsernameData::PossibleUsernameData(const PossibleUsernameData&) =
     default;
 PossibleUsernameData::~PossibleUsernameData() = default;
 
-bool IsPossibleUsernameValid(const PossibleUsernameData& possible_username,
-                             const std::string& submitted_signon_realm,
-                             const base::Time now) {
+bool IsPossibleUsernameValid(
+    const PossibleUsernameData& possible_username,
+    const std::string& submitted_signon_realm,
+    const std::vector<base::string16>& possible_usernames) {
   if (submitted_signon_realm != possible_username.signon_realm)
     return false;
+
   // The goal is to avoid false positives in considering which strings might be
   // username. In the initial version of the username first flow it is better to
-  // be conservative in that.
-  // TODO(https://crbug.com/959776): Reconsider allowing non-ascii symbols in
-  // username for the username first flow.
-  if (!base::IsStringASCII(possible_username.value))
-    return false;
-  for (char16 c : possible_username.value) {
-    if (base::IsUnicodeWhitespace(c))
-      return false;
-  }
-
-  if (now - possible_username.last_change >
-      kMaxDelayBetweenTypingUsernameAndSubmission) {
+  // be conservative in that. This check only allows usernames that match
+  // existing usernames after canonicalization.
+  base::string16 (*Canonicalize)(base::StringPiece16) = &CanonicalizeUsername;
+  if (!base::Contains(possible_usernames, Canonicalize(possible_username.value),
+                      Canonicalize)) {
     return false;
   }
 
-  return true;
+  return base::Time::Now() - possible_username.last_change <=
+         kMaxDelayBetweenTypingUsernameAndSubmission;
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/possible_username_data.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_POSSIBLE_USERNAME_DATA_H_
 
 #include <string>
+#include <vector>
 
 #include "base/optional.h"
 #include "base/strings/string16.h"
@@ -18,8 +19,8 @@ namespace password_manager {
 // The maximum time between the user typed in a text field and subsequent
 // submission of the password form, such that the typed value is considered to
 // be possible to be username.
-constexpr base::TimeDelta kMaxDelayBetweenTypingUsernameAndSubmission =
-    base::TimeDelta::FromSeconds(60);
+constexpr auto kMaxDelayBetweenTypingUsernameAndSubmission =
+    base::TimeDelta::FromMinutes(1);
 
 // Contains information that the user typed in a text field. It might be the
 // username during username first flow.
@@ -47,12 +48,14 @@ struct PossibleUsernameData {
 
 // Checks that |possible_username| might represent an username:
 // 1.|possible_username.signon_realm| == |submitted_signon_realm|
-// 2.|possible_username.value| does not have whitespaces and non-ASCII symbols.
-// 3.|possible_username.value.last_change| was not more than 60 seconds before
-// now.
-bool IsPossibleUsernameValid(const PossibleUsernameData& possible_username,
-                             const std::string& submitted_signon_realm,
-                             const base::Time now);
+// 2.|possible_username.value| is contained in |possible_usernames| after
+//   canonicalization.
+// 3.|possible_username.value.last_change| was not more than
+//   |kMaxDelayBetweenTypingUsernameAndSubmission| ago.
+bool IsPossibleUsernameValid(
+    const PossibleUsernameData& possible_username,
+    const std::string& submitted_signon_realm,
+    const std::vector<base::string16>& possible_usernames);
 
 }  // namespace password_manager
 

components/password_manager/core/browser/possible_username_data_unittest.cc

@@ -5,10 +5,10 @@
 #include "components/password_manager/core/browser/possible_username_data.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/task_environment.h"
 #include "components/autofill/core/common/renderer_id.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using base::ASCIIToUTF16;
 using base::Time;
 using base::TimeDelta;
 
@@ -16,71 +16,65 @@ namespace password_manager {
 
 namespace {
 
-class IsPossibleUsernameValidTest : public testing::Test {
- public:
-  IsPossibleUsernameValidTest()
-      : possible_username_data_(
-            "https://example.com/" /* submitted_signon_realm */,
-            autofill::FieldRendererId(1u),
-            ASCIIToUTF16("username") /* value */,
-            base::Time::Now() /* last_change */,
-            10 /* driver_id */) {}
+constexpr base::char16 kUser[] = STRING16_LITERAL("user");
 
+class IsPossibleUsernameValidTest : public testing::Test {
  protected:
-  PossibleUsernameData possible_username_data_;
+  base::test::SingleThreadTaskEnvironment task_environment_{
+      base::test::SingleThreadTaskEnvironment::TimeSource::MOCK_TIME};
+  PossibleUsernameData possible_username_data_{
+      "https://example.com/" /* submitted_signon_realm */,
+      autofill::FieldRendererId(1u), kUser /* value */,
+      base::Time::Now() /* last_change */, 10 /* driver_id */};
 };
 
 TEST_F(IsPossibleUsernameValidTest, Valid) {
-  EXPECT_TRUE(IsPossibleUsernameValid(possible_username_data_,
-                                      possible_username_data_.signon_realm,
-                                      possible_username_data_.last_change));
+  EXPECT_TRUE(IsPossibleUsernameValid(
+      possible_username_data_, possible_username_data_.signon_realm, {kUser}));
 }
 
 // Check that if time delta between last change and submission is more than 60
 // seconds, than data is invalid.
 TEST_F(IsPossibleUsernameValidTest, TimeDeltaBeforeLastChangeAndSubmission) {
-  Time valid_submission_time = possible_username_data_.last_change +
-                               kMaxDelayBetweenTypingUsernameAndSubmission;
-  Time invalid_submission_time =
-      valid_submission_time + TimeDelta::FromSeconds(1);
-  EXPECT_TRUE(IsPossibleUsernameValid(possible_username_data_,
-                                      possible_username_data_.signon_realm,
-                                      valid_submission_time));
-  EXPECT_FALSE(IsPossibleUsernameValid(possible_username_data_,
-                                       possible_username_data_.signon_realm,
-                                       invalid_submission_time));
+  task_environment_.FastForwardBy(kMaxDelayBetweenTypingUsernameAndSubmission);
+  EXPECT_TRUE(IsPossibleUsernameValid(
+      possible_username_data_, possible_username_data_.signon_realm, {kUser}));
+  task_environment_.FastForwardBy(TimeDelta::FromSeconds(1));
+  EXPECT_FALSE(IsPossibleUsernameValid(
+      possible_username_data_, possible_username_data_.signon_realm, {kUser}));
 }
 
 TEST_F(IsPossibleUsernameValidTest, SignonRealm) {
   EXPECT_FALSE(IsPossibleUsernameValid(possible_username_data_,
-                                       "https://m.example.com/",
-                                       possible_username_data_.last_change));
+                                       "https://m.example.com/", {kUser}));
 
   EXPECT_FALSE(IsPossibleUsernameValid(possible_username_data_,
-                                       "https://google.com/",
-                                       possible_username_data_.last_change));
+                                       "https://google.com/", {kUser}));
 }
 
 TEST_F(IsPossibleUsernameValidTest, PossibleUsernameValue) {
-  PossibleUsernameData possible_username_data = possible_username_data_;
-
-  // White spaces are not allowed in username.
-  possible_username_data.value = ASCIIToUTF16("user name");
-  EXPECT_FALSE(IsPossibleUsernameValid(possible_username_data,
-                                       possible_username_data.signon_realm,
-                                       possible_username_data.last_change));
-
-  // New lines are not allowed in username.
-  possible_username_data.value = ASCIIToUTF16("user\nname");
-  EXPECT_FALSE(IsPossibleUsernameValid(possible_username_data,
-                                       possible_username_data.signon_realm,
-                                       possible_username_data.last_change));
-
-  // Digits and special characters are ok.
-  possible_username_data.value = ASCIIToUTF16("User_name1234!+&%#\'\"@");
-  EXPECT_TRUE(IsPossibleUsernameValid(possible_username_data,
-                                      possible_username_data.signon_realm,
-                                      possible_username_data.last_change));
+  // Different capitalization is okay.
+  EXPECT_TRUE(IsPossibleUsernameValid(possible_username_data_,
+                                      possible_username_data_.signon_realm,
+                                      {STRING16_LITERAL("USER")}));
+  // Different email hosts are okay.
+  EXPECT_TRUE(IsPossibleUsernameValid(possible_username_data_,
+                                      possible_username_data_.signon_realm,
+                                      {STRING16_LITERAL("user@gmail.com")}));
+
+  // Other usernames are okay.
+  EXPECT_TRUE(IsPossibleUsernameValid(possible_username_data_,
+                                      possible_username_data_.signon_realm,
+                                      {kUser, STRING16_LITERAL("alice")}));
+
+  // No usernames are not okay.
+  EXPECT_FALSE(IsPossibleUsernameValid(
+      possible_username_data_, possible_username_data_.signon_realm, {}));
+
+  // Completely different usernames are not okay.
+  EXPECT_FALSE(IsPossibleUsernameValid(
+      possible_username_data_, possible_username_data_.signon_realm,
+      {STRING16_LITERAL("alice"), STRING16_LITERAL("bob")}));
 }
 
 }  // namespace