Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
870513002:
[PasswordManager] Improve detection of ignorable change password forms. (Closed)
|
Created:
5 years, 11 months ago by Pritam Nikam Modified:
5 years, 10 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Improve detection of ignorable change password forms.
BUG=448351
Committed: https://crrev.com/7a6e8ea50fb5892f35314d26797f5a173245be0d
Cr-Commit-Position: refs/heads/master@{#317577}
Patch Set 1 : #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Addresses Vaclav's review inputs. #
Total comments: 22
Patch Set 4 : #
Total comments: 2
Patch Set 5 : Incorporates review comments. #Patch Set 6 : Fixed breakage. #
Total comments: 10
Patch Set 7 : Addressed review comments. #
Total comments: 32
Patch Set 8 : Incorporated reviews. #
Total comments: 6
Patch Set 9 : Incorporated reviews. #
Messages
Total messages: 33 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
pritam.nikam@samsung.com changed reviewers: + vabr@chromium.org
Hi Vaclav, I was roughly talking about something like this CL. This is purely a WIP, and please consider it for further discussion/modifications. Please have a glance and share your thoughts. Thanks!
On 2015/01/22 11:31:46, Pritam Nikam wrote: > Hi Vaclav, > > I was roughly talking about something like this CL. This is purely a WIP, and > please consider it for further discussion/modifications. Please have a glance > and share your thoughts. > Thanks! Thanks, Pritam! I'm unfortunately unavailable for the rest of the week, but will have a look next week. P.S. Don't forget to assign the bug to yourself if you work on it!
Hi Pritam Nikam. I'm afraid this is a little too ambitious: trying to guess the username if it is not present sounds risky, we would be likely overwriting wrong credentials (in particular, all credentials on the same domain) with the new password. I suggest aiming for change-password forms, for which we do have a username candidate -- then we take the username candidate and check the store, if there is a form with the same username, and the same old password. If there is, we can update it. Saving new passwords (as opposed to updating existing) should also still work for change-password forms with the autocomplete=username attribute. There we need no checks for existing usernames. I left some semi-random comments, but will have more once you come back with a more final version of the CL. Cheers, Vaclav https://codereview.chromium.org/870513002/diff/60001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:898: for (PasswordFormMap::const_iterator iter = best_matches_.begin(); nit: You can use for (const auto& match : best_matches_) {...} https://codereview.chromium.org/870513002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:900: if (net::registry_controlled_domains::SameDomainOrHost( This is too permissive, we cannot just update passwords across domains. We do not do that for PSL matched permissions either. https://codereview.chromium.org/870513002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:904: if (matching_form != nullptr) nit: Just write: if (matching_form)
Thanks Vaclav for review. With new patch set (#3) I was trying to get align with your suggestions, hope I've interpreted pointers correctly. Thanks! https://codereview.chromium.org/870513002/diff/60001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:898: for (PasswordFormMap::const_iterator iter = best_matches_.begin(); On 2015/01/28 13:25:32, vabr (Chromium) wrote: > nit: You can use > for (const auto& match : best_matches_) {...} Done. https://codereview.chromium.org/870513002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:900: if (net::registry_controlled_domains::SameDomainOrHost( On 2015/01/28 13:25:32, vabr (Chromium) wrote: > This is too permissive, we cannot just update passwords across domains. We do > not do that for PSL matched permissions either. Done. https://codereview.chromium.org/870513002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:904: if (matching_form != nullptr) On 2015/01/28 13:25:32, vabr (Chromium) wrote: > nit: Just write: > if (matching_form) Done.
Thanks Vaclav for your inputs. I've tried addressing you input along new patchset #3, PTAL! On 2015/01/28 13:25:32, vabr (Chromium) wrote: > Hi Pritam Nikam. > > I'm afraid this is a little too ambitious: trying to guess the username if it is > not present sounds risky, we would be likely overwriting wrong credentials (in > particular, all credentials on the same domain) with the new password. > > I suggest aiming for change-password forms, for which we do have a username > candidate -- then we take the username candidate and check the store, if there > is a form with the same username, and the same old password. If there is, we can > update it. > Yes I agree, this is a bit of ambitious way of approching the problem. But for change-password forms most of the popular web sites do not offer username fields (facebook, linkedin etc...), and in my opinion, for practical scenarios the chances that a multiple users having same password saved on same chromium browser instance on a device is quite rare. > Saving new passwords (as opposed to updating existing) should also still work > for change-password forms with the autocomplete=username attribute. There we > need no checks for existing usernames. > If I've understand it right, this shall always add new entry to password manager. And at times probably that is we do not want to happen (?). > I left some semi-random comments, but will have more once you come back with a > more final version of the CL. > I've addressed these in #3. Thanks!
Patchset #4 (id:100001) has been deleted
Hi Pritam Nikam. I left a number of comments, because I question most of the changes in the CL so far. But then I took a step back, and realised, that I don't even understand the plan of the CL. You don't have tests (which I would ask for before landing anyway, but we are not that that far yet). Did you try this manually? This CL should target change-password forms without the autocomplete=username mark-up. But how would such a form even reach PasswordFormManager::ProvisionallySave through the IsIgnorableChangePasswordForm check in PasswordManager::ProvisionallySavePassword? Please take your time and review your plan. Once you have a clear plan, write the CL. Have a short break, return, check for obvious details like relics of bad merges, and then send it back for review. Thanks, Vaclav https://codereview.chromium.org/870513002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:256: bool is_username_certainly_correct = credentials.username_marked_by_site; nit: Because credentials.username_marked_by_site is not much longer than is_username_certainly_correct, and you use that bool only once here anyway, let's just get rid of the bool and use credentials.username_marked_by_site in its place instead. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:264: if (!is_username_certainly_correct && it != best_matches_.end()) { Don't narrow the condition this way -- observe that you are changing the behaviour for all types of forms (not just change-password forms), as long as they have the autocomplete=username markup. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:320: HasMatchingCredentialsInStore(credentials, &matching_form)) { This is actually unnecessary -- you already know that the single form among best_matches_, which matches credentials' username is *it->second, already stored as pending_crendentials_ on line 266. So just check the password. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:321: pending_credentials_ = matching_form; Save the copy -- just pass pending_credentials_ to HasMatchingCredentialsInStore and get rid of matching_form. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:321: pending_credentials_ = matching_form; Given the other comments, the modification needed to perform on pending_credentials_ boils down to updating the password. But that is done on line 365 below. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:322: selected_username_ = matching_form.username_value; Why do you set this? According to password_form_manager.h, this is set "if the user has selected one of the other possible usernames". However, line 263 tells me that the username of |credentials| matches one suggested in |best_matches_|. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:323: user_action_ = kUserActionOverridePassword; How do you know the password is overridden? Shouldn't you check that? https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:348: // elements are likely different than those on a login form, so do not Please do not just reformat the comments, if you don't change them. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:402: !base::CommandLine::ForCurrentProcess()->HasSwitch( This change looks like a bad merge, and nothing to do with your CL? Also applies to the 2 new #includes at the beginning of this file. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:898: if (matching_form) There should also be a check that the candidate has the same realm as the forms in best_matches_. You can compare the realms with IsPublicSuffixDomainMatch from components/password_manager/core/browser/psl_matching_helper.h. Alternatively, because this is a private function, and it can be reasoned that it is always applied to forms with the same realm (because of all forms being related to the same PasswordFormManager instance), you can make that check a DCHECK, and add a comment with the expectation about dandidate.signon_realm expectation at the method declaration. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.h:285: // Returns true if |candidate|'s registry controlled domain for origin and Please update the comment. Also mention that |matching_form| can be null.
> You don't have tests (which I would ask for before landing anyway, but we are > not that that far yet). Did you try this manually? This CL should target > change-password forms without the autocomplete=username mark-up. But how would > such a form even reach PasswordFormManager::ProvisionallySave through the > IsIgnorableChangePasswordForm check in > PasswordManager::ProvisionallySavePassword? Ah, sorry, I overlooked that you removed the check from PasswordManager. That would explain how this worked. But why not keep this test instead and extend it so that more forms pass it? Would PasswordFormManager::ProvisionallySave not work automatically? The current implementation is rather messy, it is not clear how it impacts forms other than change-password forms with no autocomplete=username markup.
https://codereview.chromium.org/870513002/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager.cc (left): https://codereview.chromium.org/870513002/diff/120001/components/password_man... components/password_manager/core/browser/password_manager.cc:237: if ((*iter)->IsIgnorableChangePasswordForm()) { Should you also delete the method, which is no longer used?
Patchset #5 (id:140001) has been deleted
Thanks Vaclav for thorough review. I've tried addressing most of your inputs, PTAL. Thanks! > Ah, sorry, I overlooked that you removed the check from PasswordManager. That > would explain how this worked. But why not keep this test instead and extend it > so that more forms pass it? Would PasswordFormManager::ProvisionallySave not > work automatically? The current implementation is rather messy, it is not clear > how it impacts forms other than change-password forms with no > autocomplete=username markup. Plan for now is to handle change-password forms. And to get stored forms inside PasswordFormManager::ProvisionallySave() few arrangements like ignoring this check is needed, along with changes made within |PasswordFormManager::FetchMatchingLoginsFromPasswordStore|. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:256: bool is_username_certainly_correct = credentials.username_marked_by_site; On 2015/02/02 14:18:49, vabr (Chromium) wrote: > nit: Because credentials.username_marked_by_site is not much longer than > is_username_certainly_correct, and you use that bool only once here anyway, > let's just get rid of the bool and use credentials.username_marked_by_site in > its place instead. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:264: if (!is_username_certainly_correct && it != best_matches_.end()) { On 2015/02/02 14:18:49, vabr (Chromium) wrote: > Don't narrow the condition this way -- observe that you are changing the > behaviour for all types of forms (not just change-password forms), as long as > they have the autocomplete=username markup. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:320: HasMatchingCredentialsInStore(credentials, &matching_form)) { On 2015/02/02 14:18:49, vabr (Chromium) wrote: > This is actually unnecessary -- you already know that the single form among > best_matches_, which matches credentials' username is *it->second, already > stored as pending_crendentials_ on line 266. So just check the password. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:321: pending_credentials_ = matching_form; On 2015/02/02 14:18:50, vabr (Chromium) wrote: > Given the other comments, the modification needed to perform on > pending_credentials_ boils down to updating the password. But that is done on > line 365 below. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:321: pending_credentials_ = matching_form; On 2015/02/02 14:18:50, vabr (Chromium) wrote: > Save the copy -- just pass pending_credentials_ to HasMatchingCredentialsInStore > and get rid of matching_form. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:322: selected_username_ = matching_form.username_value; On 2015/02/02 14:18:50, vabr (Chromium) wrote: > Why do you set this? According to password_form_manager.h, this is set "if the > user has selected one of the other possible usernames". However, line 263 tells > me that the username of |credentials| matches one suggested in |best_matches_|. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:323: user_action_ = kUserActionOverridePassword; On 2015/02/02 14:18:49, vabr (Chromium) wrote: > How do you know the password is overridden? Shouldn't you check that? Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:348: // elements are likely different than those on a login form, so do not On 2015/02/02 14:18:49, vabr (Chromium) wrote: > Please do not just reformat the comments, if you don't change them. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:402: !base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/02/02 14:18:49, vabr (Chromium) wrote: > This change looks like a bad merge, and nothing to do with your CL? > Also applies to the 2 new #includes at the beginning of this file. Done. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:898: if (matching_form) On 2015/02/02 14:18:49, vabr (Chromium) wrote: > There should also be a check that the candidate has the same realm as the forms > in best_matches_. You can compare the realms with IsPublicSuffixDomainMatch from > components/password_manager/core/browser/psl_matching_helper.h. > > Alternatively, because this is a private function, and it can be reasoned that > it is always applied to forms with the same realm (because of all forms being > related to the same PasswordFormManager instance), you can make that check a > DCHECK, and add a comment with the expectation about dandidate.signon_realm > expectation at the method declaration. Done. Removed this function, instead performed in-place check for password value. https://codereview.chromium.org/870513002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/870513002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.h:285: // Returns true if |candidate|'s registry controlled domain for origin and On 2015/02/02 14:18:50, vabr (Chromium) wrote: > Please update the comment. Also mention that |matching_form| can be null. Removed this function. https://codereview.chromium.org/870513002/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager.cc (left): https://codereview.chromium.org/870513002/diff/120001/components/password_man... components/password_manager/core/browser/password_manager.cc:237: if ((*iter)->IsIgnorableChangePasswordForm()) { On 2015/02/02 14:22:53, vabr (Chromium) wrote: > Should you also delete the method, which is no longer used? I'm keeping this as a private function.
Hi Pritam Nikam, Sorry, but this still not LGTM. > Plan for now is to handle change-password forms. And to get stored forms inside > PasswordFormManager::ProvisionallySave() few arrangements like ignoring this > check is needed, along with changes made within > |PasswordFormManager::FetchMatchingLoginsFromPasswordStore|. I don't think I understand your plan. The bug http://crbug.com/448351 clearly describes the goal, which is "Improve detection of ignorable change password forms". In particular it is not to "handle" them. In particular, please do not mix autofilling forms into this CL, that needs to be done separately, and will likely demand more changes, design, and a separate tracking bug. One of the plans to achieve the above goal would be to extend PasswordFormManager::IsIgnorableChangePasswordForm with the check for similar credentials. That sounds like a clean solution with minimal changes. What you do is much more disruptive, and you have not so far explained what are the advantages of that approach. You postpone the checking for ignorable change password forms (performance implications), put it on two places instead one (how do you make sure that they are and remain the complete list of places needing such check), and add some other changes which do not seem related to the goal. Please consider the above, and your choice. And then try to explain your intention, discuss which alternatives you considered, and why you chose what you chose. Thanks, Vaclav https://codereview.chromium.org/870513002/diff/180001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (left): https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:379: // Do not autofill on sign-up or change password forms (until we have some The goal of http://crbug.com/448351 is not at all to support autofilling on sign-up and change password forms. Revert this deletion. https://codereview.chromium.org/870513002/diff/180001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:307: // For ignorable change-password form, bail out early if usernames are Why should change password forms not be ignored in the presence of PSL matches? https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:551: if (observed_form_.new_password_element.empty()) { What's the advantage of moving the check to so late? https://codereview.chromium.org/870513002/diff/180001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1348: SubmitIngnorableChanePasswordForm_MatchingUsernameAndPassword) { Chane -> Change https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1371: // Successful match found. The PasswordManager would instruct would -> should
On 2015/02/06 16:27:07, vabr (Chromium) wrote: > Hi Pritam Nikam, > > Sorry, but this still not LGTM. > > > Plan for now is to handle change-password forms. And to get stored forms > inside > > PasswordFormManager::ProvisionallySave() few arrangements like ignoring this > > check is needed, along with changes made within > > |PasswordFormManager::FetchMatchingLoginsFromPasswordStore|. > > I don't think I understand your plan. > > The bug http://crbug.com/448351 clearly describes the goal, which is "Improve > detection of ignorable change password forms". In particular it is not to > "handle" them. In particular, please do not mix autofilling forms into this CL, > that needs to be done separately, and will likely demand more changes, design, > and a separate tracking bug. > > One of the plans to achieve the above goal would be to extend > PasswordFormManager::IsIgnorableChangePasswordForm with the check for similar > credentials. That sounds like a clean solution with minimal changes. > > What you do is much more disruptive, and you have not so far explained what are > the advantages of that approach. You postpone the checking for ignorable change > password forms (performance implications), put it on two places instead one (how > do you make sure that they are and remain the complete list of places needing > such check), and add some other changes which do not seem related to the goal. > > Please consider the above, and your choice. And then try to explain your > intention, discuss which alternatives you considered, and why you chose what you > chose. > > Thanks, > Vaclav > Hi Vaclav, thanks again for highlighting the problems with this CL. May be the choice of word "handle" is poor here, this patch still talks of improvement for detecting the ignorable change password forms. My intentions with this CL is to check whether for "change password forms" usernames & passwords are matching with existing form in store i.e. in |this->best_matches|, and if so its still *not a ignorable change password form*; otherwise its safe to go ahead and proceed with ProvisionallySave() which shall ignore any such case. But before I justfy this CL's motivation, please help me with below understanding: 1. Presently, for Change-password forms we do not populate the stored forms and instead skip fetching matching forms & keep the |this->best_matches| map explictly empty. Apparently, autofilling change-password form is not entertain with the current UX. (Refer: [A]). [A] https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... 2. |this->observed_form_| shall get populated once password form get render, and shall not hold the user filled username, password & new-password and hence extending it will either have to pass |provisionally_saved_form| from password_manger to IsIgnorableChangePasswordForm(), something like below: bool PasswordFormManager::IsIgnorableChangePasswordForm( const autofill::PasswordForm& credentials) const { bool is_change_password_form = !observed_form_.new_password_element.empty() && !observed_form_.password_element.empty(); bool is_username_certainly_correct = observed_form_.username_marked_by_site; return is_change_password_form && !is_username_certainly_correct && !MatchingCredentialsInStore(credentials); } bool PasswordFormManager::MatchingCredentialsInStore( const autofill::PasswordForm& credentials) const { for (auto match: best_matches_) { if (match.second->username_value == credentials.username_value && match.second->password_value == credentials.password_value) return true; } return false; } Or, do all checks within ProvisionallySave(). I went with later approach with this CL, and that apparently a wrong choice, as I've end up it calling it twice and late that has performance penalty (I came up with putting this check at 2 places was with my testing on sample pages). Moreover, as mentioned in point 1, to populate the stored forms within |this->best_matches| I've to skip this check performed within [A], but just to make sure that the none of the field shall be autofilled, I've moved that check to PasswordFormManager::ProcessFrame(). I'll upload a new patch set, by extending IsIgnorableChangePasswordForm() to make sure implementation looks clean & execution control shall not reach till ProvisionallySave() for genuine cases. But changes made within FetchMatchingLoginsFromPasswordStore() and ProcessFrame() are still must in my opinion. Thanks!
Hi Vaclav, PTAL @ new patch set. Thanks! https://codereview.chromium.org/870513002/diff/180001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (left): https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:379: // Do not autofill on sign-up or change password forms (until we have some On 2015/02/06 16:27:07, vabr (Chromium) wrote: > The goal of http://crbug.com/448351 is not at all to support autofilling on > sign-up and change password forms. Revert this deletion. In my opinion, this is needed to fetch stored forms and identify genuine ignorable change-password form cases. We can skip autofilling for all change-password form in later part. https://codereview.chromium.org/870513002/diff/180001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:307: // For ignorable change-password form, bail out early if usernames are On 2015/02/06 16:27:07, vabr (Chromium) wrote: > Why should change password forms not be ignored in the presence of PSL matches? Done. https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:551: if (observed_form_.new_password_element.empty()) { On 2015/02/06 16:27:07, vabr (Chromium) wrote: > What's the advantage of moving the check to so late? We can skip autofilling for all change-password form here. https://codereview.chromium.org/870513002/diff/180001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1348: SubmitIngnorableChanePasswordForm_MatchingUsernameAndPassword) { On 2015/02/06 16:27:07, vabr (Chromium) wrote: > Chane -> Change Done. https://codereview.chromium.org/870513002/diff/180001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1371: // Successful match found. The PasswordManager would instruct On 2015/02/06 16:27:07, vabr (Chromium) wrote: > would -> should Done.
Hi Pritam Nikam. Thanks, this looks already much better. I agree with you that we actually do need to fetch the stored credentials even for change-password forms now, to be able to check the typed credentials. I left some comments, please have a look. Cheers, Vaclav https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:543: // Do not autofill on sign-up or change password forms (until we have some Could you move the whole added block above line 530, which says "Proceed to autofill"? https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:546: scoped_ptr<BrowserSavePasswordProgressLogger> logger; You only need the |logger| in the if-block below, so don't use scoped_ptr, create it simply as a stack variable. Also, please add a log message identifying the current method (see the *_METHOD constants in components/autofill/core/common/save_password_progress_logger.cc for the pattern to use). In other words, please do this: if (...IsLoggingActive...) { BrowserSavePasswordProgressLogger logger(client_); logger.LogMessage(Logger::PROCESS_FRAME_METHOD); logger.LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED); } https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:551: client_->AutofillResultsComputed(); Remove this line. AutofillResultsComputed has already been called when the password store results were received. https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.h:87: // Returns true if the |candidate| form has both the current and new password Most of this comment is out of date, please improve. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.h:87: // Returns true if the |candidate| form has both the current and new password I have another slight problem with the current wording: The "Form" from the method name, means the HTML form on the page for which |this| was created. In other words, it still means the observed_form_. The role of the argument |candidate| is just to provide an evidence against the form being ignorable. While |candidate| and |observed_form_| are both of the same data type, the former represents just what the user typed here, while the latter represents the HTML form in which the user typed it. Therefore I suggest replacing |candidate| with a pair of strings, the username and password values from |candidate|: bool IsIgnorableChangePasswordForm(const base::string16& typed_username, const base::string16& typed_password) const; https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.h:286: // Returns true if |candidate|'s username and password field value matches To avoid increasing the header file, I suggest changing this method to a function in the anonymous namespace in the .cc file. For that it will need to take an additional argument, a const ref to the best_matches_, but that's OK. The name should be changed accordingly, to something like bool DoesCandidateMatchCredentials(const autofill::PasswordForm& candidate, const autofill::PasswordFormMap& credentials); https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1353: PasswordFormManager manager(NULL, &client_with_store, NULL -> nullptr Also below. (See http://chromium-cpp.appspot.com/.) https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1356: EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord()) Does the test really expect that call? If this is only about returning false, let's use ON_CALL(...).WillByDefault(Return(false)); Also in the new tests below. (I know that this is just a copy of these lines from other parts of the unittest, but let's leave cleaning the rest up for different CLs, and only make sure what we add is right.) https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1360: // User submits ignorable change-password form (i.e. without Suggested rewording, because "ignorable" has unclear meaning here: The user submits a password on a change-password form, which does not use the "autocomplete=username" mark-up (therefore Chrome had to guess what is the username), but the user-typed credentials match something already stored (which confirms that the guess was right). https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1367: credentials.preferred = true; Does the test actually need |credentials| to be preferred? https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1399: // User submits ignorable change-password form (i.e. without Suggested rewording: The user submits a password on a change-password form, which does not use the "autocomplete=username" mark-up (therefore Chrome had to guess what is the username), and the user-typed password do not match anything already stored. There is not much confidence in the guess being right, so the password should not be stored. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1410: // No match found. Password manager shall not trigger the save or update the Actually, I would be surprised if this test worked (once you fix the EXPECT_CALL as outlined below). That's because the decision not to save the password is done in PasswordManager, not PasswordFormManager. Normally, the call from line 1407 would not happen. But once it does happen, the expectations below will not be met. I'm afraid you'll need to find a way to move all these three tests to password manager unittest, because it is actually PasswordManager we are testing here. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1412: EXPECT_FALSE(manager.IsNewLogin()); I suggest dropping this line. It seems to just test that the default value for IsNewLogin is false, which is not what this test should do. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1413: EXPECT_CALL(*mock_store(), UpdateLogin(_)).Times(testing::Exactly(0)); You need to put this expectation before the ProvisionallySave call, and also you need to run RunAllPendingTasks() after ProvisionallySave, to be sure that the password store query would have been run. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1429: // User submits ignorable change-password form (i.e. without Suggested rewording: The user submits a password on a change-password form, which does not use the "autocomplete=username" mark-up (therefore Chrome had to guess what is the username), and the user-typed username does not match anything already stored. There is not much confidence in the guess being right, so the password should not be stored. https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:282: std::vector<PasswordForm*> result; // Empty password store. Please revert the added lines 282 and 284-285, they are not needed any more. In a recent change https://codereview.chromium.org/866983003/ I supplied the empty store to all these tests by default.
Thanks Vaclav for inputs. I've incorporated review comments with new patchset PTAL. Thanks! https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:543: // Do not autofill on sign-up or change password forms (until we have some On 2015/02/10 18:54:56, vabr (OOO until 22 Feb) wrote: > Could you move the whole added block above line 530, which says "Proceed to > autofill"? Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:546: scoped_ptr<BrowserSavePasswordProgressLogger> logger; On 2015/02/10 18:54:56, vabr (OOO until 22 Feb) wrote: > You only need the |logger| in the if-block below, so don't use scoped_ptr, > create it simply as a stack variable. > Also, please add a log message identifying the current method (see the *_METHOD > constants in components/autofill/core/common/save_password_progress_logger.cc > for the pattern to use). In other words, please do this: > if (...IsLoggingActive...) { > BrowserSavePasswordProgressLogger logger(client_); > logger.LogMessage(Logger::PROCESS_FRAME_METHOD); > logger.LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED); > } Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:551: client_->AutofillResultsComputed(); On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Remove this line. AutofillResultsComputed has already been called when the > password store results were received. Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.h:87: // Returns true if the |candidate| form has both the current and new password On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > I have another slight problem with the current wording: > The "Form" from the method name, means the HTML form on the page for which > |this| was created. In other words, it still means the observed_form_. The role > of the argument |candidate| is just to provide an evidence against the form > being ignorable. While |candidate| and |observed_form_| are both of the same > data type, the former represents just what the user typed here, while the latter > represents the HTML form in which the user typed it. > > Therefore I suggest replacing |candidate| with a pair of strings, the username > and password values from |candidate|: > > bool IsIgnorableChangePasswordForm(const base::string16& typed_username, const > base::string16& typed_password) const; Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.h:87: // Returns true if the |candidate| form has both the current and new password On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Most of this comment is out of date, please improve. Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager.h:286: // Returns true if |candidate|'s username and password field value matches On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > To avoid increasing the header file, I suggest changing this method to a > function in the anonymous namespace in the .cc file. > For that it will need to take an additional argument, a const ref to the > best_matches_, but that's OK. The name should be changed accordingly, to > something like > bool DoesCandidateMatchCredentials(const autofill::PasswordForm& candidate, > const autofill::PasswordFormMap& credentials); Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1353: PasswordFormManager manager(NULL, &client_with_store, On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > NULL -> nullptr > Also below. > (See http://chromium-cpp.appspot.com/.) Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1356: EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord()) On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Does the test really expect that call? > If this is only about returning false, let's use > ON_CALL(...).WillByDefault(Return(false)); > > Also in the new tests below. > > (I know that this is just a copy of these lines from other parts of the > unittest, but let's leave cleaning the rest up for different CLs, and only make > sure what we add is right.) Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1360: // User submits ignorable change-password form (i.e. without On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Suggested rewording, because "ignorable" has unclear meaning here: > > The user submits a password on a change-password form, which does not use the > "autocomplete=username" mark-up (therefore Chrome had to guess what is the > username), but the user-typed credentials match something already stored (which > confirms that the guess was right). Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1367: credentials.preferred = true; On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Does the test actually need |credentials| to be preferred? Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1399: // User submits ignorable change-password form (i.e. without On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Suggested rewording: > > The user submits a password on a change-password form, which does not use the > "autocomplete=username" mark-up (therefore Chrome had to guess what is the > username), and the user-typed password do not match anything already stored. > There is not much confidence in the guess being right, so the password should > not be stored. Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1410: // No match found. Password manager shall not trigger the save or update the On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Actually, I would be surprised if this test worked (once you fix the EXPECT_CALL > as outlined below). > That's because the decision not to save the password is done in PasswordManager, > not PasswordFormManager. Normally, the call from line 1407 would not happen. But > once it does happen, the expectations below will not be met. > > I'm afraid you'll need to find a way to move all these three tests to password > manager unittest, because it is actually PasswordManager we are testing here. Instead we will test the public function IsIgnorableChangePasswordForm() and if it is returning false i.e. its not an ignorable credentials; we will go ahead and test ProvisionallySave(). https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1412: EXPECT_FALSE(manager.IsNewLogin()); On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > I suggest dropping this line. It seems to just test that the default value for > IsNewLogin is false, which is not what this test should do. Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1413: EXPECT_CALL(*mock_store(), UpdateLogin(_)).Times(testing::Exactly(0)); On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > You need to put this expectation before the ProvisionallySave call, and also you > need to run RunAllPendingTasks() after ProvisionallySave, to be sure that the > password store query would have been run. Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1429: // User submits ignorable change-password form (i.e. without On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Suggested rewording: > > The user submits a password on a change-password form, which does not use the > "autocomplete=username" mark-up (therefore Chrome had to guess what is the > username), and the user-typed username does not match anything already stored. > There is not much confidence in the guess being right, so the password should > not be stored. Done. https://codereview.chromium.org/870513002/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:282: std::vector<PasswordForm*> result; // Empty password store. On 2015/02/10 18:54:57, vabr (OOO until 22 Feb) wrote: > Please revert the added lines 282 and 284-285, they are not needed any more. In > a recent change https://codereview.chromium.org/866983003/ I supplied the empty > store to all these tests by default. Done.
Thanks, Pritam. This LGTM already, with some comments below. Cheers, Vaclav https://codereview.chromium.org/870513002/diff/220001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/870513002/diff/220001/components/password_man... components/password_manager/core/browser/password_form_manager.h:90: // field values do not match to the credentials already stored. In these cases grammar nit: match to the -> match the https://codereview.chromium.org/870513002/diff/220001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/220001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1351: SubmitIngnorableChangePasswordForm_MatchingUsernameAndPassword) { nit: Please rename the new tests by replacing "Submit" with "Is" to match the tested method name. https://codereview.chromium.org/870513002/diff/220001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1372: manager.ProvisionallySave( I suggest to drop the lines 1372-1383. They don't involve any code using IsIgnorableChangePasswordForm. The test should test just one thing (IsIgnorableChangePasswordForm), not more (e.g., whether ProvisionallySave works), unless they are inseparable.
Also, a note for future CLs: Please try to separate rebasing and own changes. Here, patch set 8 contained both your changes against patch set 7, and was rebased on a newer state of the codebase. That means that if the reviewer displays the diff between patches 7 and 8, your changes get mixed with the changes in the different bases. This makes an incremental review impossible. Instead, next time when you start making changes to your work, first do just "git pull --rebase" and immediately afterwards "git cl upload", with a CL description "just a rebase" (or similar). Then add your changes, and upload another patch set without rebasing in between. That would lead to 2 new patch sets, 8 and 9, and a separation of the diff between the bases (7 vs. 8), and the diff of your changes (8 vs. 9). Thanks! Vaclav
New patchsets have been uploaded after l-g-t-m from vabr@chromium.org
Thanks Vaclav! Addresses your inputs PTAL, Thanks! https://codereview.chromium.org/870513002/diff/220001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/870513002/diff/220001/components/password_man... components/password_manager/core/browser/password_form_manager.h:90: // field values do not match to the credentials already stored. In these cases On 2015/02/23 10:15:26, vabr (Chromium) wrote: > grammar nit: > match to the -> match the Done. https://codereview.chromium.org/870513002/diff/220001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/870513002/diff/220001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1351: SubmitIngnorableChangePasswordForm_MatchingUsernameAndPassword) { On 2015/02/23 10:15:26, vabr (Chromium) wrote: > nit: Please rename the new tests by replacing "Submit" with "Is" to match the > tested method name. Done. https://codereview.chromium.org/870513002/diff/220001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1372: manager.ProvisionallySave( On 2015/02/23 10:15:26, vabr (Chromium) wrote: > I suggest to drop the lines 1372-1383. They don't involve any code using > IsIgnorableChangePasswordForm. The test should test just one thing > (IsIgnorableChangePasswordForm), not more (e.g., whether ProvisionallySave > works), unless they are inseparable. Done.
On 2015/02/23 10:19:49, vabr (Chromium) wrote: > Also, a note for future CLs: > > Please try to separate rebasing and own changes. Here, patch set 8 contained > both your changes against patch set 7, and was rebased on a newer state of the > codebase. That means that if the reviewer displays the diff between patches 7 > and 8, your changes get mixed with the changes in the different bases. This > makes an incremental review impossible. > > Instead, next time when you start making changes to your work, first do just > "git pull --rebase" and immediately afterwards "git cl upload", with a CL > description "just a rebase" (or similar). Then add your changes, and upload > another patch set without rebasing in between. That would lead to 2 new patch > sets, 8 and 9, and a separation of the diff between the bases (7 vs. 8), and the > diff of your changes (8 vs. 9). > > Thanks! > Vaclav I'll make of a note of this in future patches. Thanks!
https://codereview.chromium.org/870513002/#ps240001 LGTM. Thanks! Vaclav
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/870513002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/870513002/240001
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7a6e8ea50fb5892f35314d26797f5a173245be0d Cr-Commit-Position: refs/heads/master@{#317577} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
