Test that LoginDatabase preserves PasswordForm::Scheme through migration

Test that LoginDatabase preserves PasswordForm::Scheme through migration

When looking at http://crbug.com/640897 I was asking myself if a credential
stored for a basic HTTP Auth could end up being stored with
PasswordForm::SCHEME_HTML. So I added some tests to check this. They pass, so
hopefully there is no issue with this in the production code.

BUG=640897
Committed: https://crrev.com/209c98125433d97245ff70663ccb0698b3dc6aa9
Cr-Commit-Position: refs/heads/master@{#415968}

[vabr (Chromium), 2016-09-01 11:06:33]

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

[commit-bot: I haz the power, 2016-09-01 11:06:52]

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

[commit-bot: I haz the power, 2016-09-01 11:45:37]

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

[commit-bot: I haz the power, 2016-09-01 11:45:38]

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

[vabr (Chromium), 2016-09-01 12:01:44]

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

[commit-bot: I haz the power, 2016-09-01 12:02:01]

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

[vabr (Chromium), 2016-09-01 13:02:28]

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

[vabr (Chromium), 2016-09-01 13:02:29]

Hi Vasilii,

Does this additional testing look good to you?

There was not a real bug, just a hypothesis I wanted to verify with the tests.
But the additional tests could be worth keeping, hence this CL.

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-09-01 13:44:27]

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

[commit-bot: I haz the power, 2016-09-01 13:44:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vasilii, 2016-09-01 13:46:44]

lgtm

[vabr (Chromium), 2016-09-01 14:14:55]

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

[commit-bot: I haz the power, 2016-09-01 14:15:06]

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

[vabr (Chromium), 2016-09-01 14:25:20]

Thanks, Vasilii.

Could you please review my additional change to
components/password_manager/core/browser/login_database_win.cc?

Reason for the change:
As I started calling LoginDatabase::GetAutofillableLogins, the DB started to try
and decrypt the always empty password value in the test files. This is
non-trivial only on Mac and Windows. On Mac, the OSCrypt module already treats
the empty string as its own encryption. On Windows, however, it does not. That's
why the tests failed on Win.

Solutions:
(A) Provide actual encrypted values in the test files.
    - This is problematic unless we have different files for different
platforms.

(B) Modify OSCrypt on Win to also treat the empty string as its own encryption.
    - Might be viable, but I'm not sure about the impact on other OSCrypt
consumers.

(C) Modify the Win implementation of LoginDatabase to bypass decryption for the
empty string.
    - Here the impact is clear: We never store empty passwords in production
code, so no change there.

I went with (C), but happy to hear other suggestions.

Cheers,
Vaclav

[vasilii, 2016-09-01 14:58:32]

lgtm

https://codereview.chromium.org/2291123008/diff/40001/components/password_man...
File components/password_manager/core/browser/login_database_win.cc (right):

https://codereview.chromium.org/2291123008/diff/40001/components/password_man...
components/password_manager/core/browser/login_database_win.cc:25: if
(cipher_text.empty()) {
Add a comment explaining why it's here.

[vabr (Chromium), 2016-09-01 15:19:24]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-09-01 15:19:25]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2291123008/#ps60001
(title: "Add comment")

[vabr (Chromium), 2016-09-01 15:19:51]

https://codereview.chromium.org/2291123008/diff/40001/components/password_man...
File components/password_manager/core/browser/login_database_win.cc (right):

https://codereview.chromium.org/2291123008/diff/40001/components/password_man...
components/password_manager/core/browser/login_database_win.cc:25: if
(cipher_text.empty()) {
On 2016/09/01 14:58:32, vasilii wrote:
> Add a comment explaining why it's here.

Done.

[commit-bot: I haz the power, 2016-09-01 15:19:58]

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

[commit-bot: I haz the power, 2016-09-01 16:32:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 16:35:03]

Description was changed from

==========
Test that LoginDatabase preserves PasswordForm::Scheme through migration

When looking at http://crbug.com/640897 I was asking myself if a credential
stored for a basic HTTP Auth could end up being stored with
PasswordForm::SCHEME_HTML. So I added some tests to check this. They pass, so
hopefully there is no issue with this in the production code.

BUG=640897
==========

to

==========
Test that LoginDatabase preserves PasswordForm::Scheme through migration

When looking at http://crbug.com/640897 I was asking myself if a credential
stored for a basic HTTP Auth could end up being stored with
PasswordForm::SCHEME_HTML. So I added some tests to check this. They pass, so
hopefully there is no issue with this in the production code.

BUG=640897

Committed: https://crrev.com/209c98125433d97245ff70663ccb0698b3dc6aa9
Cr-Commit-Position: refs/heads/master@{#415968}
==========

[commit-bot: I haz the power, 2016-09-01 16:35:04]

Patchset 4 (id:??) landed as
https://crrev.com/209c98125433d97245ff70663ccb0698b3dc6aa9
Cr-Commit-Position: refs/heads/master@{#415968}