Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1272613002:
Remove credentials with empty username (Closed)
|
Created:
5 years, 4 months ago by dvadym Modified:
5 years, 4 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIf we have credentials with the empty username and the user uses them for sign-in with non-empty username then delete no-username credentials. These no-username credentials most probably were stored on change password or reset forms, and we don't need them anymore.
BUG=359315
Committed: https://crrev.com/a733022db8b61afa96ab2579bbc4c4fb81506d48
Cr-Commit-Position: refs/heads/master@{#342360}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed reviewer's comments #Patch Set 3 : Tests are implemented #
Total comments: 16
Patch Set 4 : Tests imrovement #
Total comments: 6
Patch Set 5 : Rebase #Patch Set 6 : Addressed comments #Patch Set 7 : Implemented checker #
Messages
Total messages: 17 (3 generated)
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Could you please review these changes? Probably you will have some comments about exact conditions when to remove, so I didn't make tests yet. I'll make tests when we clarify all details. Best regards, Vadym
Thanks, Vadym. This looks good, looking forward to the tests. Cheers, Vaclav https://codereview.chromium.org/1272613002/diff/1/components/password_manager... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.cc:976: if (!best_matches_.empty()) { nit: To decrease indentation, do an early return instead: if (empty) return; ... https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.cc:976: if (!best_matches_.empty()) { Another early return condition is that |pending_credentials_.username_value| should not be empty. Otherwise we will remove that particular credential. https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.cc:987: password_store->RemoveLogin(*form); |form| should also be deleted from |best_matches_| and |preferred_match_|. https://codereview.chromium.org/1272613002/diff/1/components/password_manager... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.h:358: // Delete each form from |best_matches_| that is not PSL-matched, has an empty This sentence makes it unclear from where the form is deleted (is "from |best_matches_|" indicating the origin of the form, or the place of deletion?). Therefore I suggest saying something like: If |pending_credentials_.username_value| is not empty, iterates over all forms from |best_matches_| and deletes from |best_matches_| and from the password store all which are not PSL-matched, have an empty username, and a password equal to |pending_credentials_.password_value|. (Note that I assumed that my other comments were addressed, so the comment does not reflect what the method does now.)
Thanks Vaclav! I've addressed your comments in PatchSet 2. https://codereview.chromium.org/1272613002/diff/1/components/password_manager... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.cc:976: if (!best_matches_.empty()) { On 2015/08/05 12:48:36, vabr (Chromium) wrote: > Another early return condition is that |pending_credentials_.username_value| > should not be empty. Otherwise we will remove that particular credential. Done. https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.cc:976: if (!best_matches_.empty()) { On 2015/08/05 12:48:36, vabr (Chromium) wrote: > nit: To decrease indentation, do an early return instead: > if (empty) > return; > ... Done. https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.cc:987: password_store->RemoveLogin(*form); On 2015/08/05 12:48:36, vabr (Chromium) wrote: > |form| should also be deleted from |best_matches_| and |preferred_match_|. After offline discussion we decide not to remove from |best_matches_| and |preferred_match_| https://codereview.chromium.org/1272613002/diff/1/components/password_manager... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/1272613002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.h:358: // Delete each form from |best_matches_| that is not PSL-matched, has an empty On 2015/08/05 12:48:36, vabr (Chromium) wrote: > This sentence makes it unclear from where the form is deleted (is "from > |best_matches_|" indicating the origin of the form, or the place of deletion?). > Therefore I suggest saying something like: > > If |pending_credentials_.username_value| is not empty, iterates over all forms > from |best_matches_| and deletes from |best_matches_| and from the password > store all which are not PSL-matched, have an empty username, and a password > equal to |pending_credentials_.password_value|. > > (Note that I assumed that my other comments were addressed, so the comment does > not reflect what the method does now.) Done.
Thanks, looks good so far. Now the tests. :) Cheers, Vaclav
Hi Vaclav, I've uploaded tests for these changes, PTAL.
Thanks, Vadym. LGTM, with some comments. Cheers, Vaclav https://codereview.chromium.org/1272613002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1684: scoped_refptr<TestPasswordStore> password_store = new TestPasswordStore; I would prefer the mock store (see http://crbug.com/517457), but as the test store is already in use in this file, I won't force it on you. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1692: saved_match()->username_value = ASCIIToUTF16(""); nit: Use .clear(). (Also below.) https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1694: manager.FetchMatchingLoginsFromPasswordStore(PasswordStore::ALLOW_PROMPT); I suggest DISALLOW_PROMPT just to be sure that the unittests don't pop-up any blocking UI anywhere. (I cannot see how they would, but let's be safe.) Also below. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1744: EXPECT_EQ(ASCIIToUTF16(""), nit: base:string16() instead of the converted "" (But I endorse the EXPECT_EQ instead of just testing .empty(), as EXPECT_EQ prints the string if it is non-empty.) https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1761: password_store->AddLogin(*saved_match()); Could you please ASSERT_FALSE(saved_match()->username_value.empty()); before this line? This is mainly to make reading the test easier by highlighting that the saved credential has a username. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1799: submitted_form.password_value = ASCIIToUTF16("pw"); Could you please ASSERT_NE(saved_match()->password_value, submitted_form.password_value); ? Alternatively, you can create the submitted password by appending "pw" to the saved_match password. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1813: } // namespace password_manager One more thing to test would be this scenario: 1. Add ("username", "password"). 2. Add ("", "password"). 3. Update ("username", "password2"). 4. Update ("", "password2"). As far as I understand, no stored password should be deleted in this process, right?
Thanks Vaclav! I've addressed your comments about tests. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1684: scoped_refptr<TestPasswordStore> password_store = new TestPasswordStore; On 2015/08/06 13:15:54, vabr (Chromium) wrote: > I would prefer the mock store (see http://crbug.com/517457), but as the test > store is already in use in this file, I won't force it on you. Thanks, I've rewritten with MockStore and it turned out much simpler then with TestPasswordStore. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1692: saved_match()->username_value = ASCIIToUTF16(""); On 2015/08/06 13:15:54, vabr (Chromium) wrote: > nit: Use .clear(). > > (Also below.) Done. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1694: manager.FetchMatchingLoginsFromPasswordStore(PasswordStore::ALLOW_PROMPT); On 2015/08/06 13:15:54, vabr (Chromium) wrote: > I suggest DISALLOW_PROMPT just to be sure that the unittests don't pop-up any > blocking UI anywhere. (I cannot see how they would, but let's be safe.) Also > below. Done. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1744: EXPECT_EQ(ASCIIToUTF16(""), On 2015/08/06 13:15:54, vabr (Chromium) wrote: > nit: base:string16() instead of the converted "" > > (But I endorse the EXPECT_EQ instead of just testing .empty(), as EXPECT_EQ > prints the string if it is non-empty.) Done. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1761: password_store->AddLogin(*saved_match()); On 2015/08/06 13:15:54, vabr (Chromium) wrote: > Could you please > ASSERT_FALSE(saved_match()->username_value.empty()); > before this line? > This is mainly to make reading the test easier by highlighting that the saved > credential has a username. Done. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1799: submitted_form.password_value = ASCIIToUTF16("pw"); On 2015/08/06 13:15:54, vabr (Chromium) wrote: > Could you please > ASSERT_NE(saved_match()->password_value, submitted_form.password_value); > ? > Alternatively, you can create the submitted password by appending "pw" to the > saved_match password. Done. https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1813: } // namespace password_manager I've divided this case on two tests: 1.Adding ("", "password") when ("username", "password") is already in store - it should be saved. 2.Updating ("username", "password") when both ("", "password") and ("username", "password") are present in store.
Thank you! LGTM with some nits. Vaclav https://codereview.chromium.org/1272613002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1684: scoped_refptr<TestPasswordStore> password_store = new TestPasswordStore; On 2015/08/07 12:44:04, dvadym wrote: > On 2015/08/06 13:15:54, vabr (Chromium) wrote: > > I would prefer the mock store (see http://crbug.com/517457), but as the test > > store is already in use in this file, I won't force it on you. > > Thanks, I've rewritten with MockStore and it turned out much simpler then with > TestPasswordStore. Good to know, thanks! https://codereview.chromium.org/1272613002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1813: } // namespace password_manager On 2015/08/07 12:44:04, dvadym wrote: > I've divided this case on two tests: > 1.Adding ("", "password") when ("username", "password") is already in store - > it should be saved. > 2.Updating ("username", "password") when both ("", "password") and > ("username", "password") are present in store. Great, thanks! https://codereview.chromium.org/1272613002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1703: EXPECT_CALL(*mock_store(), AddLogin(_)).Times(1); If you replace _ with submitted_form, does the test break? (Also below: whenever you expect a specific call, it is better to also specify the arguments, for more checking. The _ is useful for checking that no calls are seen.) https://codereview.chromium.org/1272613002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1703: EXPECT_CALL(*mock_store(), AddLogin(_)).Times(1); I believe Times(1) is the default, so you don't have to specify it (also below).
https://codereview.chromium.org/1272613002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1703: EXPECT_CALL(*mock_store(), AddLogin(_)).Times(1); On 2015/08/07 13:02:48, vabr (Chromium) wrote: > I believe Times(1) is the default, so you don't have to specify it (also below). Done. https://codereview.chromium.org/1272613002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1703: EXPECT_CALL(*mock_store(), AddLogin(_)).Times(1); On 2015/08/07 13:02:48, vabr (Chromium) wrote: > If you replace _ with submitted_form, does the test break? > > (Also below: whenever you expect a specific call, it is better to also specify > the arguments, for more checking. The _ is useful for checking that no calls are > seen.) yes, because form in AddLogin will have data_created updated. I think that the main reason of this test is to test removing, so it's not so important to check parameter of AddLogin. There are a lot of tests on adding.
Still LGTM. Vaclav https://codereview.chromium.org/1272613002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1703: EXPECT_CALL(*mock_store(), AddLogin(_)).Times(1); On 2015/08/07 13:51:59, dvadym wrote: > On 2015/08/07 13:02:48, vabr (Chromium) wrote: > > If you replace _ with submitted_form, does the test break? > > > > (Also below: whenever you expect a specific call, it is better to also specify > > the arguments, for more checking. The _ is useful for checking that no calls > are > > seen.) > > yes, because form in AddLogin will have data_created updated. I think that the > main reason of this test is to test removing, so it's not so important to check > parameter of AddLogin. There are a lot of tests on adding. As discussed offline, my suggestion is to use MATCHER_P2 to define a matcher for a given username of the form. Alternatively, you can save the form and EXPECT_EQ on its fields, or you can leave the test as is. I'm fine with all of these options. I agree that other tests are checking the invoking of AddLogin correctly, for this test my only concern was that we guard against saving the wrong one of the two used forms.
https://codereview.chromium.org/1272613002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/1272613002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:1703: EXPECT_CALL(*mock_store(), AddLogin(_)).Times(1); On 2015/08/07 14:00:40, vabr (Chromium) wrote: > On 2015/08/07 13:51:59, dvadym wrote: > > On 2015/08/07 13:02:48, vabr (Chromium) wrote: > > > If you replace _ with submitted_form, does the test break? > > > > > > (Also below: whenever you expect a specific call, it is better to also > specify > > > the arguments, for more checking. The _ is useful for checking that no calls > > are > > > seen.) > > > > yes, because form in AddLogin will have data_created updated. I think that the > > main reason of this test is to test removing, so it's not so important to > check > > parameter of AddLogin. There are a lot of tests on adding. > > As discussed offline, my suggestion is to use MATCHER_P2 to define a matcher for > a given username of the form. Alternatively, you can save the form and EXPECT_EQ > on its fields, or you can leave the test as is. I'm fine with all of these > options. > > I agree that other tests are checking the invoking of AddLogin correctly, for > this test my only concern was that we guard against saving the wrong one of the > two used forms. CheckUsername matcher is implemented
The CQ bit was checked by dvadym@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/1272613002/#ps120001 (title: "Implemented checker")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272613002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272613002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a733022db8b61afa96ab2579bbc4c4fb81506d48 Cr-Commit-Position: refs/heads/master@{#342360} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
