CREDENTIAL: Filter affiliated credentials.

CREDENTIAL: Filter affiliated credentials.

This patch adjusts our handling of affiliated matches so that we're doing
the same filtering there that we do for locally-stored credentials (e.g.
correctly excluding non-matching federations, and only including passwords
if requested).

BUG=589011
R=vabr@chromium.org

Committed: https://crrev.com/b97dbebc4f4b202b8278629e473642370f0dd224
Cr-Commit-Position: refs/heads/master@{#376966}

[Mike West, 2016-02-23 09:08:52]

Mind taking a look, vabr@?

[Mike West, 2016-02-23 09:08:56]

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

[commit-bot: I haz the power, 2016-02-23 09:09:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723583003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723583003/1

[vabr (Chromium), 2016-02-23 09:24:54]

LGTM with nits.

Thanks,
Vaclav

https://codereview.chromium.org/1723583003/diff/1/components/password_manager...
File
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1723583003/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:682:
store_->SetAffiliatedMatchHelper(make_scoped_ptr(mock_helper));
nit: Here and below, please either inline mock_helper, or make it a scoped_ptr
instead of a raw one and std::move it:

auto mock_helper = make_scoped_ptr(new MockAffiliatedMatchHelper);
store_->SetAffiliatedMatchHelper(std::move(mock_helper));

You can use store_->affiliated_match_helper() to get the weak pointer when
needed.

While it is unlikely that somebody would insert an early exit between lines 681
and 682, it's easier to just always play it safe than to reason about what could
happen or not.

https://codereview.chromium.org/1723583003/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:708:
dispatcher()->OnRequestCredential(kRequestId, true, false, federations);
nit: The "false" here is the only difference to the test above, and it causes
the different test outcome. We should have a comment about what the "false"
means (and the "true" on the same position on line 690).

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

https://codereview.chromium.org/1723583003/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:65:
if ((form->federation_url.is_empty() && include_passwords_) ||
optional nit: Would it make sense to pull-out the condition from lines 65-67 and
57-59?

if (!((form->federation_url.is_empty() && include_passwords_) ||
(!form->federation_url.is_empty() &&
federations_.count(form->federation_url.spec())))) {
  continue;
}
if (form->origin.GetOrigin() == origin_.GetOrigin()) {...}

[Mike West, 2016-02-23 09:52:49]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-02-23 09:52:49]

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/1723583003/#ps20001
(title: "vabr@")

[commit-bot: I haz the power, 2016-02-23 09:53:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723583003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723583003/20001

[commit-bot: I haz the power, 2016-02-23 10:45:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-23 10:45:55]

Description was changed from

==========
CREDENTIAL: Filter affiliated credentials.

This patch adjusts our handling of affiliated matches so that we're doing
the same filtering there that we do for locally-stored credentials (e.g.
correctly excluding non-matching federations, and only including passwords
if requested).

BUG=589011
R=vabr@chromium.org
==========

to

==========
CREDENTIAL: Filter affiliated credentials.

This patch adjusts our handling of affiliated matches so that we're doing
the same filtering there that we do for locally-stored credentials (e.g.
correctly excluding non-matching federations, and only including passwords
if requested).

BUG=589011
R=vabr@chromium.org

Committed: https://crrev.com/b97dbebc4f4b202b8278629e473642370f0dd224
Cr-Commit-Position: refs/heads/master@{#376966}
==========

[commit-bot: I haz the power, 2016-02-23 10:45:55]

Patchset 2 (id:??) landed as
https://crrev.com/b97dbebc4f4b202b8278629e473642370f0dd224
Cr-Commit-Position: refs/heads/master@{#376966}