[Password Manager] Fix password saving on Macys registration page

[Password Manager] Fix password saving on Macys registration page

This requires a few changes to how we evaluate pages during the saving process.

1) Instead of ignoring forms where there are more than 3 password fields, assume that those fields are not related to actual passwords. Also change the case of 3 passwords to assume that if the first and second password match, that the third is not related. I don't have statistics on this, but I have seen security questions that are entered into password fields much more often than I have seen a new password entered before and old password (which was the previous assumption).

2) Move change password detection to work on the submitted password form instead of the observed form. The submitted form is more likely to be identified correctly because we have additional data after submission. This failed on Macy's because we incorrectly assume that, given the number of password fields, one of them is a current password field during parsing.

3) Don't require HTML field attributes to match for signup forms. This is normally required because otherwise we can't fill the password we have just saved. However in the case of signup forms this doesn't matter because we don't want to fill on the same form again anyway.

BUG=452306, 451631
Committed: https://crrev.com/33138a0083999ed6ca80ddf789756c25e6cac93a
Cr-Commit-Position: refs/heads/master@{#318850}

[Garrett Casto, 2015-02-25 01:10:16]

gcasto@chromium.org changed reviewers:
+ dvadym@chromium.org, vabr@chromium.org

[Garrett Casto, 2015-02-25 01:10:17]

One issue in this change is that it makes it possible for a signup form
submssion to match with an entirely different form if the HTML attributes change
and more than one form exists on the page.

This doesn't matter for matching that much because we wipe the html attributes
anyway, but it does matter for form_data. One possible solution would be to copy
the form data from the submitted_form in this case, but if the signature of the
form changes by submission time this would cause problem. I'm still thinking if
there is a better way to fix this. Suggestions welcome.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:407:
expected_saved_form.origin = saved_match()->origin;
This change tickled this bug. This previously worked because the observed and
submitted forms matched their actions even though the origins were different. In
reality this should never happen, as the origin of these two should always be
the same for PasswordManager to think that this particular PasswordFormManager
is the correct one.

[vabr (Chromium), 2015-02-25 10:21:15]

This LGTM so far, but you have a non-trivial conflict in rebasing to ToT ahead.

I have not much ideas about guarding against the false-positives in matching
saved forms against existing form managers (left one in the comments). Let's see
if that turns out being a problem. The biggest potential issue I can imagine is
saving passwords which we cannot autofill: taking lufthansa as an example -- if
Chrome constructs any password form manager on their login page, then after your
change it could possibly save the login form (recall that they switch the type
of the password field from text to password dynamically), but will never be able
to autofill it (the observed form has no password field on load). This does not
sound too severe if it happens on login forms (the user has typed the credential
before, so at least they have it elsewhere), maybe just confusing.

Cheers,
Vaclav

https://codereview.chromium.org/944163003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/944163003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:1587: //
Regression test for crbug.com/452306
nit: Please add http:// to make the link clickable in codesearch.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:48:
RESULT_HTML_ATTRIBUTES_MATCH = 1 << 1,  // Bare minimum to be a match.
The "bare minimum" comment is now a bit vague, since there are more than two
flags, and it is not clear which order the "minimum" is with respect to.
Probably just drop it, the code sides using these flags should be clear about
which flags constitute a good match.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:407:
expected_saved_form.origin = saved_match()->origin;
On 2015/02/25 01:10:17, Garrett Casto wrote:
> This change tickled this bug. This previously worked because the observed and
> submitted forms matched their actions even though the origins were different.
In
> reality this should never happen, as the origin of these two should always be
> the same for PasswordManager to think that this particular PasswordFormManager
> is the correct one.

Acknowledged.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:1133:
EXPECT_EQ(PasswordFormManager::RESULT_ORIGINS_MATCH,
optional: A shorter way to write this would be EXPECT_NE(0, ...), but I'm fine
with both.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:118: bool
IsIgnorableChangePasswordForm(const PasswordForm& form) {
Heads-up: Your base files are out of date. There has been
https://codereview.chromium.org/870513002 in the mean time, which introduced
checking the saved form credentials against the content of the password store.
Please make sure to not break what that fixed.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:289: //
TODO(gcasto): Matching in this way is very inprecise and will take
typo: imprecise

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:295:
matched_manager_it = iter;
Should we at least give precedence to matches coming from line 283? Something
like storing the last result, and if matched_manager_it is assigned, and the
last result better than just RESULT_ORIGINS_MATCH, we would not execute the
current line 295?

[Garrett Casto, 2015-02-26 07:11:02]

I think that the scenario that you outlined is pretty unlikely. We would need to
have |new_password_element| set for the form submission in order to save without
a good PasswordFormManager match, and I've never seen that happen on a login
form. I can imagine it would be possible if there were hidden password fields or
an OTP that uses a password field, but the combination of that plus changing
HTML attributes seems like it should be very rare.

https://codereview.chromium.org/944163003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/944163003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:1587: //
Regression test for crbug.com/452306
On 2015/02/25 10:21:15, vabr (Chromium) wrote:
> nit: Please add http:// to make the link clickable in codesearch.

Done.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:48:
RESULT_HTML_ATTRIBUTES_MATCH = 1 << 1,  // Bare minimum to be a match.
On 2015/02/25 10:21:15, vabr (Chromium) wrote:
> The "bare minimum" comment is now a bit vague, since there are more than two
> flags, and it is not clear which order the "minimum" is with respect to.
> Probably just drop it, the code sides using these flags should be clear about
> which flags constitute a good match.

Done.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:1133:
EXPECT_EQ(PasswordFormManager::RESULT_ORIGINS_MATCH,
On 2015/02/25 10:21:15, vabr (Chromium) wrote:
> optional: A shorter way to write this would be EXPECT_NE(0, ...), but I'm fine
> with both.

Acknowledged.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:118: bool
IsIgnorableChangePasswordForm(const PasswordForm& form) {
On 2015/02/25 10:21:15, vabr (Chromium) wrote:
> Heads-up: Your base files are out of date. There has been
> https://codereview.chromium.org/870513002 in the mean time, which introduced
> checking the saved form credentials against the content of the password store.
> Please make sure to not break what that fixed.

Rebased and moved this back into PasswordFormManager to incorporate both
changes.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:289: //
TODO(gcasto): Matching in this way is very inprecise and will take
On 2015/02/25 10:21:15, vabr (Chromium) wrote:
> typo: imprecise

Done.

https://codereview.chromium.org/944163003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:295:
matched_manager_it = iter;
On 2015/02/25 10:21:15, vabr (Chromium) wrote:
> Should we at least give precedence to matches coming from line 283? Something
> like storing the last result, and if matched_manager_it is assigned, and the
> last result better than just RESULT_ORIGINS_MATCH, we would not execute the
> current line 295?

That's a good idea. The change I made also will prefer action + origin matches
to just origin matches.

[vabr (Chromium), 2015-02-26 08:54:48]

LGTM!

You are right that my previous scenario is unlikely, I guess I forgot about the
check for a non-empty new password element.

Cheers,
Vaclav

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:46: // prefered
than lower matches.
typo: preferred

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:46: // prefered
than lower matches.
nit: I was a bit lost on what "preferred" means here, and why is it important.

From the meaning of the flags I understand that the higher the numeric value of
the match result, the better the forms match. So, e.g., two completely different
forms with the same origin are a better match than two identical forms on
slightly different origins. (Is that so?)

I understood the "why is it important" when looking in PasswordManager. It could
be helpful to explain here, that the numerical value of match results is
considered when choosing the best matching pending login manager for a submitted
form. That would make sure that whoever adds a new flag can check if the chosen
numerical value makes sense.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:92: //
"autocomplete=username" and the user-typed username and current password
nit: The comment could be a little clearer on how is |form| tied to the
user-typed username and password, and what other information of |form| is used.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:264:
~PasswordFormManager::RESULT_ACTION_MATCH)) {
Because action match is the lowest in value, a |result| satisfying the above
condition is guaranteed to be the second-best, and thus at least as good as
|current_match_result|. Therefore you currently don't need to do the "result >
current_match_result" test here.

But I would feel it were more robust to do that check, just to drop the implicit
assumption about the enum numerical values. It will change the behaviour
slightly in that the first matching (as opposed to the last matching) pending
login manager from this else-if branch will be chosen. But that should be OK
(and better for performance, anyway).

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:272:
current_match_result = result;
nit: Move this line after the whole if-else block (after line 286). That will
spare the duplication, and make it clearer that |current_match_result| is always
updated (unless the for-loop is exited early).

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:963: //
Saved signup forms don't have password and username elements set.
optional: The comment is speaking about a username element as well, but the code
below only clears the password element. Even if |expected_form| has the
username_element empty for some reason already, it could be easier to understand
to just clear it explicitly here.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:981:
ASSERT_TRUE(form_to_save.get());
nit: You can drop ".get()".

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:981:
ASSERT_TRUE(form_to_save.get());
optional/question: Why is this an ASSERT_*, when the code to fill that pointer
is under EXPECT_* ?

[Garrett Casto, 2015-02-27 01:12:33]

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:46: // prefered
than lower matches.
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> typo: preferred

Done.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:46: // prefered
than lower matches.
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> nit: I was a bit lost on what "preferred" means here, and why is it important.
> 
> From the meaning of the flags I understand that the higher the numeric value
of
> the match result, the better the forms match. So, e.g., two completely
different
> forms with the same origin are a better match than two identical forms on
> slightly different origins. (Is that so?)
> 
> I understood the "why is it important" when looking in PasswordManager. It
could
> be helpful to explain here, that the numerical value of match results is
> considered when choosing the best matching pending login manager for a
submitted
> form. That would make sure that whoever adds a new flag can check if the
chosen
> numerical value makes sense.

Done.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:92: //
"autocomplete=username" and the user-typed username and current password
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> nit: The comment could be a little clearer on how is |form| tied to the
> user-typed username and password, and what other information of |form| is
used.

Done.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:264:
~PasswordFormManager::RESULT_ACTION_MATCH)) {
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> Because action match is the lowest in value, a |result| satisfying the above
> condition is guaranteed to be the second-best, and thus at least as good as
> |current_match_result|. Therefore you currently don't need to do the "result >
> current_match_result" test here.
> 
> But I would feel it were more robust to do that check, just to drop the
implicit
> assumption about the enum numerical values. It will change the behaviour
> slightly in that the first matching (as opposed to the last matching) pending
> login manager from this else-if branch will be chosen. But that should be OK
> (and better for performance, anyway).

I'm not sure that I understand this second paragraph. What exactly are you
asking to change? Are you asking to update matched_manager_it if result >
current_match_result instead of checking for the specific value of
RESULT_COMPLETE_MATCH & ~RESULT_ACTION_MATCH? I don't think we want that change
in behavior because it would allow us to save login passwords that we won't
fill.

If you are asking for something else, I unfortunately don't know what it is.
Could you clarify?

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:272:
current_match_result = result;
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> nit: Move this line after the whole if-else block (after line 286). That will
> spare the duplication, and make it clearer that |current_match_result| is
always
> updated (unless the for-loop is exited early).

That's not actually true. The match may be worse, in which case we shouldn't
update matched_manager_it or current_match_result.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:963: //
Saved signup forms don't have password and username elements set.
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> optional: The comment is speaking about a username element as well, but the
code
> below only clears the password element. Even if |expected_form| has the
> username_element empty for some reason already, it could be easier to
understand
> to just clear it explicitly here.

Removed the comment about username_element. I updated the code and forget to
update the comment.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:981:
ASSERT_TRUE(form_to_save.get());
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> optional/question: Why is this an ASSERT_*, when the code to fill that pointer
> is under EXPECT_* ?

Because if this is false, then the call to Save() will crash. I generally find
it better to avoid crashing in this case since the core dumb doesn't give any
additional value

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:981:
ASSERT_TRUE(form_to_save.get());
On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> nit: You can drop ".get()".

Done.

[vabr (Chromium), 2015-03-02 13:23:12]

Still LGTM.

Cheers,
Vaclav

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:264:
~PasswordFormManager::RESULT_ACTION_MATCH)) {
On 2015/02/27 01:12:32, Garrett Casto wrote:
> On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> > Because action match is the lowest in value, a |result| satisfying the above
> > condition is guaranteed to be the second-best, and thus at least as good as
> > |current_match_result|. Therefore you currently don't need to do the "result
>
> > current_match_result" test here.
> > 
> > But I would feel it were more robust to do that check, just to drop the
> implicit
> > assumption about the enum numerical values. It will change the behaviour
> > slightly in that the first matching (as opposed to the last matching)
pending
> > login manager from this else-if branch will be chosen. But that should be OK
> > (and better for performance, anyway).
> 
> I'm not sure that I understand this second paragraph. What exactly are you
> asking to change? Are you asking to update matched_manager_it if result >
> current_match_result instead of checking for the specific value of
> RESULT_COMPLETE_MATCH & ~RESULT_ACTION_MATCH? I don't think we want that
change
> in behavior because it would allow us to save login passwords that we won't
> fill.
> 
> If you are asking for something else, I unfortunately don't know what it is.
> Could you clarify?  

Sorry for being unclear.

I was suggesting both keeping the current check
result == (RESULT_COMPLETE_MATCH & ~RESULT_ACTION_MATCH)
and adding also
result > current_match_result
(as you do on line 273).

The only thing this would change would be that the first, not the last, manager
matching this else-if branch would be chosen in the absence of better matches.
The code would be more explicit about the expectation that RESULT_COMPLETE_MATCH
& ~RESULT_ACTION_MATCH is actually the second best match, which is currently
implicit.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:272:
current_match_result = result;
On 2015/02/27 01:12:32, Garrett Casto wrote:
> On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> > nit: Move this line after the whole if-else block (after line 286). That
will
> > spare the duplication, and make it clearer that |current_match_result| is
> always
> > updated (unless the for-loop is exited early).
> 
> That's not actually true. The match may be worse, in which case we shouldn't
> update matched_manager_it or current_match_result.

Acknowledged. You are correct, I managed to miss in that comment that this is
"else if", not "else".

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:963: //
Saved signup forms don't have password and username elements set.
On 2015/02/27 01:12:33, Garrett Casto wrote:
> On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> > optional: The comment is speaking about a username element as well, but the
> code
> > below only clears the password element. Even if |expected_form| has the
> > username_element empty for some reason already, it could be easier to
> understand
> > to just clear it explicitly here.
> 
> Removed the comment about username_element. I updated the code and forget to
> update the comment.

Acknowledged.

https://codereview.chromium.org/944163003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:981:
ASSERT_TRUE(form_to_save.get());
On 2015/02/27 01:12:32, Garrett Casto wrote:
> On 2015/02/26 08:54:47, vabr (OOO until 2 March) wrote:
> > optional/question: Why is this an ASSERT_*, when the code to fill that
pointer
> > is under EXPECT_* ?
> 
> Because if this is false, then the call to Save() will crash. I generally find
> it better to avoid crashing in this case since the core dumb doesn't give any
> additional value

Acknowledged.

[Garrett Casto, 2015-03-03 06:42:23]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-03-03 06:42:24]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/944163003/#ps100001
(title: "Comment")

[commit-bot: I haz the power, 2015-03-03 06:43:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944163003/100001

[commit-bot: I haz the power, 2015-03-03 07:43:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-03 07:44:19]

Patchset 6 (id:??) landed as
https://crrev.com/33138a0083999ed6ca80ddf789756c25e6cac93a
Cr-Commit-Position: refs/heads/master@{#318850}