Commit f46ff482 authored by dvadym's avatar dvadym Committed by Commit bot

In some cases we encounter situation when in processing onSubmit of password...

In some cases we encounter situation when in processing onSubmit of password form we didn't receive information from password store yet. For example this can happen when JavaScript creates password form after submission and submit it (as for nytimes.com).

To process such situation this CL postpones checking if password form fetched from store until moment when we are going to decide if we should save it.

In order to process this correctly creating pending credentials in PasswordManager was moved to separate method and it is called only when both events happen - form submission and fetching from store finished.

Tests are not added yet

BUG=470322

Review URL: https://codereview.chromium.org/1050903002

Cr-Commit-Position: refs/heads/master@{#324400}
parent 96d29c0c
......@@ -92,6 +92,9 @@ PasswordFormManager::PasswordFormManager(
bool ssl_valid)
: best_matches_deleter_(&best_matches_),
observed_form_(CopyAndModifySSLValidity(observed_form, ssl_valid)),
provisionally_saved_form_(nullptr),
other_possible_username_action_(
PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES),
is_new_login_(true),
has_generated_password_(false),
password_manager_(password_manager),
......@@ -258,7 +261,7 @@ bool PasswordFormManager::HasGeneratedPassword() const {
}
bool PasswordFormManager::HasValidPasswordForm() const {
DCHECK_EQ(state_, POST_MATCHING_PHASE);
DCHECK(state_ == MATCHING_PHASE || state_ == POST_MATCHING_PHASE) << state_;
// Non-HTML password forms (primarily HTTP and FTP autentication)
// do not contain username_element and password_element values.
if (observed_form_.scheme != PasswordForm::SCHEME_HTML)
......@@ -270,120 +273,13 @@ bool PasswordFormManager::HasValidPasswordForm() const {
void PasswordFormManager::ProvisionallySave(
const PasswordForm& credentials,
OtherPossibleUsernamesAction action) {
DCHECK_EQ(state_, POST_MATCHING_PHASE);
DCHECK(state_ == MATCHING_PHASE || state_ == POST_MATCHING_PHASE) << state_;
DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials));
provisionally_saved_form_.reset(new PasswordForm(credentials));
other_possible_username_action_ = action;
base::string16 password_to_save(PasswordToSave(credentials));
// Make sure the important fields stay the same as the initially observed or
// autofilled ones, as they may have changed if the user experienced a login
// failure.
// Look for these credentials in the list containing auto-fill entries.
PasswordFormMap::const_iterator it =
best_matches_.find(credentials.username_value);
if (it != best_matches_.end()) {
// The user signed in with a login we autofilled.
pending_credentials_ = *it->second;
bool password_changed =
pending_credentials_.password_value != password_to_save;
if (IsPendingCredentialsPublicSuffixMatch()) {
// If the autofilled credentials were only a PSL match, store a copy with
// the current origin and signon realm. This ensures that on the next
// visit, a precise match is found.
is_new_login_ = true;
user_action_ = password_changed ? kUserActionChoosePslMatch
: kUserActionOverridePassword;
// Update credential to reflect that it has been used for submission.
// If this isn't updated, then password generation uploads are off for
// sites where PSL matching is required to fill the login form, as two
// PASSWORD votes are uploaded per saved password instead of one.
//
// TODO(gcasto): It would be nice if other state were shared such that if
// say a password was updated on one match it would update on all related
// passwords. This is a much larger change.
UpdateMetadataForUsage(pending_credentials_);
// Normally, the copy of the PSL matched credentials, adapted for the
// current domain, is saved automatically without asking the user, because
// the copy likely represents the same account, i.e., the one for which
// the user already agreed to store a password.
//
// However, if the user changes the suggested password, it might indicate
// that the autofilled credentials and |credentials| actually correspond
// to two different accounts (see http://crbug.com/385619). In that case
// the user should be asked again before saving the password. This is
// ensured by clearing |original_signon_realm| on |pending_credentials_|,
// which unmarks it as a PSL match.
//
// There is still the edge case when the autofilled credentials represent
// the same account as |credentials| but the stored password was out of
// date. In that case, the user just had to manually enter the new
// password, which is now in |credentials|. The best thing would be to
// save automatically, and also update the original credentials. However,
// we have no way to tell if this is the case. This will likely happen
// infrequently, and the inconvenience put on the user by asking them is
// not significant, so we are fine with asking here again.
if (password_changed) {
pending_credentials_.original_signon_realm.clear();
DCHECK(!IsPendingCredentialsPublicSuffixMatch());
}
} else { // Not a PSL match.
is_new_login_ = false;
if (password_changed)
user_action_ = kUserActionOverridePassword;
}
} else if (action == ALLOW_OTHER_POSSIBLE_USERNAMES &&
UpdatePendingCredentialsIfOtherPossibleUsername(
credentials.username_value)) {
// |pending_credentials_| is now set. Note we don't update
// |pending_credentials_.username_value| to |credentials.username_value|
// yet because we need to keep the original username to modify the stored
// credential.
selected_username_ = credentials.username_value;
is_new_login_ = false;
} else {
// User typed in a new, unknown username.
user_action_ = kUserActionOverrideUsernameAndPassword;
pending_credentials_ = observed_form_;
if (credentials.was_parsed_using_autofill_predictions)
pending_credentials_.username_element = credentials.username_element;
pending_credentials_.username_value = credentials.username_value;
pending_credentials_.other_possible_usernames =
credentials.other_possible_usernames;
// The password value will be filled in later, remove any garbage for now.
pending_credentials_.password_value.clear();
pending_credentials_.new_password_value.clear();
// If this was a sign-up or change password form, the names of the elements
// are likely different than those on a login form, so do not bother saving
// them. We will fill them with meaningful values in UpdateLogin() when the
// user goes onto a real login form for the first time.
if (!credentials.new_password_element.empty()) {
pending_credentials_.password_element.clear();
pending_credentials_.new_password_element.clear();
}
}
pending_credentials_.action = credentials.action;
// If the user selected credentials we autofilled from a PasswordForm
// that contained no action URL (IE6/7 imported passwords, for example),
// bless it with the action URL from the observed form. See bug 1107719.
if (pending_credentials_.action.is_empty())
pending_credentials_.action = observed_form_.action;
pending_credentials_.password_value = password_to_save;
pending_credentials_.preferred = credentials.preferred;
if (user_action_ == kUserActionOverridePassword &&
pending_credentials_.type == PasswordForm::TYPE_GENERATED &&
!has_generated_password_) {
LogPasswordGenerationSubmissionEvent(PASSWORD_OVERRIDDEN);
}
if (has_generated_password_)
pending_credentials_.type = PasswordForm::TYPE_GENERATED;
if (HasCompletedMatching())
CreatePendingCredentials();
}
void PasswordFormManager::Save() {
......@@ -623,6 +519,11 @@ void PasswordFormManager::OnGetPasswordStoreResults(
OnRequestDone(results.Pass());
state_ = POST_MATCHING_PHASE;
// If password store was slow and provisionally saved form is already here
// then create pending credentials (see http://crbug.com/470322).
if (provisionally_saved_form_)
CreatePendingCredentials();
if (manager_action_ != kManagerActionBlacklisted) {
for (auto const& driver : drivers_)
ProcessFrame(driver);
......@@ -869,6 +770,128 @@ bool PasswordFormManager::UploadPasswordForm(
return success;
}
void PasswordFormManager::CreatePendingCredentials() {
DCHECK(provisionally_saved_form_);
base::string16 password_to_save(PasswordToSave(*provisionally_saved_form_));
// Make sure the important fields stay the same as the initially observed or
// autofilled ones, as they may have changed if the user experienced a login
// failure.
// Look for these credentials in the list containing auto-fill entries.
PasswordFormMap::const_iterator it =
best_matches_.find(provisionally_saved_form_->username_value);
if (it != best_matches_.end()) {
// The user signed in with a login we autofilled.
pending_credentials_ = *it->second;
bool password_changed =
pending_credentials_.password_value != password_to_save;
if (IsPendingCredentialsPublicSuffixMatch()) {
// If the autofilled credentials were only a PSL match, store a copy with
// the current origin and signon realm. This ensures that on the next
// visit, a precise match is found.
is_new_login_ = true;
user_action_ = password_changed ? kUserActionChoosePslMatch
: kUserActionOverridePassword;
// Update credential to reflect that it has been used for submission.
// If this isn't updated, then password generation uploads are off for
// sites where PSL matching is required to fill the login form, as two
// PASSWORD votes are uploaded per saved password instead of one.
//
// TODO(gcasto): It would be nice if other state were shared such that if
// say a password was updated on one match it would update on all related
// passwords. This is a much larger change.
UpdateMetadataForUsage(pending_credentials_);
// Normally, the copy of the PSL matched credentials, adapted for the
// current domain, is saved automatically without asking the user, because
// the copy likely represents the same account, i.e., the one for which
// the user already agreed to store a password.
//
// However, if the user changes the suggested password, it might indicate
// that the autofilled credentials and |provisionally_saved_form_|
// actually correspond to two different accounts (see
// http://crbug.com/385619). In that case the user should be asked again
// before saving the password. This is ensured by clearing
// |original_signon_realm| on |pending_credentials_|, which unmarks it as
// a PSL match.
//
// There is still the edge case when the autofilled credentials represent
// the same account as |provisionally_saved_form_| but the stored password
// was out of date. In that case, the user just had to manually enter the
// new password, which is now in |provisionally_saved_form_|. The best
// thing would be to save automatically, and also update the original
// credentials. However, we have no way to tell if this is the case.
// This will likely happen infrequently, and the inconvenience put on the
// user by asking them is not significant, so we are fine with asking
// here again.
if (password_changed) {
pending_credentials_.original_signon_realm.clear();
DCHECK(!IsPendingCredentialsPublicSuffixMatch());
}
} else { // Not a PSL match.
is_new_login_ = false;
if (password_changed)
user_action_ = kUserActionOverridePassword;
}
} else if (other_possible_username_action_ ==
ALLOW_OTHER_POSSIBLE_USERNAMES &&
UpdatePendingCredentialsIfOtherPossibleUsername(
provisionally_saved_form_->username_value)) {
// |pending_credentials_| is now set. Note we don't update
// |pending_credentials_.username_value| to |credentials.username_value|
// yet because we need to keep the original username to modify the stored
// credential.
selected_username_ = provisionally_saved_form_->username_value;
is_new_login_ = false;
} else {
// User typed in a new, unknown username.
user_action_ = kUserActionOverrideUsernameAndPassword;
pending_credentials_ = observed_form_;
if (provisionally_saved_form_->was_parsed_using_autofill_predictions)
pending_credentials_.username_element =
provisionally_saved_form_->username_element;
pending_credentials_.username_value =
provisionally_saved_form_->username_value;
pending_credentials_.other_possible_usernames =
provisionally_saved_form_->other_possible_usernames;
// The password value will be filled in later, remove any garbage for now.
pending_credentials_.password_value.clear();
pending_credentials_.new_password_value.clear();
// If this was a sign-up or change password form, the names of the elements
// are likely different than those on a login form, so do not bother saving
// them. We will fill them with meaningful values in UpdateLogin() when the
// user goes onto a real login form for the first time.
if (!provisionally_saved_form_->new_password_element.empty()) {
pending_credentials_.password_element.clear();
pending_credentials_.new_password_element.clear();
}
}
pending_credentials_.action = provisionally_saved_form_->action;
// If the user selected credentials we autofilled from a PasswordForm
// that contained no action URL (IE6/7 imported passwords, for example),
// bless it with the action URL from the observed form. See bug 1107719.
if (pending_credentials_.action.is_empty())
pending_credentials_.action = observed_form_.action;
pending_credentials_.password_value = password_to_save;
pending_credentials_.preferred = provisionally_saved_form_->preferred;
if (user_action_ == kUserActionOverridePassword &&
pending_credentials_.type == PasswordForm::TYPE_GENERATED &&
!has_generated_password_) {
LogPasswordGenerationSubmissionEvent(PASSWORD_OVERRIDDEN);
}
if (has_generated_password_)
pending_credentials_.type = PasswordForm::TYPE_GENERATED;
provisionally_saved_form_.reset();
}
int PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
DCHECK_EQ(state_, MATCHING_PHASE);
// For scoring of candidate login data:
......
......@@ -200,7 +200,6 @@ class PasswordFormManager : public PasswordStoreConsumer {
}
#endif
protected:
const autofill::PasswordForm& observed_form() const { return observed_form_; }
private:
......@@ -326,6 +325,10 @@ class PasswordFormManager : public PasswordStoreConsumer {
bool UploadPasswordForm(const autofill::FormData& form_data,
const autofill::ServerFieldType& password_type);
// Create pending credentials from provisionally saved form and forms received
// from password store.
void CreatePendingCredentials();
// Set of PasswordForms from the DB that best match the form
// being managed by this. Use a map instead of vector, because we most
// frequently require lookups by username value in IsNewLogin.
......@@ -338,6 +341,12 @@ class PasswordFormManager : public PasswordStoreConsumer {
// The PasswordForm from the page or dialog managed by |this|.
const autofill::PasswordForm observed_form_;
// Stores provisionally saved form until |pending_credentials_| is created.
scoped_ptr<const autofill::PasswordForm> provisionally_saved_form_;
// Stores if for creating |pending_credentials_| other possible usernames
// option should apply.
OtherPossibleUsernamesAction other_possible_username_action_;
// The origin url path of observed_form_ tokenized, for convenience when
// scoring.
std::vector<std::string> form_path_tokens_;
......
......@@ -178,7 +178,6 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
// Below, "matching" is in DoesManage-sense and "not ready" in
// !HasCompletedMatching sense. We keep track of such PasswordFormManager
// instances for UMA.
bool has_found_matching_managers_which_were_not_ready = false;
for (ScopedVector<PasswordFormManager>::iterator iter =
pending_login_managers_.begin();
iter != pending_login_managers_.end(); ++iter) {
......@@ -195,11 +194,6 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
continue;
}
if (!(*iter)->HasCompletedMatching()) {
has_found_matching_managers_which_were_not_ready = true;
continue;
}
if (result == PasswordFormManager::RESULT_COMPLETE_MATCH) {
// If we find a manager that exactly matches the submitted form including
// the action URL, exit the loop.
......@@ -241,25 +235,11 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
// |manager|.
manager.reset(*matched_manager_it);
pending_login_managers_.weak_erase(matched_manager_it);
} else if (has_found_matching_managers_which_were_not_ready) {
// We found some managers, but none finished matching yet. The user has
// tried to submit credentials before we had time to even find matching
// results for the given form and autofill. If this is the case, we just
// give up.
RecordFailure(MATCHING_NOT_COMPLETE, form.origin, logger.get());
return;
} else {
RecordFailure(NO_MATCHING_FORM, form.origin, logger.get());
return;
}
// Also get out of here if the user told us to 'never remember' passwords for
// this form.
if (manager->IsBlacklisted()) {
RecordFailure(FORM_BLACKLISTED, form.origin, logger.get());
return;
}
// Bail if we're missing any of the necessary form components.
if (!manager->HasValidPasswordForm()) {
RecordFailure(INVALID_FORM, form.origin, logger.get());
......@@ -467,6 +447,26 @@ void PasswordManager::OnPasswordFormsRendered(
return;
}
if (!provisional_save_manager_->HasCompletedMatching()) {
// We have a provisional save manager, but it didn't finish matching yet.
// We just give up.
RecordFailure(MATCHING_NOT_COMPLETE,
provisional_save_manager_->observed_form().origin,
logger.get());
provisional_save_manager_.reset();
return;
}
// Also get out of here if the user told us to 'never remember' passwords for
// this form.
if (provisional_save_manager_->IsBlacklisted()) {
RecordFailure(FORM_BLACKLISTED,
provisional_save_manager_->observed_form().origin,
logger.get());
provisional_save_manager_.reset();
return;
}
DCHECK(client_->IsSavingEnabledForCurrentPage());
// If the server throws an internal error, access denied page, page not
......
......@@ -25,6 +25,7 @@ using base::ASCIIToUTF16;
using testing::_;
using testing::AnyNumber;
using testing::Exactly;
using testing::Invoke;
using testing::Return;
using testing::WithArg;
......@@ -1028,4 +1029,52 @@ TEST_F(PasswordManagerTest, FormSubmittedChangedWithAutofillResponse) {
form_to_save->Save();
}
TEST_F(PasswordManagerTest, SubmitNotFetchedFromStoreForm) {
// Test that observing a newly submitted form that is fetched after on submit
// shows the save password bar.
EXPECT_CALL(driver_, FillPasswordForm(_)).Times(Exactly(0));
std::vector<PasswordForm> observed;
PasswordForm form(MakeSimpleForm());
observed.push_back(form);
PasswordStoreConsumer* form_manager = nullptr;
// Do not call back from store after GetLogins is called. Instead, save the
// pointer to the form manager for calling back later. This emulates that
// PasswordStore does not manage to fetch a form till moment of submission.
ON_CALL(*store_, GetLogins(_, _, _))
.WillByDefault(testing::SaveArg<2>(&form_manager));
// The initial load.
manager()->OnPasswordFormsParsed(&driver_, observed);
// The initial layout.
manager()->OnPasswordFormsRendered(&driver_, observed, true);
ASSERT_TRUE(form_manager);
// And the form submit contract is to call ProvisionallySavePassword.
manager()->ProvisionallySavePassword(form);
// Emulate fetching password form from PasswordStore after submission but
// before post-navigation load.
form_manager->OnGetPasswordStoreResults(
ScopedVector<autofill::PasswordForm>());
scoped_ptr<PasswordFormManager> form_to_save;
EXPECT_CALL(client_,
PromptUserToSavePasswordPtr(
_, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
// Now the password manager waits for the navigation to complete.
observed.clear();
// The post-navigation load.
manager()->OnPasswordFormsParsed(&driver_, observed);
// The post-navigation layout.
manager()->OnPasswordFormsRendered(&driver_, observed, true);
ASSERT_TRUE(form_to_save);
EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
// Simulate saving the form, as if the info bar was accepted.
form_to_save->Save();
}
} // namespace password_manager
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment