|
|
Created:
3 years, 6 months ago by melandory Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTeach PasswordAutofillAgent sometimes match prefixes of usernames.
This CL fixes the issue that is specific to email provider web sites in following scenario:
1. The user is able to log in with username "someuser".
2) Chrome stores the credentials with "someuser" as the username.
3) On a next visit to a web site the full username someuser@example.com is prefilled.
5) Chrome does not fill in the stored password, because its username "someuser" does not exactly match the content of the read-only field.
The CL relaxes the matching condition in case the read-only username field and when the prefilled username looks like email.
BUG=543435
Review-Url: https://codereview.chromium.org/2906383003
Cr-Commit-Position: refs/heads/master@{#481569}
Committed: https://chromium.googlesource.com/chromium/src/+/d2290e01a65d77cc3b21bbfe67295938f97f5e3e
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 8
Patch Set 4 : dvadym@ comments #Patch Set 5 : dvadym@ comments #
Total comments: 30
Patch Set 6 : some Vaclav comments #Patch Set 7 : drop check #Patch Set 8 : format #Patch Set 9 : IsPrefixEndingAtSign IsPrefixOfEmailEndingAtSign #Patch Set 10 : . #Patch Set 11 : more browser tests #
Total comments: 12
Patch Set 12 : . #
Total comments: 2
Patch Set 13 : . #
Dependent Patchsets: Messages
Total messages: 73 (61 generated)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Password manager should deal with username being read-only and only partly-matching stored credentials. BUG=543435 ========== to ========== Partly match stored username. BUG=543435 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
melandory@chromium.org changed reviewers: + dvadym@chromium.org
PTAL at *password* files. Thanks!
In general it looks good. Some comments below. https://codereview.chromium.org/2906383003/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:277: current_username, potential_suggestion, true)); As far as I understand the main use case is emails, so it looks that we can compare Case Insensitive. And probably even remove |case_sensitive| argument in IsPrefixStartingOnTokenBoundary. WDYT? https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:128: const std::string tokens(kPrefixMatchTockens); Since kPrefixMatchTockens is array only with 1 element, my feeling that it's better to keep things simple and just make const char not const char[], that would allow avoid creating a string here. Or you can even avoid creating a constant by directly checking full_string[prefix.size()] == '@'. If we ever decide to expand on another tokens, const array could be easily added. https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_util.h:60: // end on a token. prefix end -> prefix ends https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_util.h:61: bool IsPrefixStartingOnTokenBoundary(const base::string16& full_string, As far as I understand |prefix| should end at Token, so probably name something like IsPrefixEndingOnTokenBoundary is more correct.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2906383003/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:277: current_username, potential_suggestion, true)); On 2017/06/01 12:15:56, dvadym wrote: > As far as I understand the main use case is emails, so it looks that we can > compare Case Insensitive. And probably even remove |case_sensitive| argument in > IsPrefixStartingOnTokenBoundary. WDYT? My motivation was that since currently gmail suffers from this bug and it teats Example@gmail.com and example@gmail.com as one account, we can do the same. But either way is fine by me. https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_util.h:60: // end on a token. On 2017/06/01 12:15:56, dvadym wrote: > prefix end -> prefix ends Done. https://codereview.chromium.org/2906383003/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_util.h:61: bool IsPrefixStartingOnTokenBoundary(const base::string16& full_string, On 2017/06/01 12:15:56, dvadym wrote: > As far as I understand |prefix| should end at Token, so probably name something > like IsPrefixEndingOnTokenBoundary is more correct. Done.
melandory@chromium.org changed reviewers: + vabr@chromium.org
vabr@chromium.org: Please review changes in this CL.
Hi Tanja, First, could you please expand on the CL title and description? Please make the title identify roughly the code area, something like: "Teach PasswordAutofillAgent sometimes match prefixes of usernames", then expand the description on: * What issue this is solving, and * how it is solved. In general, the CL goes in the right direction, I'd just like to clean-up some unnecessary renaming as pointed out below, as well as some minor details. Cheers, Vaclav https://codereview.chromium.org/2906383003/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:277: current_username, potential_suggestion, true)); On 2017/06/12 14:56:12, melandory wrote: > On 2017/06/01 12:15:56, dvadym wrote: > > As far as I understand the main use case is emails, so it looks that we can > > compare Case Insensitive. And probably even remove |case_sensitive| argument > in > > IsPrefixStartingOnTokenBoundary. WDYT? > My motivation was that since currently gmail suffers from this bug and it teats > mailto:Example@gmail.com and mailto:example@gmail.com as one account, we can do the same. But > either way is fine by me. Currently case sensitive equality is required. Let's keep using case sensitive comparison also for the prefix, as for not to introduce two changes with one CL. If we find out case sensitivity is an issue, we can change that in a separate CL. https://codereview.chromium.org/2906383003/diff/120001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2906383003/diff/120001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:723: AutocompletePasswordForReadonlyUsernamePrefixMatched) { Could you please also add a negative test? For example for username vs username + "suffix_without_@". The goal is not to test all options (that's done by the unittest), but to test that the negative path is executed at all. Furthermore, could you please also add a test that if there is no username field, and the saved credentials contain an empty username, then the password is filled properly? The part of code which you are changing handles this event as well (despite it is not related to prefix matching), and AFAICT, we don't have a test for that yet. https://codereview.chromium.org/2906383003/diff/120001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:728: // Filled even though username is not the preferred match. nit: "preferred match" is a sort of a term, used in PasswordFormManager to denote the last used credentials on a form. Here it seems more precise to say: "...though the username in the form is only a proper prefix of the stored username" https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:274: bool match = (potential_suggestion == current_username); optional nit: I am a bit concerned that we have two bool variables called *match, but their logical type is different (one is a directive about how strict matching to use, the other captures whether the two strings match). A little more concise way to write this is by pulling the match out of the branch, and it gets around the above issue by not naming |match|. if (potential_suggestion == current_username) return true; return !exact_match && IsPrefixEndingOnTokenBoundary(...) https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:376: void FindMatchesByUsername(const PasswordFormFillData& fill_data, nit: Please comment on what the function does and what is the meaning of the arguments. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:414: if (!username->empty() && !password->empty()) I know that this is just the old code moved around, but I think it is slightly wrong here. I would just drop the !username->empty() part -- the username could be empty. The pre-condition of the cycle implies that the password is empty, and whenever a username is assigned inside it, password is also. So even restricted to non-empty usernames, just checking !password->empty() should be equivalent. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:476: } else if (exact_username_match && current_username != username) { There is not code path to satisfy this condition: exact_username_match means that either username was set equal to current_username above, or the password stayed empty, in which case this function exited on line 456. Please drop the branch completely. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:567: // User the exact match for the editable username fields and allow prefix nit: User -> Use https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:569: // Note that this sets username to autofilled. nit: The latter sentence no longer makes sense ("this" referred to "Fill...", which was deleted). Also, I suspect that it is not true for read-only fields. My suggestion is to drop this sentence. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util.h:54: bool IsSubstringStartingOnTokenBoundary(const base::string16& suggestion, nit: I'm not sure what is the advantage of dropping "Field" from the name of this function. It generates unnecessary churn in the code and confuses per-line history with unrelated changes. Also the "field" from the name is reflected in the "field" part of |field_contents|, with the latter making less sense now. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util.h:58: // Currently, a token for the purposes of this method is defined as {'@'}. nit: Redefining "token" for this method and keeping the old definition for the methods above and below is confusing. Also, it is unlikely that more token separators will be added for this function. I propose replacing "TokenBoundary" with "AtSign". https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:25: struct TokenBoundaryPrefixCase { nit: Please move this just before its first use. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:31: class SubstringStartingOnTokenBoundaryTest nit: Similarly to the name of FieldIsSuggestionSubstringStartingOnTokenBoundary, also here I suggest not renaming tests not introduced for your change. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:108: TokenBoundaryPrefixCase{"", "ab", false}, Let's not duplicate the test cases :). (Also below.) https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:110: TokenBoundaryPrefixCase{"ab", "", false})); More cases to add: ab@c, ab@, false ab@cd@g, ab, false ab@cd@g, ab@cd, false abc, abc, false https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:126: << "full string = " << test_case.field_suggestion nit: Also here, please do not change the unrelated test. Independently of whether this makes the log more readable, let's not split the focus of this CL. I'm happy to approve a separate CL with these 2 lines, if you strongly want to make this change.
Description was changed from ========== Partly match stored username. BUG=543435 ========== to ========== Teach PasswordAutofillAgent sometimes match prefixes of usernames BUG=543435 ==========
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Teach PasswordAutofillAgent sometimes match prefixes of usernames BUG=543435 ========== to ========== Teach PasswordAutofillAgent sometimes match prefixes of usernames. This CL fixes the issue that is specific to email provider web sites in following scenario: 1. The user is able to log in with username "someuser". 2) Chrome stores the credentials with "someuser" as the username. 3) On a next visit to a web site the full username someuser@example.com is prefilled. 5) Chrome does not fill in the stored password, because its username "someuser" does not exactly match the content of the read-only field. The CL relaxes the matching condition in case the read-only username field and when the prefilled username looks like email. BUG=543435 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2906383003/diff/120001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2906383003/diff/120001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:723: AutocompletePasswordForReadonlyUsernamePrefixMatched) { On 2017/06/12 18:56:58, vabr (Chromium) wrote: > Could you please also add a negative test? For example for username vs username > + "suffix_without_@". > The goal is not to test all options (that's done by the unittest), but to test > that the negative path is executed at all. Done > > Furthermore, could you please also add a test that if there is no username > field, and the saved credentials contain an empty username, then the password is > filled properly? The part of code which you are changing handles this event as > well (despite it is not related to prefix matching), and AFAICT, we don't have a > test for that yet. https://codereview.chromium.org/2949533002 https://codereview.chromium.org/2906383003/diff/120001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:728: // Filled even though username is not the preferred match. On 2017/06/12 18:56:58, vabr (Chromium) wrote: > nit: "preferred match" is a sort of a term, used in PasswordFormManager to > denote the last used credentials on a form. Here it seems more precise to say: > "...though the username in the form is only a proper prefix of the stored > username" Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:274: bool match = (potential_suggestion == current_username); On 2017/06/12 18:56:58, vabr (Chromium) wrote: > optional nit: I am a bit concerned that we have two bool variables called > *match, but their logical type is different (one is a directive about how strict > matching to use, the other captures whether the two strings match). > > A little more concise way to write this is by pulling the match out of the > branch, and it gets around the above issue by not naming |match|. > > if (potential_suggestion == current_username) > return true; > return !exact_match && IsPrefixEndingOnTokenBoundary(...) Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:376: void FindMatchesByUsername(const PasswordFormFillData& fill_data, On 2017/06/12 18:56:58, vabr (Chromium) wrote: > nit: Please comment on what the function does and what is the meaning of the > arguments. Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:414: if (!username->empty() && !password->empty()) On 2017/06/12 18:56:58, vabr (Chromium) wrote: > I know that this is just the old code moved around, but I think it is slightly > wrong here. I would just drop the !username->empty() part -- the username could > be empty. > > The pre-condition of the cycle implies that the password is empty, and whenever > a username is assigned inside it, password is also. So even restricted to > non-empty usernames, just checking !password->empty() should be equivalent. Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:476: } else if (exact_username_match && current_username != username) { On 2017/06/12 18:56:58, vabr (Chromium) wrote: > There is not code path to satisfy this condition: exact_username_match means > that either username was set equal to current_username above, or the password > stayed empty, in which case this function exited on line 456. Please drop the > branch completely. Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:567: // User the exact match for the editable username fields and allow prefix On 2017/06/12 18:56:58, vabr (Chromium) wrote: > nit: User -> Use Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:569: // Note that this sets username to autofilled. On 2017/06/12 18:56:58, vabr (Chromium) wrote: > nit: The latter sentence no longer makes sense ("this" referred to "Fill...", > which was deleted). Also, I suspect that it is not true for read-only fields. My > suggestion is to drop this sentence. Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util.h:54: bool IsSubstringStartingOnTokenBoundary(const base::string16& suggestion, On 2017/06/12 18:56:58, vabr (Chromium) wrote: > nit: I'm not sure what is the advantage of dropping "Field" from the name of > this function. It generates unnecessary churn in the code and confuses per-line > history with unrelated changes. Also the "field" from the name is reflected in > the "field" part of |field_contents|, with the latter making less sense now. I brought old name back https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util.h:58: // Currently, a token for the purposes of this method is defined as {'@'}. On 2017/06/12 18:56:59, vabr (Chromium) wrote: > nit: Redefining "token" for this method and keeping the old definition for the > methods above and below is confusing. Also, it is unlikely that more token > separators will be added for this function. I propose replacing "TokenBoundary" > with "AtSign". Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:25: struct TokenBoundaryPrefixCase { On 2017/06/12 18:56:59, vabr (Chromium) wrote: > nit: Please move this just before its first use. Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:31: class SubstringStartingOnTokenBoundaryTest On 2017/06/12 18:56:59, vabr (Chromium) wrote: > nit: Similarly to the name of FieldIsSuggestionSubstringStartingOnTokenBoundary, > also here I suggest not renaming tests not introduced for your change. Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:108: TokenBoundaryPrefixCase{"", "ab", false}, On 2017/06/12 18:56:59, vabr (Chromium) wrote: > Let's not duplicate the test cases :). > (Also below.) Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:110: TokenBoundaryPrefixCase{"ab", "", false})); On 2017/06/12 18:56:59, vabr (Chromium) wrote: > More cases to add: > ab@c, ab@, false > ab@cd@g, ab, false > ab@cd@g, ab@cd, false > abc, abc, false Done. https://codereview.chromium.org/2906383003/diff/120001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:126: << "full string = " << test_case.field_suggestion On 2017/06/12 18:56:59, vabr (Chromium) wrote: > nit: Also here, please do not change the unrelated test. Independently of > whether this makes the log more readable, let's not split the focus of this CL. > I'm happy to approve a separate CL with these 2 lines, if you strongly want to > make this change. Done.
Thanks, Tanja! Almost there, just a discussion about the e-mail-related restrictions + a handful of nits. Cheers, Vaclav https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:377: // |current_username| by scanning |fill_data|. the result is written in nit: the -> The https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.cc:122: // weak check if |full_string| is a valid email address. nit: weak -> A weak Or maybe better "An incomplete check". https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.cc:123: bool is_email = Well, actually, hinting at e-mail, but only doing it just a little is confusing. The original suggestion https://crbug.com/543435#c10 from the security team does only request that instead of testing that the suggestion is a prefix of field contents, we test that suggestion + "@" is a prefix of the field contents. So I suggest replacing the whole function with just that: base::StartsWith(full_string, prefix + "@", base::CompareCase::SENSITIVE)); with the following reasons: * It is secure enough: this still respects origins and follows the security team suggestion; also other incomplete e-mail address checks are not clearly better. * Code will be significantly simpler. * We might still tune this based on user feedback (I'm looking especially at the CompareCase::SENSITIVE), so let's start simple. https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.h:59: // Currently, a token for the purposes of this method is defined as {'@'}. nit: Please update the comment not to mention any tokens, just inline '@'. https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.h:62: bool IsPrefixOfEmailEndingAtSign(const base::string16& full_string, nit: EndingAtSign -> EndingWithAtSign (The current name has an ambiguous use of "at", let's make clear "At" is meant as the name of the sign.) https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:73: struct TokenBoundaryPrefixCase { nit: TokenBoundary -> AtSign ? Let's get rid of mentioning token in order not to confuse it with the more general notion of token used by the neighbouring functions and tests.
https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:377: // |current_username| by scanning |fill_data|. the result is written in On 2017/06/19 12:54:15, vabr (Chromium) wrote: > nit: the -> The Done. https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.cc:122: // weak check if |full_string| is a valid email address. On 2017/06/19 12:54:15, vabr (Chromium) wrote: > nit: weak -> A weak > Or maybe better "An incomplete check". Done. https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.cc:123: bool is_email = On 2017/06/19 12:54:15, vabr (Chromium) wrote: > Well, actually, hinting at e-mail, but only doing it just a little is confusing. > The original suggestion https://crbug.com/543435#c10 from the security team does > only request that instead of testing that the suggestion is a prefix of field > contents, we test that suggestion + "@" is a prefix of the field contents. > > So I suggest replacing the whole function with just that: > > base::StartsWith(full_string, prefix + "@", base::CompareCase::SENSITIVE)); > > with the following reasons: > > * It is secure enough: this still respects origins and follows the security team > suggestion; also other incomplete e-mail address checks are not clearly better. > * Code will be significantly simpler. > * We might still tune this based on user feedback (I'm looking especially at the > CompareCase::SENSITIVE), so let's start simple. In this case "ab@cd@g", "ab@cd" will result in match. Is it fine? https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.h:62: bool IsPrefixOfEmailEndingAtSign(const base::string16& full_string, On 2017/06/19 12:54:15, vabr (Chromium) wrote: > nit: EndingAtSign -> EndingWithAtSign > > (The current name has an ambiguous use of "at", let's make clear "At" is meant > as the name of the sign.) Done. https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:73: struct TokenBoundaryPrefixCase { On 2017/06/19 12:54:15, vabr (Chromium) wrote: > nit: TokenBoundary -> AtSign ? > Let's get rid of mentioning token in order not to confuse it with the more > general notion of token used by the neighbouring functions and tests. Done.
Thanks, Tanja. The current code LGTM, but I found one edge-case I missed before (see below). If you could address that in this CL, that would be great, otherwise let's create a bug for that (feel free to assign to me in case you are unlikely to get to it by tomorrow). Cheers, Vaclav https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2906383003/diff/240001/components/autofill/co... components/autofill/core/common/autofill_util.cc:123: bool is_email = On 2017/06/21 12:00:42, melandory wrote: > On 2017/06/19 12:54:15, vabr (Chromium) wrote: > > Well, actually, hinting at e-mail, but only doing it just a little is > confusing. > > The original suggestion https://crbug.com/543435#c10 from the security team > does > > only request that instead of testing that the suggestion is a prefix of field > > contents, we test that suggestion + "@" is a prefix of the field contents. > > > > So I suggest replacing the whole function with just that: > > > > base::StartsWith(full_string, prefix + "@", base::CompareCase::SENSITIVE)); > > > > with the following reasons: > > > > * It is secure enough: this still respects origins and follows the security > team > > suggestion; also other incomplete e-mail address checks are not clearly > better. > > * Code will be significantly simpler. > > * We might still tune this based on user feedback (I'm looking especially at > the > > CompareCase::SENSITIVE), so let's start simple. > > In this case "ab@cd@g", "ab@cd" will result in match. Is it fine? I think it is fine. From usability perspective I don't see any issues, and for security, this follows the guidance from security team given on the bug. https://codereview.chromium.org/2906383003/diff/260001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2906383003/diff/260001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:455: FindMatchesByUsername(fill_data, current_username, exact_username_match, There is still one issue: In the rare case the user saved two credentials: username / password1 username@example.com / password2 it may now happen that Chrome autofills password2 for username, creating an incompatible mix. We need to ensure that the exact match is always preferred. So instead of just one pass trying to find everything, prefix- or exactly matching, we should first check all sources for exact match, and only after that for the prefix match. The easiest way to do this is to call FindMatchesByUsername here with the third parameter as true in any case, and then call it once more with false if exact_username_match is false. We should also add a test for the scenario I described above. https://codereview.chromium.org/2906383003/diff/260001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2906383003/diff/260001/components/autofill/co... components/autofill/core/common/autofill_util.cc:120: return base::StartsWith(full_string, prefix + base::UTF8ToUTF16(""), "" -> "@"
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by melandory@chromium.org
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/2906383003/#ps280001 (title: ".")
The CQ bit was unchecked by melandory@chromium.org
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1498145962300460, "parent_rev": "4b4ec5c8ec1ef6e2d76b116aae230ddec9b6451d", "commit_rev": "d2290e01a65d77cc3b21bbfe67295938f97f5e3e"}
Message was sent while issue was closed.
Description was changed from ========== Teach PasswordAutofillAgent sometimes match prefixes of usernames. This CL fixes the issue that is specific to email provider web sites in following scenario: 1. The user is able to log in with username "someuser". 2) Chrome stores the credentials with "someuser" as the username. 3) On a next visit to a web site the full username someuser@example.com is prefilled. 5) Chrome does not fill in the stored password, because its username "someuser" does not exactly match the content of the read-only field. The CL relaxes the matching condition in case the read-only username field and when the prefilled username looks like email. BUG=543435 ========== to ========== Teach PasswordAutofillAgent sometimes match prefixes of usernames. This CL fixes the issue that is specific to email provider web sites in following scenario: 1. The user is able to log in with username "someuser". 2) Chrome stores the credentials with "someuser" as the username. 3) On a next visit to a web site the full username someuser@example.com is prefilled. 5) Chrome does not fill in the stored password, because its username "someuser" does not exactly match the content of the read-only field. The CL relaxes the matching condition in case the read-only username field and when the prefilled username looks like email. BUG=543435 Review-Url: https://codereview.chromium.org/2906383003 Cr-Commit-Position: refs/heads/master@{#481569} Committed: https://chromium.googlesource.com/chromium/src/+/d2290e01a65d77cc3b21bbfe6729... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as https://chromium.googlesource.com/chromium/src/+/d2290e01a65d77cc3b21bbfe6729... |