Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2634163002:
Fetch federated PSL-matches from the password store. (Closed)
DescriptionFetch federated PSL-matches from the password store.
This change adds the possibility to apply PSL matches to federated logins. In
order to do so it adds a PSL matching helper method and modifies the SQL
statement used.
BUG=666340
R=vasilii@chromium.org
Review-Url: https://codereview.chromium.org/2634163002
Cr-Commit-Position: refs/heads/master@{#444689}
Committed: https://chromium.googlesource.com/chromium/src/+/e92e9a9d5c688c348a8260c6bcdea2214755014d
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed comments. #
Total comments: 2
Patch Set 3 : Addressed comments. #
Total comments: 2
Patch Set 4 : Updated Histogram. #
Total comments: 1
Messages
Total messages: 28 (17 generated)
The CQ bit was checked by jdoerrie@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...
Hey Vasilii, PTAL. This is the bare minimum that makes the test pass, but I probably should make the several backends also use |IsFederatedPSLMatch| and include tests for that. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1099: signon_realm.port() + ")?/.+$"; Couple questions here: - Do federated matches ever have a port? I believe so, but I am not quite sure. - Should the match for the path be more restrictive? Currently it matches anything that is non-empty (not counting '/'). https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1234: psl_domain_match_metric = PSL_DOMAIN_MATCH_FOUND; Should we consider adding a new value to |PSLDomainMatchMetric| for this federated PSL match case? https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/psl_matching_helper.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/psl_matching_helper.cc:77: return IsPublicSuffixDomainMatch(https_signon_realm.GetOrigin().spec(), Should we create an overload of |IsPublicSuffixDomainMatch| taking GURLs as argument? Currently the arguments are strings, but they are converted to GURLs immediately in the function body.
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...)
Added comment regarding test failure. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database_unittest.cc:416: EXPECT_THAT(result, testing::ElementsAre(Pointee(form))); This test fails now, since |form2| will also be returned as a federated PSL match. Adding |form2.is_public_suffix_match = true;| and replacing the line with |EXPECT_THAT(result, UnorderedElementsAre(Pointee(form), Pointee(form2)));| (see line 423) makes this test pass. With my change there is no way to just get strict federated matches, federated PSL matches will always be present. Is this desired?
https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1099: signon_realm.port() + ")?/.+$"; On 2017/01/16 18:04:14, jdoerrie wrote: > Couple questions here: > - Do federated matches ever have a port? I believe so, but I am not quite sure. > - Should the match for the path be more restrictive? Currently it matches > anything that is non-empty (not counting '/'). No, we don't save port. That is correct for the regexp. There is another code running later to filter the results on some conditions (see StatementToForms). https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1234: psl_domain_match_metric = PSL_DOMAIN_MATCH_FOUND; On 2017/01/16 18:04:14, jdoerrie wrote: > Should we consider adding a new value to |PSLDomainMatchMetric| for this > federated PSL match case? Sure. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1290: std::string federated_statement = Seems to be obsolete https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database_unittest.cc:416: EXPECT_THAT(result, testing::ElementsAre(Pointee(form))); On 2017/01/17 09:21:40, jdoerrie wrote: > This test fails now, since |form2| will also be returned as a federated PSL > match. Adding |form2.is_public_suffix_match = true;| and replacing the line with > |EXPECT_THAT(result, UnorderedElementsAre(Pointee(form), Pointee(form2)));| (see > line 423) makes this test pass. With my change there is no way to just get > strict federated matches, federated PSL matches will always be present. Is this > desired? Yes, you can fix the test the way you described. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/psl_matching_helper.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/psl_matching_helper.cc:77: return IsPublicSuffixDomainMatch(https_signon_realm.GetOrigin().spec(), On 2017/01/16 18:04:14, jdoerrie wrote: > Should we create an overload of |IsPublicSuffixDomainMatch| taking GURLs as > argument? Currently the arguments are strings, but they are converted to GURLs > immediately in the function body. Probably in a separate patch. It makes sense to make it public if there are other consumers.
The CQ bit was checked by jdoerrie@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...
https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1099: signon_realm.port() + ")?/.+$"; On 2017/01/17 12:41:24, vasilii wrote: > On 2017/01/16 18:04:14, jdoerrie wrote: > > Couple questions here: > > - Do federated matches ever have a port? I believe so, but I am not quite > sure. > > - Should the match for the path be more restrictive? Currently it matches > > anything that is non-empty (not counting '/'). > > No, we don't save port. > That is correct for the regexp. There is another code running later to filter > the results on some conditions (see StatementToForms). Acknowledged. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1290: std::string federated_statement = On 2017/01/17 12:41:24, vasilii wrote: > Seems to be obsolete Acknowledged. It's obsolete for matching federated PSL credentials, but still has value for federated matches without PSL. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/login_database_unittest.cc:416: EXPECT_THAT(result, testing::ElementsAre(Pointee(form))); On 2017/01/17 12:41:24, vasilii wrote: > On 2017/01/17 09:21:40, jdoerrie wrote: > > This test fails now, since |form2| will also be returned as a federated PSL > > match. Adding |form2.is_public_suffix_match = true;| and replacing the line > with > > |EXPECT_THAT(result, UnorderedElementsAre(Pointee(form), Pointee(form2)));| > (see > > line 423) makes this test pass. With my change there is no way to just get > > strict federated matches, federated PSL matches will always be present. Is > this > > desired? > > Yes, you can fix the test the way you described. Done. https://codereview.chromium.org/2634163002/diff/1/components/password_manager... File components/password_manager/core/browser/psl_matching_helper.cc (right): https://codereview.chromium.org/2634163002/diff/1/components/password_manager... components/password_manager/core/browser/psl_matching_helper.cc:77: return IsPublicSuffixDomainMatch(https_signon_realm.GetOrigin().spec(), On 2017/01/17 12:41:24, vasilii wrote: > On 2017/01/16 18:04:14, jdoerrie wrote: > > Should we create an overload of |IsPublicSuffixDomainMatch| taking GURLs as > > argument? Currently the arguments are strings, but they are converted to GURLs > > immediately in the function body. > > Probably in a separate patch. It makes sense to make it public if there are > other consumers. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2634163002/diff/20001/components/password_man... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2634163002/diff/20001/components/password_man... components/password_manager/core/browser/login_database.cc:1093: // matches the empty subdomain. Hence exact domain matches are also Optionally: move the code into the previous if (should_PSL_matching_apply). Then there is no need to clarify |registered_domain| escaping. https://codereview.chromium.org/2634163002/diff/20001/components/password_man... File components/password_manager/core/browser/psl_matching_helper.h (right): https://codereview.chromium.org/2634163002/diff/20001/components/password_man... components/password_manager/core/browser/psl_matching_helper.h:23: PSL_DOMAIN_MATCH_COUNT With this change you need to update PasswordManager.PslDomainMatchTriggering description in histograms.xml
The CQ bit was checked by jdoerrie@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...
jdoerrie@chromium.org changed reviewers: + isherman@chromium.org
isherman, please review histogram.xml.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2634163002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2634163002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:44585: + possible matches. Seperately counts PSL matches for federated entries. Sorry, I don't understand what this added sentence means. Do you mean that matches for federated entries are included in this histogram? Are excluded, and separately tracked by a different histogram? Something else? Please reword this to clarify.
Thanks for providing feedback, could you have another look? https://codereview.chromium.org/2634163002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2634163002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:44585: + possible matches. Seperately counts PSL matches for federated entries. On 2017/01/18 01:44:49, Ilya Sherman wrote: > Sorry, I don't understand what this added sentence means. Do you mean that > matches for federated entries are included in this histogram? Are excluded, and > separately tracked by a different histogram? Something else? Please reword > this to clarify. Done.
Metrics LGTM, thanks. https://codereview.chromium.org/2634163002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2634163002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:99841: + <int value="3" label="Federated match"/> Ah, I see, you are adding a bucket. This change makes more sense now =)
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2634163002/#ps60001 (title: "Updated Histogram.")
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": 60001, "attempt_start_ts": 1484814403691480,
"parent_rev": "e8a03e2020bf9aad333ae60eb57541e7eafb4f24", "commit_rev":
"e92e9a9d5c688c348a8260c6bcdea2214755014d"}
Message was sent while issue was closed.
Description was changed from ========== Fetch federated PSL-matches from the password store. This change adds the possibility to apply PSL matches to federated logins. In order to do so it adds a PSL matching helper method and modifies the SQL statement used. BUG=666340 R=vasilii@chromium.org ========== to ========== Fetch federated PSL-matches from the password store. This change adds the possibility to apply PSL matches to federated logins. In order to do so it adds a PSL matching helper method and modifies the SQL statement used. BUG=666340 R=vasilii@chromium.org Review-Url: https://codereview.chromium.org/2634163002 Cr-Commit-Position: refs/heads/master@{#444689} Committed: https://chromium.googlesource.com/chromium/src/+/e92e9a9d5c688c348a8260c6bcde... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e92e9a9d5c688c348a8260c6bcde... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
