[Password Manager] Store forms with field name and id attributes missing.

[Password Manager] Store forms with field name and id attributes missing.

Currently, password form (and change password form) having no name or id
attribute specified for password input field, treated as an invalid password
form. And submission of such password form does not trigger "Save password"
prompt.

CL is split into 2 parts:
[A] Allow us storing the password forms having name or id attributes missing
from the fields [this patch].
[B] Allow us to autofill such password forms.

With this patch we leverage the fact that for non-password forms we do not
create PasswordFormManager instance. That allow us to safely drop the pre
submit check for |PasswordFormManager::HasValidPasswordForm| and save the
password forms that do not have name or id attributes specified for input
fields.

BUG=497630
TEST=PasswordFormConversionUtilsTest
       .IdentifyingFieldswithoutNameOrIdAttributes

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

[Pritam Nikam, 2015-08-12 11:49:18]

pritam.nikam@samsung.com changed reviewers:
+ vivek.vg@samsung.com

[Pritam Nikam, 2015-08-12 11:55:37]

On 2015/08/12 11:49:18, Pritam Nikam wrote:
> mailto:pritam.nikam@samsung.com changed reviewers:
> + mailto:vivek.vg@samsung.com

Hi Vivek,

Please have a look at this patch before I add Vaclav (file owner) for the
review. Its related to storing password forms that does not have fields with
name or id specified. Solution is planned to be split across 2 parts. This patch
is 1'st part where we allow storing such form into DB. PTAL!

Thanks!

[vivekg, 2015-08-14 04:35:15]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-08-14 04:35:16]

https://codereview.chromium.org/1286593003/diff/1/chrome/test/data/password/n...
File chrome/test/data/password/non_password_form.html (right):

https://codereview.chromium.org/1286593003/diff/1/chrome/test/data/password/n...
chrome/test/data/password/non_password_form.html:3: <input type="text"
placeholder="firstname" />
nit: 2 space indent

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

https://codereview.chromium.org/1286593003/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager.h:125: size_t
PendingLoginManagersCount() { return pending_login_managers_.size(); }
nit: HasPendingLoginManagerForTesting -> bool

[Pritam Nikam, 2015-08-14 06:03:42]

Thanks Vivek for review inputs.

I've incorporated these along patch set #2, ptal.
Thanks!

https://codereview.chromium.org/1286593003/diff/1/chrome/test/data/password/n...
File chrome/test/data/password/non_password_form.html (right):

https://codereview.chromium.org/1286593003/diff/1/chrome/test/data/password/n...
chrome/test/data/password/non_password_form.html:3: <input type="text"
placeholder="firstname" />
On 2015/08/14 04:35:16, vivekg_ wrote:
> nit: 2 space indent

Done.

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

https://codereview.chromium.org/1286593003/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager.h:125: size_t
PendingLoginManagersCount() { return pending_login_managers_.size(); }
On 2015/08/14 04:35:16, vivekg_ wrote:
> nit: HasPendingLoginManagerForTesting -> bool

Done.

[vivekg, 2015-08-14 06:06:25]

nit: 80 column limit for CL description.

[Pritam Nikam, 2015-08-14 06:21:08]

On 2015/08/14 06:06:25, vivekg_ wrote:
> nit: 80 column limit for CL description.

Done. Thanks!

[vivekg, 2015-08-14 06:34:07]

On 2015/08/14 at 06:21:08, pritam.nikam wrote:
> On 2015/08/14 06:06:25, vivekg_ wrote:
> > nit: 80 column limit for CL description.
> 
> Done. Thanks!

Looks good to me. Lets wait for the OWNERs review on it.

[Pritam Nikam, 2015-08-14 06:45:27]

pritam.nikam@samsung.com changed reviewers:
+ vabr@chromium.org

[Pritam Nikam, 2015-08-14 06:45:27]

Hi Vaclav,

Please have a look at this patch. I've split it in 2 parts, first part is to
allow to store in password db (this cl). And 2nd part will be to autofill such
forms, which is WIP.

With this patch we drop the |PasswordFormManager::HasValidPasswordForm| check
and that allow us to save password forms to store. Also added a test verifying
that for non-password form we do not create PasswordFormManager object proving
HasValidPasswordForm is safe to drop.

Thanks!

[vivekg, 2015-08-14 06:46:04]

vivekg@chromium.org changed reviewers:
- vivek.vg@samsung.com

[vabr (Chromium), 2015-08-14 13:52:49]

On 2015/08/14 06:45:27, Pritam Nikam wrote:
> Hi Vaclav,
> 
> Please have a look at this patch. I've split it in 2 parts, first part is to
> allow to store in password db (this cl). And 2nd part will be to autofill such
> forms, which is WIP.
> 
> With this patch we drop the |PasswordFormManager::HasValidPasswordForm| check
> and that allow us to save password forms to store. Also added a test verifying
> that for non-password form we do not create PasswordFormManager object proving
> HasValidPasswordForm is safe to drop.
> 
> Thanks!

Hi Pritam Nikam,

I'll do my best, but I'm unlikely to find time today. Hopefully, I'll be able to
review on Monday. After Monday I'm on holiday. If that's too slow, feel free to
ask another passwords code OWNER for review.

With apologies,
Vaclav

[vabr (Chromium), 2015-08-15 17:23:00]

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

[vabr (Chromium), 2015-08-15 17:23:01]

(@dvadym -- please have a look at the concerns I brought up)

Hi Pritam Nikam,
I left some comments, but I'm a bit nervous about the fact that we currently use
the empty password_element and new_password_element to decide the type of the
password form. Perhaps we do need to use a fake non-empty string if an element
has neither name nor id, as you suggested earlier.

@dvadym: In addition to the concern above, would we also have problems with
recognising forms with and without username fields at all? I would appreciate
your opinion here. Also, as I'm heading for holidays soon, it would be great if
you could take this review over.

Thanks!
Vaclav

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

https://codereview.chromium.org/1286593003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2129: // Test
that loading a non-password form will not create a PasswordFormManager
I don't think you need an end-to-end test for this. Please replace it with just
testing that a HTML form without password fields produces no PasswordForm, in
password_form_conversion_utils_browsertest.cc.

Once you do this, please remove
PasswordManager::HasPendingLoginManagerForTesting() again.

https://codereview.chromium.org/1286593003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2137: // Verify
that the PasswordFormManager will get instantiated post a password
nit: The comments are redundant, the code is short and clear. Please drop lines
2137-2138 and 2142-2143.

[Pritam Nikam, 2015-08-18 14:04:13]

Thanks Vaclav for review.
I've incorporated review comments in new patchset, and also added dummy names to
username and password fields if missing from HTML forms, ptal!

Thanks!

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

https://codereview.chromium.org/1286593003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2129: // Test
that loading a non-password form will not create a PasswordFormManager
On 2015/08/15 17:23:01, vabr (on holiday) wrote:
> I don't think you need an end-to-end test for this. Please replace it with
just
> testing that a HTML form without password fields produces no PasswordForm, in
> password_form_conversion_utils_browsertest.cc.
> 
> Once you do this, please remove
> PasswordManager::HasPendingLoginManagerForTesting() again.

Done.
I've removed this test.

We already have this test-case
|MAYBE_PasswordFormConversionUtilsTest.InvalidFormDueToNoPasswordFields| for
coverage, testing that a HTML form without password fields produces no
PasswordForm.

https://codereview.chromium.org/1286593003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2137: // Verify
that the PasswordFormManager will get instantiated post a password
On 2015/08/15 17:23:01, vabr (on holiday) wrote:
> nit: The comments are redundant, the code is short and clear. Please drop
lines
> 2137-2138 and 2142-2143.

Done.

[dvadym, 2015-08-24 13:32:08]

Hi Pritam Nikam,

  Sorry for late response, I was very busy last week with finishing Cls to
branch point.

  Changes look good, I agree with fake name approach, it should also work fine
for change password forms (not-emptiness of new password element is used as
condition for change password forms). 

  Could you please add tests for conversion utils in
password_form_conversion_utils_browsertest.cc?

  Is CL for filling forms with missing name and id ready for review? I would
prefer review both of them together.

Thanks,
Vadym

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:65: const
char kDummyUsernameField[] = "anonymous_username";
Probably it's better to make fake names that are not allowed to be attribute
name in HTML in order to avoid possible name collision.

[Pritam Nikam, 2015-08-25 09:01:10]

Thanks Vadym for review inputs.


> Hi Pritam Nikam,
> 
>   Sorry for late response, I was very busy last week with finishing Cls to
> branch point.
> 
>   Changes look good, I agree with fake name approach, it should also work fine
> for change password forms (not-emptiness of new password element is used as
> condition for change password forms). 
> 
>   Could you please add tests for conversion utils in
> password_form_conversion_utils_browsertest.cc?

Done.

Added one test
|PasswordFormConversionUtilsTest.IdentifyingFieldswithoutNameOrIdAttributes|.


> 
>   Is CL for filling forms with missing name and id ready for review? I would
> prefer review both of them together.

I've added a patch 'https://codereview.chromium.org/1292693004/' to autofill
such forms (and extended to password forms having ambiguous ids and names). It
can go irrespective of user prefer to save credentials to store or not. It also
make use of dummy names to autofill.


Thanks!

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:65: const
char kDummyUsernameField[] = "anonymous_username";
On 2015/08/24 13:32:08, dvadym wrote:
> Probably it's better to make fake names that are not allowed to be attribute
> name in HTML in order to avoid possible name collision.

Acknowledged.

According to W3C spec, <input name> or <input id> attributes are of CDATA types.
I'm not sure what can be an invalid dummy name of our choice. Can you suggest
any thing meaningful here?

[vabr (Chromium), 2015-09-04 13:19:56]

vabr@chromium.org changed reviewers:
- vabr@chromium.org

[dvadym, 2015-09-07 15:19:59]

LGTM with nits, but please don't land this CL until finishing the second CL. You
don't need to wait until landing of the CL to use changes from it in another CL,
you can rebase on the top of it:

 To make branch B depend on A (so that in the end the changes in B are the
difference of the changes in B originally minus changes in A):

git checkout B
git branch -u A
git pull --rebase

https://codereview.chromium.org/1286593003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1286593003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:913:
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
Could you please check that correct credential is stored? For this you can use
function CheckThatCredentialsStored.

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:65: const
char kDummyUsernameField[] = "anonymous_username";
On 2015/08/25 09:01:10, Pritam Nikam wrote:
> On 2015/08/24 13:32:08, dvadym wrote:
> > Probably it's better to make fake names that are not allowed to be attribute
> > name in HTML in order to avoid possible name collision.
> 
> Acknowledged.
> 
> According to W3C spec, <input name> or <input id> attributes are of CDATA
types.
> I'm not sure what can be an invalid dummy name of our choice. Can you suggest
> any thing meaningful here?

Ok, I agree, that we hardly can do something with this. Current names are ok

[Pritam Nikam, 2015-09-08 15:09:36]

Thanks! Vadym.
Incorporated input along new patch set.

Thanks!

https://codereview.chromium.org/1286593003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1286593003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:913:
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
On 2015/09/07 15:19:58, dvadym wrote:
> Could you please check that correct credential is stored? For this you can use
> function CheckThatCredentialsStored.

Done.

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1286593003/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:65: const
char kDummyUsernameField[] = "anonymous_username";
On 2015/09/07 15:19:58, dvadym wrote:
> On 2015/08/25 09:01:10, Pritam Nikam wrote:
> > On 2015/08/24 13:32:08, dvadym wrote:
> > > Probably it's better to make fake names that are not allowed to be
attribute
> > > name in HTML in order to avoid possible name collision.
> > 
> > Acknowledged.
> > 
> > According to W3C spec, <input name> or <input id> attributes are of CDATA
> types.
> > I'm not sure what can be an invalid dummy name of our choice. Can you
suggest
> > any thing meaningful here?
> 
> Ok, I agree, that we hardly can do something with this. Current names are ok

Acknowledged.

[Pritam Nikam, 2015-09-10 09:30:01]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-10 09:30:18]

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

[commit-bot: I haz the power, 2015-09-10 09:50:17]

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

[commit-bot: I haz the power, 2015-09-10 09:50:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Pritam Nikam, 2015-09-25 08:34:28]

The CQ bit was checked by pritam.nikam@samsung.com

[Pritam Nikam, 2015-09-25 08:34:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/1286593003/#ps160001
(title: "Fixed mac bot failure.")

[commit-bot: I haz the power, 2015-09-25 08:34:38]

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

[commit-bot: I haz the power, 2015-09-25 09:22:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-25 09:23:01]

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