Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652243002:
Implement Federated PSL Matches in Native Backends (Closed)
DescriptionImplement Federated PSL Matches in Native Backends
In a follow-up to http://crrev.com/2634163002 this change implements the feature
to fetch federated PSL matches in the native backends as well.
BUG=666340
R=vasilii@chromium.org
Review-Url: https://codereview.chromium.org/2652243002
Cr-Commit-Position: refs/heads/master@{#446634}
Committed: https://chromium.googlesource.com/chromium/src/+/bb90def05df40c3aebf65c5a65c6376baf779f61
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix Code for Stupid Compilers. #Patch Set 3 : Naming things is hard. #
Total comments: 32
Patch Set 4 : Addressed comments. #
Total comments: 4
Messages
Total messages: 28 (19 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...
Hi Vasilii, please review. https://codereview.chromium.org/2652243002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2652243002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1086: std::string regexp = "^(" + scheme + ":\\/\\/)([\\w-]+\\.)*" + Slightly offtopic: Should we make this less restrictive and allow other characters in subdomains as well? Currently we would not include subdomains containing the following characters: ~:/?#[]@!$&'()*,;= We are filtering later anyway, so there should be no risk in matching more URLs here. https://codereview.chromium.org/2652243002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1095: "^federation://([\\w-]+\\.)*" + registered_domain + "/.+$"); Same as above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2652243002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2652243002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1086: std::string regexp = "^(" + scheme + ":\\/\\/)([\\w-]+\\.)*" + On 2017/01/25 13:54:19, jdoerrie wrote: > Slightly offtopic: Should we make this less restrictive and allow other > characters in subdomains as well? Currently we would not include subdomains > containing the following characters: ~:/?#[]@!$&'()*,;= > > We are filtering later anyway, so there should be no risk in matching more URLs > here. If it means making the regex more difficult to read then the answer is not now :-) What we should double check is that non-latin domains are supported fine (e.g. https://xn--f1ae4a2b.xn--p1ai/ in Punycode). But I just did it. https://codereview.chromium.org/2652243002/diff/40001/chrome/browser/password... File chrome/browser/password_manager/native_backend_gnome_x.cc (right): https://codereview.chromium.org/2652243002/diff/40001/chrome/browser/password... chrome/browser/password_manager/native_backend_gnome_x.cc:146: case MatchResult::PSL_MATCH: |allow_psl_match| isn't used anymore. Deliberately? https://codereview.chromium.org/2652243002/diff/40001/components/password_man... File components/password_manager/core/browser/psl_matching_helper.cc (right): https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper.cc:15: #include "url/origin.h" unused. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper.cc:46: form.scheme != PasswordForm::SCHEME_HTML) You should add a comment that PSL and federations are applicable to HTML forms only. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... File components/password_manager/core/browser/psl_matching_helper_unittest.cc (right): https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:28: {"http://facebook.com", "", autofill::PasswordForm::SCHEME_HTML, I think signon_realm has a trailing '/' https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:31: {"http://facebook.com/path", "", autofill::PasswordForm::SCHEME_HTML, I don't think that this is a valid input for signon_realm. See password_specifics.proto https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:49: "http://m.facebook.com/path", "", MatchResult::PSL_MATCH}, As above it looks like an origin. The realm is without path. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:80: {"federation://example.com/google.com", "", Did you want to test the wrong input here? https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:85: "https://facebook.com", MatchResult::NO_MATCH}, Unsure about two things: - how is it related to federations - Why is it no match? Basically in all your examples you should specify both digest_signon_realm and digest_origin https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:88: MatchResult::NO_MATCH}, Doesn't fit into the federation bucket. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:96: MatchResult::FEDERATED_MATCH}, That's actually shouldn't work. The federated credentials are saved only for https and shouldn't match http. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:111: autofill::PasswordForm::SCHEME_HTML, "", "http://sub.example.com", https://sub.example.com https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:120: autofill::PasswordForm::SCHEME_HTML, "http://example.com", Here and below: signon_realm and origin should match each other. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:131: // Federated PSL matches do not apply to Google. Would be useful to have this test for normal PSLs too. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:256: {"federation://example.com/google.com", "example.com", false}, Copy/paste from 246? https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:258: {"federation://example.com/", "http://example.com/", false}, 247? https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:259: {"federation://example.com/google.com", "example", false}, SAme as 248?
Thanks for the thorough review! Could you have another look? https://codereview.chromium.org/2652243002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2652243002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:1086: std::string regexp = "^(" + scheme + ":\\/\\/)([\\w-]+\\.)*" + On 2017/01/26 13:35:17, vasilii wrote: > On 2017/01/25 13:54:19, jdoerrie wrote: > > Slightly offtopic: Should we make this less restrictive and allow other > > characters in subdomains as well? Currently we would not include subdomains > > containing the following characters: ~:/?#[]@!$&'()*,;= > > > > We are filtering later anyway, so there should be no risk in matching more > URLs > > here. > > If it means making the regex more difficult to read then the answer is not now > :-) > What we should double check is that non-latin domains are supported fine (e.g. > https://xn--f1ae4a2b.xn--p1ai/ in Punycode). But I just did it. Alright. It probably would actually simplify the regex, but there is no reason to change it, if this is working properly. https://codereview.chromium.org/2652243002/diff/40001/chrome/browser/password... File chrome/browser/password_manager/native_backend_gnome_x.cc (right): https://codereview.chromium.org/2652243002/diff/40001/chrome/browser/password... chrome/browser/password_manager/native_backend_gnome_x.cc:146: case MatchResult::PSL_MATCH: On 2017/01/26 13:35:17, vasilii wrote: > |allow_psl_match| isn't used anymore. Deliberately? Yeah, it's now part of |GetMatchResult|. It's still used when recording the UMA histogram (line 169), moved it down there. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... File components/password_manager/core/browser/psl_matching_helper.cc (right): https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper.cc:15: #include "url/origin.h" On 2017/01/26 13:35:17, vasilii wrote: > unused. Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper.cc:46: form.scheme != PasswordForm::SCHEME_HTML) On 2017/01/26 13:35:17, vasilii wrote: > You should add a comment that PSL and federations are applicable to HTML forms > only. Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... File components/password_manager/core/browser/psl_matching_helper_unittest.cc (right): https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:28: {"http://facebook.com", "", autofill::PasswordForm::SCHEME_HTML, On 2017/01/26 13:35:18, vasilii wrote: > I think signon_realm has a trailing '/' Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:31: {"http://facebook.com/path", "", autofill::PasswordForm::SCHEME_HTML, On 2017/01/26 13:35:17, vasilii wrote: > I don't think that this is a valid input for signon_realm. See > password_specifics.proto Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:49: "http://m.facebook.com/path", "", MatchResult::PSL_MATCH}, On 2017/01/26 13:35:18, vasilii wrote: > As above it looks like an origin. The realm is without path. Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:80: {"federation://example.com/google.com", "", On 2017/01/26 13:35:18, vasilii wrote: > Did you want to test the wrong input here? Yes, made it hopefully more clear now. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:85: "https://facebook.com", MatchResult::NO_MATCH}, On 2017/01/26 13:35:18, vasilii wrote: > Unsure about two things: > - how is it related to federations > - Why is it no match? > > Basically in all your examples you should specify both digest_signon_realm and > digest_origin Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:88: MatchResult::NO_MATCH}, On 2017/01/26 13:35:18, vasilii wrote: > Doesn't fit into the federation bucket. Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:96: MatchResult::FEDERATED_MATCH}, On 2017/01/26 13:35:17, vasilii wrote: > That's actually shouldn't work. The federated credentials are saved only for > https and shouldn't match http. Discussed offline, currently they do match. Will follow up in another CL. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:111: autofill::PasswordForm::SCHEME_HTML, "", "http://sub.example.com", On 2017/01/26 13:35:18, vasilii wrote: > https://sub.example.com Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:120: autofill::PasswordForm::SCHEME_HTML, "http://example.com", On 2017/01/26 13:35:18, vasilii wrote: > Here and below: signon_realm and origin should match each other. Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:131: // Federated PSL matches do not apply to Google. On 2017/01/26 13:35:18, vasilii wrote: > Would be useful to have this test for normal PSLs too. Done in line 45 already. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:256: {"federation://example.com/google.com", "example.com", false}, On 2017/01/26 13:35:18, vasilii wrote: > Copy/paste from 246? Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:258: {"federation://example.com/", "http://example.com/", false}, On 2017/01/26 13:35:18, vasilii wrote: > 247? Done. https://codereview.chromium.org/2652243002/diff/40001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:259: {"federation://example.com/google.com", "example", false}, On 2017/01/26 13:35:18, vasilii wrote: > SAme as 248? Done.
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...
lgtm https://codereview.chromium.org/2652243002/diff/60001/components/password_man... File components/password_manager/core/browser/psl_matching_helper_unittest.cc (right): https://codereview.chromium.org/2652243002/diff/60001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:108: "example.com", MatchResult::NO_MATCH}, Is it invalid?
https://codereview.chromium.org/2652243002/diff/60001/components/password_man... File components/password_manager/core/browser/psl_matching_helper_unittest.cc (right): https://codereview.chromium.org/2652243002/diff/60001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:108: "example.com", MatchResult::NO_MATCH}, On 2017/01/26 15:29:33, vasilii wrote: > Is it invalid? Yes, it's supposed to be invalid, the origin does not include a scheme.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2652243002/diff/60001/components/password_man... File components/password_manager/core/browser/psl_matching_helper_unittest.cc (right): https://codereview.chromium.org/2652243002/diff/60001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:108: "example.com", MatchResult::NO_MATCH}, On 2017/01/26 16:21:48, jdoerrie wrote: > On 2017/01/26 15:29:33, vasilii wrote: > > Is it invalid? > > Yes, it's supposed to be invalid, the origin does not include a scheme. Can you check what GURL says if it's constructed from that string?
https://codereview.chromium.org/2652243002/diff/60001/components/password_man... File components/password_manager/core/browser/psl_matching_helper_unittest.cc (right): https://codereview.chromium.org/2652243002/diff/60001/components/password_man... components/password_manager/core/browser/psl_matching_helper_unittest.cc:108: "example.com", MatchResult::NO_MATCH}, On 2017/01/26 17:03:56, vasilii wrote: > On 2017/01/26 16:21:48, jdoerrie wrote: > > On 2017/01/26 15:29:33, vasilii wrote: > > > Is it invalid? > > > > Yes, it's supposed to be invalid, the origin does not include a scheme. > > Can you check what GURL says if it's constructed from that string? The API does not directly document this, but parsing a string without a scheme results in an invalid GURL with empty spec.
The CQ bit was checked by jdoerrie@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": 60001, "attempt_start_ts": 1485512346944300,
"parent_rev": "588a513a3772d7cc466568f9007780aee8251003", "commit_rev":
"bb90def05df40c3aebf65c5a65c6376baf779f61"}
Message was sent while issue was closed.
Description was changed from ========== Implement Federated PSL Matches in Native Backends In a follow-up to http://crrev.com/2634163002 this change implements the feature to fetch federated PSL matches in the native backends as well. BUG=666340 R=vasilii@chromium.org ========== to ========== Implement Federated PSL Matches in Native Backends In a follow-up to http://crrev.com/2634163002 this change implements the feature to fetch federated PSL matches in the native backends as well. BUG=666340 R=vasilii@chromium.org Review-Url: https://codereview.chromium.org/2652243002 Cr-Commit-Position: refs/heads/master@{#446634} Committed: https://chromium.googlesource.com/chromium/src/+/bb90def05df40c3aebf65c5a65c6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bb90def05df40c3aebf65c5a65c6... |
