Skip to content

GitLab

T
tangled
Commit 2d89d824 authored by vabr@chromium.org's avatar vabr@chromium.org
Browse files
Options

Only consider PasswordFromManager which HasCompletedMatching when provisionally saving passwords 

In PasswordManager::ProvisionallySavePassword, PasswordFormManager instances are sought, which could be managing the form being saved. Some of those managers may not be ready, because they still did not complete matching against the PasswordStore. There might also be more form managers suitable for the form being saved, with some having completed the matching, and some not.

Currently, the code first finds the first suitable form manager, and then drops it if it has not completed matching. This ignores the fact that there might be a second choice form manager, which has completed matching. This happens currently on live.com (see the bug).

This CL ignores form managers which did not complete matching when looking for a suitable form manager for the form being saved.

This CL also improves a poorly phrased log message related to the situation when form managers have not completed matching.

BUG=367768

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282935 0039d316-1c4b-4281-b951-d872f2087c98
parent bd49c237
Hide whitespace changes
Inline Side-by-side
Showing with 156 additions and 52 deletions
chrome/browser/password_manager/password_manager_browsertest.cc
View file @ 2d89d824
...@@ -1099,3 +1099,27 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, NoLastLoadGoodLastLoad) { ...@@ -1099,3 +1099,27 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, NoLastLoadGoodLastLoad) {
run_loop.RunUntilIdle(); run_loop.RunUntilIdle();
EXPECT_FALSE(password_store->IsEmpty()); EXPECT_FALSE(password_store->IsEmpty());
} }
// In some situations, multiple PasswordFormManager instances from
// PasswordManager::pending_login_managers_ would match (via DoesManage) a form
// to be provisionally saved. One of them might be a complete match, the other
// all-but-action match. Normally, the former should be preferred, but if the
// former has not finished matching, and the latter has, the latter should be
// used (otherwise we'd give up even though we could have saved the password).
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
PreferPasswordFormManagerWhichFinishedMatching) {
NavigateToFile("/password/create_form_copy_on_submit.html");
NavigationObserver observer(WebContents());
scoped_ptr<PromptObserver> prompt_observer(
PromptObserver::Create(WebContents()));
std::string submit =
"document.getElementById('username').value = 'overwrite_me';"
"document.getElementById('password').value = 'random';"
"document.getElementById('non-form-button').click();";
ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), submit));
observer.Wait();
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
}
chrome/test/data/password/between_parsing_and_rendering.html
View file @ 2d89d824
...@@ -23,47 +23,10 @@ some webkit-only rules for background properties. ...@@ -23,47 +23,10 @@ some webkit-only rules for background properties.
--> -->
<html> <html>
<head> <head>
<script src="form_utils.js"></script>
<script> <script>
function createForm() {
var form = document.createElement('form');
form.setAttribute('action', 'done.html');
var username_label = document.createElement('label');
username_label.setAttribute('for', 'username');
username_label.innerText = 'Username: ';
var username = document.createElement('input');
username.type = 'text';
username.name = 'username';
username.id = 'username';
var password_label = document.createElement('label');
password_label.innerText = 'Password: ';
password_label.setAttribute('for', 'password');
var password = document.createElement('input');
password.type = 'password';
password.name = 'password';
password.id = 'password';
var submit = document.createElement('input');
submit.type = 'submit';
submit.id = 'submit-button';
submit.value = 'Submit';
form.appendChild(username_label);
form.appendChild(username);
form.appendChild(password_label);
form.appendChild(password);
form.appendChild(submit);
return form;
}
function addForm(form) {
document.getElementsByTagName('body')[0].appendChild(form);
}
function onLoadHandler() { function onLoadHandler() {
addForm(createForm()); document.body.appendChild(createSimplePasswordForm());
} }
</script> </script>
<style> <style>
......
chrome/test/data/password/create_form_copy_on_submit.html 0 → 100644
View file @ 2d89d824
<!--
This page enables to simulate the following scenario:
On password form submit, the page's JavaScript deletes that form, creates
another one, almost an exact copy of the deleted form, just with a different
action, and submits the new one.
The issue demonstrated here is that there is very little time between
creating and submitting the second form. As observed in
http://crbug.com/367768, PasswordManager is not able to process the form
quickly enough in such cases: the PasswordFormManager associated with the
created form has not enough time to asynchronously get matching results from
the password store, and is hence not ready to provisionally save the new
credential. This test checks that PasswordManager still makes use of the
PasswordFormManager associated with the first form (which is a reasonable
match for the credential, though worse than the newer PasswordFormManager)
and works in this scenario.
-->
<html>
<head>
<script src="form_utils.js"></script>
<script>
function preCreatePasswordForm() {
// Remember the filled in password + destroy the old form.
var old_password = document.getElementById('password').value;
document.getElementById('contains-form').innerText = '';
// Spin the message loop: it's not clear spinning it is needed, but
// let's make sure the deletion side effects, if any, have time to
// propagate and don't cause flakes.
window.setTimeout(createPasswordForm, 0, old_password);
}
function createPasswordForm(old_password) {
// Create and append the new password form. It is almost the
// same as the deleted one, only with a different action.
document.body.appendChild(createSimplePasswordForm());
// Spin the message loop again, to let the creation settle in.
window.setTimeout(postCreatePasswordForm, 0, old_password);
}
function postCreatePasswordForm(old_password) {
// Copy over the old password + add a dummy username, and submit
// the new form.
document.getElementById('username').value = 'test';
document.getElementById('password').value = old_password;
document.getElementById('submit-button').click();
}
</script>
<title>Test dynamically created password form</title>
</head>
<body>
<div id="contains-form">
<form action="none.html">
Old Form (to visually distinguish it from the form it is replaced with):
<label for="username">Username</label>
<input type="text" id="username" name="username">
<label for="password">Password</label>
<input type="password" id="password" name="password">
<input type="submit" id="submit-button" value="Don't click!">
</form>
<button id="non-form-button" onclick="preCreatePasswordForm();">
Click!
</button>
</div>
</body>
</html>
chrome/test/data/password/form_utils.js 0 → 100644
View file @ 2d89d824
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Creates the following form:
// <form action="done.html">
// <label for="username">Username</label>
// <input type="text" id="username" name="username">
// <label for="password">Password</label>
// <input type="password" id="password" name="password">
// <input type="submit" id="submit-button">
// </form>
function createSimplePasswordForm() {
var form = document.createElement('form');
form.setAttribute('action', 'done.html');
var username_label = document.createElement('label');
username_label.htmlFor = 'username';
username_label.innerText = 'Username: ';
var username = document.createElement('input');
username.type = 'text';
username.name = 'username';
username.id = 'username';
var password_label = document.createElement('label');
password_label.innerText = 'Password: ';
password_label.htmlFor = 'password';
var password = document.createElement('input');
password.type = 'password';
password.name = 'password';
password.id = 'password';
var submit = document.createElement('input');
submit.type = 'submit';
submit.id = 'submit-button';
submit.value = 'Submit';
form.appendChild(username_label);
form.appendChild(username);
form.appendChild(password_label);
form.appendChild(password);
form.appendChild(submit);
return form;
}
components/autofill/core/common/save_password_progress_logger.cc
View file @ 2d89d824
...@@ -137,8 +137,8 @@ std::string GetStringFromID(SavePasswordProgressLogger::StringID id) { ...@@ -137,8 +137,8 @@ std::string GetStringFromID(SavePasswordProgressLogger::StringID id) {
return "Form manager found, exact match."; return "Form manager found, exact match.";
case SavePasswordProgressLogger::STRING_MATCH_WITHOUT_ACTION: case SavePasswordProgressLogger::STRING_MATCH_WITHOUT_ACTION:
return "Form manager found, match except for action."; return "Form manager found, match except for action.";
case SavePasswordProgressLogger::STRING_NO_FORM_MANAGER: case SavePasswordProgressLogger::STRING_MATCHING_NOT_COMPLETE:
return "No form manager found."; return "No form manager has completed matching.";
case SavePasswordProgressLogger::STRING_FORM_BLACKLISTED: case SavePasswordProgressLogger::STRING_FORM_BLACKLISTED:
return "Form blacklisted."; return "Form blacklisted.";
case SavePasswordProgressLogger::STRING_INVALID_FORM: case SavePasswordProgressLogger::STRING_INVALID_FORM:
......
components/autofill/core/common/save_password_progress_logger.h
View file @ 2d89d824
...@@ -87,7 +87,7 @@ class SavePasswordProgressLogger { ...@@ -87,7 +87,7 @@ class SavePasswordProgressLogger {
STRING_EMPTY_PASSWORD, STRING_EMPTY_PASSWORD,
STRING_EXACT_MATCH, STRING_EXACT_MATCH,
STRING_MATCH_WITHOUT_ACTION, STRING_MATCH_WITHOUT_ACTION,
STRING_NO_FORM_MANAGER, STRING_MATCHING_NOT_COMPLETE,
STRING_FORM_BLACKLISTED, STRING_FORM_BLACKLISTED,
STRING_INVALID_FORM, STRING_INVALID_FORM,
STRING_AUTOCOMPLETE_OFF, STRING_AUTOCOMPLETE_OFF,
......
components/password_manager/core/browser/password_manager.cc
View file @ 2d89d824
...@@ -149,11 +149,22 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) { ...@@ -149,11 +149,22 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
scoped_ptr<PasswordFormManager> manager; scoped_ptr<PasswordFormManager> manager;
ScopedVector<PasswordFormManager>::iterator matched_manager_it = ScopedVector<PasswordFormManager>::iterator matched_manager_it =
pending_login_managers_.end(); pending_login_managers_.end();
// 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 = for (ScopedVector<PasswordFormManager>::iterator iter =
pending_login_managers_.begin(); pending_login_managers_.begin();
iter != pending_login_managers_.end(); iter != pending_login_managers_.end();
++iter) { ++iter) {
PasswordFormManager::MatchResultMask result = (*iter)->DoesManage(form); PasswordFormManager::MatchResultMask result = (*iter)->DoesManage(form);
if (!(*iter)->HasCompletedMatching()) {
if (result != PasswordFormManager::RESULT_NO_MATCH)
has_found_matching_managers_which_were_not_ready = true;
continue;
}
if (result == PasswordFormManager::RESULT_COMPLETE_MATCH) { if (result == PasswordFormManager::RESULT_COMPLETE_MATCH) {
// If we find a manager that exactly matches the submitted form including // If we find a manager that exactly matches the submitted form including
// the action URL, exit the loop. // the action URL, exit the loop.
...@@ -180,20 +191,18 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) { ...@@ -180,20 +191,18 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
// |manager|. // |manager|.
manager.reset(*matched_manager_it); manager.reset(*matched_manager_it);
pending_login_managers_.weak_erase(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.host(), logger.get());
return;
} else { } else {
RecordFailure(NO_MATCHING_FORM, form.origin.host(), logger.get()); RecordFailure(NO_MATCHING_FORM, form.origin.host(), logger.get());
return; return;
} }
// If we found a manager but it didn't finish 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.
if (!manager->HasCompletedMatching()) {
RecordFailure(MATCHING_NOT_COMPLETE, form.origin.host(), logger.get());
return;
}
// Also get out of here if the user told us to 'never remember' passwords for // Also get out of here if the user told us to 'never remember' passwords for
// this form. // this form.
if (manager->IsBlacklisted()) { if (manager->IsBlacklisted()) {
...@@ -270,7 +279,7 @@ void PasswordManager::RecordFailure(ProvisionalSaveFailure failure, ...@@ -270,7 +279,7 @@ void PasswordManager::RecordFailure(ProvisionalSaveFailure failure,
logger->LogMessage(Logger::STRING_EMPTY_PASSWORD); logger->LogMessage(Logger::STRING_EMPTY_PASSWORD);
break; break;
case MATCHING_NOT_COMPLETE: case MATCHING_NOT_COMPLETE:
logger->LogMessage(Logger::STRING_NO_FORM_MANAGER); logger->LogMessage(Logger::STRING_MATCHING_NOT_COMPLETE);
break; break;
case NO_MATCHING_FORM: case NO_MATCHING_FORM:
logger->LogMessage(Logger::STRING_NO_MATCHING_FORM); logger->LogMessage(Logger::STRING_NO_MATCHING_FORM);
......
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