Skip to content

Commit

Permalink
[Passwords] Require matching username for UsernameFirstFlow
Browse files Browse the repository at this point in the history 
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}
  • Loading branch information
@jdoerrie
jdoerrie authored and Chromium LUCI CQ committed Jan 15, 2021
1 parent 48bc863 commit b6b21a5
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 93 deletions.
12 changes: 10 additions & 2 deletions components/password_manager/core/browser/password_form_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 6 additions & 5 deletions components/password_manager/core/browser/password_form_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */);
Expand All @@ -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)
}

Expand Down Expand Up @@ -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 */);
Expand Down
34 changes: 20 additions & 14 deletions components/password_manager/core/browser/password_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 */);
Expand All @@ -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)
Expand Down
35 changes: 18 additions & 17 deletions components/password_manager/core/browser/possible_username_data.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
19 changes: 11 additions & 8 deletions components/password_manager/core/browser/possible_username_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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

Expand Down
88 changes: 41 additions & 47 deletions components/password_manager/core/browser/possible_username_data_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,82 +5,76 @@
#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;

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
Expand Down

0 comments on commit b6b21a5

Please sign in to comment.