Fix retrieving federated credentials for the password store for localhost.

Fix retrieving federated credentials for the password store for localhost.

Before we used to ignore port for the federated matches which is a security problem. We also disregarded localhost federated credentials because they are HTTP.

BUG=710838
Review-Url: https://codereview.chromium.org/2816033002
Cr-Commit-Position: refs/heads/master@{#464474}
Committed: https://chromium.googlesource.com/chromium/src/+/66e4895ec848860ed812821d2fd48b5caf23981a

[vasilii, 2017-04-13 15:22:54]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[vasilii, 2017-04-13 15:23:21]

vasilii@chromium.org changed reviewers:
+ vabr@chromium.org

[vasilii, 2017-04-13 15:23:21]

Hi Vaclav,

please review.

[commit-bot: I haz the power, 2017-04-13 15:23:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-04-13 15:43:25]

Thanks, Vasilii!

This LGTM, although I suggested slight changes to the code structure.

Cheers,
Vaclav

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
File components/password_manager/core/browser/psl_matching_helper.cc (right):

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
components/password_manager/core/browser/psl_matching_helper.cc:120: // The
format should be "federation://origin.host/federation.host;
Would it make sense to avoid code duplication by returning
IsFederatedMatch(form_signon_realm, origin, origin)
instead?

Or even better by having the IsFederatedRealm as suggested in my other comment
below?

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
File components/password_manager/core/browser/test_password_store.cc (right):

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
components/password_manager/core/browser/test_password_store.cc:92: // Below we
test that |elements| may contain federated matches by passing
This is a hack. What about removing the origin equality check from
IsFederatedMatch and leaving just the part checking the realm format
(IsFederatedRealm?). You could call that here without explanation, and you could
also call that from within IsFederatedPSLMatch. The only other callsite of
IsFederatedMatch inside GetMatchResult is easily rewritten with the new
IsFederatedRealm and equality check of the origins.

[commit-bot: I haz the power, 2017-04-13 16:07:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 16:07:42]

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_...)

[vasilii, 2017-04-13 17:18:34]

I fixed comments and added some tests. I didn't like the idea to expose
IsFederatedRealm publicly, so I put it under defined(UNIT_TEST).

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
File components/password_manager/core/browser/psl_matching_helper.cc (right):

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
components/password_manager/core/browser/psl_matching_helper.cc:120: // The
format should be "federation://origin.host/federation.host;
On 2017/04/13 15:43:24, vabr (Chromium) wrote:
> Would it make sense to avoid code duplication by returning
> IsFederatedMatch(form_signon_realm, origin, origin)
> instead?
> 
> Or even better by having the IsFederatedRealm as suggested in my other comment
> below?

Done.

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
File components/password_manager/core/browser/test_password_store.cc (right):

https://codereview.chromium.org/2816033002/diff/1/components/password_manager...
components/password_manager/core/browser/test_password_store.cc:92: // Below we
test that |elements| may contain federated matches by passing
On 2017/04/13 15:43:24, vabr (Chromium) wrote:
> This is a hack. What about removing the origin equality check from
> IsFederatedMatch and leaving just the part checking the realm format
> (IsFederatedRealm?). You could call that here without explanation, and you
could
> also call that from within IsFederatedPSLMatch. The only other callsite of
> IsFederatedMatch inside GetMatchResult is easily rewritten with the new
> IsFederatedRealm and equality check of the origins.

Done.

[vasilii, 2017-04-13 17:22:40]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2017-04-13 17:22:40]

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/2816033002/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2017-04-13 17:23:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-04-13 18:12:09]

Thanks, Vasilii.

LGTM with some questions and nits below.

Cheers,
Vaclav

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:450: //
Match twice.
I'm not sure I understand "twice". Could you explain?

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:458: //
Match against the mobile site.
What mobile site are you referring to? localhost:8080 does not look particularly
mobile :).

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
File components/password_manager/core/browser/psl_matching_helper.h (right):

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/psl_matching_helper.h:36: #if
defined(UNIT_TEST)
nit: Do you think we should warn in the comment that while the public
declaration is only for tests, the function itself is used in production code?
My concern is about future modifications to the function which might happen
under the assumption that it's for tests only (the bar is lower for test code,
performance is not so important, etc.).

But maybe I'm just paranoid. :)

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
File components/password_manager/core/browser/test_password_store.cc (right):

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/test_password_store.cc:90: // no test
need it so far.
minor nit: "no test needs" or "no tests need"

[vasilii, 2017-04-13 18:17:00]

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:450: //
Match twice.
On 2017/04/13 18:12:09, vabr (Chromium) wrote:
> I'm not sure I understand "twice". Could you explain?

I call GetLogins twice.

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:458: //
Match against the mobile site.
On 2017/04/13 18:12:09, vabr (Chromium) wrote:
> What mobile site are you referring to? localhost:8080 does not look
particularly
> mobile :).

Copy/paste problem :)

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
File components/password_manager/core/browser/psl_matching_helper.h (right):

https://codereview.chromium.org/2816033002/diff/20001/components/password_man...
components/password_manager/core/browser/psl_matching_helper.h:36: #if
defined(UNIT_TEST)
On 2017/04/13 18:12:09, vabr (Chromium) wrote:
> nit: Do you think we should warn in the comment that while the public
> declaration is only for tests, the function itself is used in production code?
> My concern is about future modifications to the function which might happen
> under the assumption that it's for tests only (the bar is lower for test code,
> performance is not so important, etc.).
> 
> But maybe I'm just paranoid. :)

Hmm, but an editor of the cc file should notice that it's production one. I can
add a comment of course.

[commit-bot: I haz the power, 2017-04-13 18:21:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492104160219580,
"parent_rev": "d9848157615e65a8b8cf5e877916f97d2eb098ff", "commit_rev":
"66e4895ec848860ed812821d2fd48b5caf23981a"}

[commit-bot: I haz the power, 2017-04-13 18:22:31]

Description was changed from

==========
Fix retrieving federated credentials for the password store for localhost.

Before we used to ignore port for the federated matches which is a security
problem. We also disregarded localhost federated credentials because they are
HTTP.

BUG=710838
==========

to

==========
Fix retrieving federated credentials for the password store for localhost.

Before we used to ignore port for the federated matches which is a security
problem. We also disregarded localhost federated credentials because they are
HTTP.

BUG=710838

Review-Url: https://codereview.chromium.org/2816033002
Cr-Commit-Position: refs/heads/master@{#464474}
Committed:
https://chromium.googlesource.com/chromium/src/+/66e4895ec848860ed812821d2fd4...
==========

[commit-bot: I haz the power, 2017-04-13 18:22:32]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/66e4895ec848860ed812821d2fd4...