Support Android username-only credentials.

Support Android username-only credentials.

They are collected implicitly and should be treated specially.
The local credentials without password are just ignored by both the form autofill and Credential Manager API. The federated credentials can be used by the API. However, they shouldn't be used for auto signin.

BUG=527073
Committed: https://crrev.com/ee98c3b98d1b34e8a16543ccbcdf69168f8af56b
Cr-Commit-Position: refs/heads/master@{#354021}

[vasilii, 2015-09-29 09:25:28]

vasilii@chromium.org changed reviewers:
+ engedy@chromium.org, mkwst@chromium.org, vabr@chromium.org

[vasilii, 2015-09-29 09:25:29]

Hi guys, please review the CL.

[Mike West, 2015-09-29 09:36:10]

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.h:57: SCHEME_HTML,
Nit: `= 0`? Or does `: int` imply that?
Nit: `unsigned` rather than `int`?

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
components/autofill/core/common/save_password_progress_logger.cc:186: return
"Android username only";
How about "username only", so we can be happy in a magical future world that
supports iOS.

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:142: for
(auto& form : android_credentials) {
Nit: Why change this to `auto`? It seemed clearer before.

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:144: if
(form->federation_url.is_empty())
I don't really understand this. Why do we have federated username only
credentials? Why don't we ignore them?

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliated_match_helper.h (right):

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.h:96: //
thread. Non-federated username-only credentials are filtered.
Why only "non-federated"?

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper_unittest.cc:595:
ASSERT_EQ(3u, transformed_forms.size());
Why isn't this 4? You're adding two distinct forms above, one federated, one
username only?

[engedy, 2015-10-08 17:31:01]

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:242: bool
IsDBOnlyForm(const autofill::PasswordForm& form) {
nit: IsLoginDatabaseOnlyForm

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.h:57: SCHEME_HTML,
On 2015/09/29 09:36:09, Mike West (slow) wrote:
> Nit: `= 0`? Or does `: int` imply that?

Whether the underlying type is fixed or not has nothing to do with the values
that get assigned to each enumerator. §7.2.2 in the standard states that: "If
the first enumerator has no initializer, the value of the corresponding constant
is zero.".
 
> Nit: `unsigned` rather than `int`?

I'm not sure if it makes sense to make this unsigned. The corresponding value in
the protobuf is a signed integer anyway.

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
components/autofill/core/common/save_password_progress_logger.cc:186: return
"Android username only";
On 2015/09/29 09:36:09, Mike West (slow) wrote:
> How about "username only", so we can be happy in a magical future world that
> supports iOS.

+1.

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:144: if
(form->federation_url.is_empty())
On 2015/09/29 09:36:09, Mike West (slow) wrote:
> I don't really understand this. Why do we have federated username only
> credentials? Why don't we ignore them?

Note that I have recently landed a fix for https://crbug.com/539833 which
disables returning federated logins as affiliated matches from
PasswordStore::GetLogins(). However, that does not affect the Credential Manager
API, as a different code path is used by the API.

If we want to include them here or in the Manage Passwords bubble, that will
need to be changed.

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper_unittest.cc:588:
username_only.federation_url = GURL(kTestFederationURL);
Please either set |skip_zero_click| to false explicitly here.

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper_unittest.cc:595:
ASSERT_EQ(3u, transformed_forms.size());
On 2015/09/29 09:36:09, Mike West (slow) wrote:
> Why isn't this 4? You're adding two distinct forms above, one federated, one
> username only?

If I understand correctly, the non-federated credential is ought to be removed.

[vabr (Chromium), 2015-10-09 07:53:09]

LGTM once the comments from all reviewers are addressed.

Cheers,
Vaclav

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:479: // We never merge
blacklist entries between our store and the Keychain,
nit: This comment is slightly out of date. And given that the name of the new
function is well-chosen, I don't think the comment is needed at all.

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:142: for
(auto& form : android_credentials) {
On 2015/09/29 09:36:09, Mike West (slow) wrote:
> Nit: Why change this to `auto`? It seemed clearer before.

Vasilii now needs a reference to that pointer to be able to null it in the end.
So it would have to be something like autofill::PasswordForm*& if that's a valid
syntax at all. To capture that this is a reference, "auto&" is better, because
the & in the end is easier to spot. But I'm indifferent on which way this ends
up being written, so will let mkwst and vasilii figure out what's best. :)

[Mike West, 2015-10-09 08:35:21]

On 2015/10/09 at 07:53:09, vabr wrote:
>
https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
> File components/password_manager/core/browser/affiliated_match_helper.cc
(right):
> 
>
https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
> components/password_manager/core/browser/affiliated_match_helper.cc:142: for
(auto& form : android_credentials) {
> On 2015/09/29 09:36:09, Mike West (slow) wrote:
> > Nit: Why change this to `auto`? It seemed clearer before.
> 
> Vasilii now needs a reference to that pointer to be able to null it in the
end. So it would have to be something like autofill::PasswordForm*& if that's a
valid syntax at all. To capture that this is a reference, "auto&" is better,
because the & in the end is easier to spot. But I'm indifferent on which way
this ends up being written, so will let mkwst and vasilii figure out what's
best. :)

Makes sense. I'm happy with this explanation.

[vasilii, 2015-10-12 15:12:48]

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:242: bool
IsDBOnlyForm(const autofill::PasswordForm& form) {
On 2015/10/08 17:31:00, engedy wrote:
> nit: IsLoginDatabaseOnlyForm

Done.

https://codereview.chromium.org/1375883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:479: // We never merge
blacklist entries between our store and the Keychain,
On 2015/10/09 07:53:09, vabr (Chromium) wrote:
> nit: This comment is slightly out of date. And given that the name of the new
> function is well-chosen, I don't think the comment is needed at all.

Done.

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.h:57: SCHEME_HTML,
On 2015/10/08 17:31:01, engedy wrote:
> On 2015/09/29 09:36:09, Mike West (slow) wrote:
> > Nit: `= 0`? Or does `: int` imply that?
> 
> Whether the underlying type is fixed or not has nothing to do with the values
> that get assigned to each enumerator. §7.2.2 in the standard states that: "If
> the first enumerator has no initializer, the value of the corresponding
constant
> is zero.".
>  
> > Nit: `unsigned` rather than `int`?
> 
> I'm not sure if it makes sense to make this unsigned. The corresponding value
in
> the protobuf is a signed integer anyway.

Acknowledged.

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1375883002/diff/1/components/autofill/core/co...
components/autofill/core/common/save_password_progress_logger.cc:186: return
"Android username only";
On 2015/10/08 17:31:01, engedy wrote:
> On 2015/09/29 09:36:09, Mike West (slow) wrote:
> > How about "username only", so we can be happy in a magical future world that
> > supports iOS.
> 
> +1.

Done.

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:142: for
(auto& form : android_credentials) {
On 2015/10/09 07:53:09, vabr (Chromium) wrote:
> On 2015/09/29 09:36:09, Mike West (slow) wrote:
> > Nit: Why change this to `auto`? It seemed clearer before.
> 
> Vasilii now needs a reference to that pointer to be able to null it in the
end.
> So it would have to be something like autofill::PasswordForm*& if that's a
valid
> syntax at all. To capture that this is a reference, "auto&" is better, because
> the & in the end is easier to spot. But I'm indifferent on which way this ends
> up being written, so will let mkwst and vasilii figure out what's best. :)

Acknowledged.

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:144: if
(form->federation_url.is_empty())
On 2015/10/08 17:31:01, engedy wrote:
> On 2015/09/29 09:36:09, Mike West (slow) wrote:
> > I don't really understand this. Why do we have federated username only
> > credentials? Why don't we ignore them?
> 
> Note that I have recently landed a fix for https://crbug.com/539833 which
> disables returning federated logins as affiliated matches from
> PasswordStore::GetLogins(). However, that does not affect the Credential
Manager
> API, as a different code path is used by the API.
> 
> If we want to include them here or in the Manage Passwords bubble, that will
> need to be changed.

We can ignore them but they still exist on the machine. Given that they may be
valuable for the user why not to show them in the account chooser (without
zero-click of course)?

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliated_match_helper.h (right):

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.h:96: //
thread. Non-federated username-only credentials are filtered.
On 2015/09/29 09:36:09, Mike West (slow) wrote:
> Why only "non-federated"?

Done.

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

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper_unittest.cc:588:
username_only.federation_url = GURL(kTestFederationURL);
On 2015/10/08 17:31:01, engedy wrote:
> Please either set |skip_zero_click| to false explicitly here.

Done.

https://codereview.chromium.org/1375883002/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper_unittest.cc:595:
ASSERT_EQ(3u, transformed_forms.size());
On 2015/10/08 17:31:01, engedy wrote:
> On 2015/09/29 09:36:09, Mike West (slow) wrote:
> > Why isn't this 4? You're adding two distinct forms above, one federated, one
> > username only?
> 
> If I understand correctly, the non-federated credential is ought to be
removed.

Acknowledged.

[engedy, 2015-10-13 12:11:33]

LGTM % comments.  I think the CL description says "without username" instead of
"without password". Could you double-check?

https://codereview.chromium.org/1375883002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1375883002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1190: if
(db_form->is_public_suffix_match)
We need to take |is_affiliation_based_match| into account here as well. Or
instead, check if |form.signon_realm != db_form.signon_realm|, so that it's more
robust.

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

https://codereview.chromium.org/1375883002/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:137: void
AffiliatedMatchHelper::TrimAndroidUsernameOnlyCredentials(
nit: Given that the SCHEME is no longer specific to Android, maybe we could
rename this to TrimUsernameOnlyCredentials, and move this out of
AffiliatedMatchHelper?

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

https://codereview.chromium.org/1375883002/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.h:96:
ScopedVector<autofill::PasswordForm>* android_credentials);
What is the reason for passing a pointer to the ScopedVector, as opposed to
taking and returning a ScopedVector by value? That solution would allow
functional-style chaining of filters.

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

https://codereview.chromium.org/1375883002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.cc:391:
AffiliatedMatchHelper::TrimAndroidUsernameOnlyCredentials(&more_results);
Could you please update test PasswordStoreTest.GetLoginsWithAffiliations with a
username-only entry to have coverage that these are removed?

[vasilii, 2015-10-13 18:05:26]

Mike?

https://codereview.chromium.org/1375883002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1375883002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1190: if
(db_form->is_public_suffix_match)
On 2015/10/13 12:11:33, engedy wrote:
> We need to take |is_affiliation_based_match| into account here as well. Or
> instead, check if |form.signon_realm != db_form.signon_realm|, so that it's
more
> robust.

That's unrelated to the current CL. Please follow up with Vadym about it.

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

https://codereview.chromium.org/1375883002/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:137: void
AffiliatedMatchHelper::TrimAndroidUsernameOnlyCredentials(
On 2015/10/13 12:11:33, engedy wrote:
> nit: Given that the SCHEME is no longer specific to Android, maybe we could
> rename this to TrimUsernameOnlyCredentials, and move this out of
> AffiliatedMatchHelper?

Done.

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

https://codereview.chromium.org/1375883002/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.h:96:
ScopedVector<autofill::PasswordForm>* android_credentials);
On 2015/10/13 12:11:33, engedy wrote:
> What is the reason for passing a pointer to the ScopedVector, as opposed to
> taking and returning a ScopedVector by value? That solution would allow
> functional-style chaining of filters.

It's used in 2 places. Both of them benefit from modifying the parameter
directly.
I'd agree with the functional style if we didn't need to call Pass() every time.

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

https://codereview.chromium.org/1375883002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.cc:391:
AffiliatedMatchHelper::TrimAndroidUsernameOnlyCredentials(&more_results);
On 2015/10/13 12:11:33, engedy wrote:
> Could you please update test PasswordStoreTest.GetLoginsWithAffiliations with
a
> username-only entry to have coverage that these are removed?

Done.

[vasilii, 2015-10-14 12:37:26]

Mike, please review.

[Mike West, 2015-10-14 13:58:19]

LGTM.

[Mike West, 2015-10-14 14:00:40]

On 2015/10/14 at 13:58:19, Mike West (slow) wrote:
> LGTM.

(I still don't like that we're accepting username-only federations, but if
that's the product decision, and the privacy team has OK'd it, I'm not going to
stick my nose in)

[vasilii, 2015-10-14 14:02:23]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-10-14 14:02:23]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/1375883002/#ps40001
(title: "engedy@ comments")

[commit-bot: I haz the power, 2015-10-14 14:03:00]

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

[commit-bot: I haz the power, 2015-10-14 14:56:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-14 14:59:02]

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