NSSDecryptor::ReadAndParseSignons() - improved password form scheme detection,

NSSDecryptor::ReadAndParseSignons() - improved password form scheme detection,
when database row has non-empty realm, assume autofill::PasswordForm::SCHEME_BASIC.

BUG=375158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277393

[Ilya Sherman, 2014-05-30 23:17:54]

Thanks for the fix!  Could you please also add test coverage?

https://codereview.chromium.org/306123004/diff/1/chrome/utility/importer/nss_...
File chrome/utility/importer/nss_decryptor.cc (right):

https://codereview.chromium.org/306123004/diff/1/chrome/utility/importer/nss_...
chrome/utility/importer/nss_decryptor.cc:289: // TODO(ljagielski) this isn't
perfect, find out how to
nit: "TODO(ljagielski)" -> "TODO(ljagielski):"

https://codereview.chromium.org/306123004/diff/1/chrome/utility/importer/nss_...
chrome/utility/importer/nss_decryptor.cc:290: // identify scheme digest
This TODO is a bit vague on what the problem is.  Please provide slightly more
detail, so that someone coming across this code out of context would still be
able to understand what this TODO is all about.

[vabr (Chromium), 2014-06-02 09:09:57]

Thanks for the fix!
+1 to what Ilya said so far. :)

[Lukasz Jagielski, 2014-06-03 13:16:21]

On 2014/05/30 23:17:54, Ilya Sherman wrote:
> Thanks for the fix!  Could you please also add test coverage?
> 
>
https://codereview.chromium.org/306123004/diff/1/chrome/utility/importer/nss_...
> File chrome/utility/importer/nss_decryptor.cc (right):
> 
>
https://codereview.chromium.org/306123004/diff/1/chrome/utility/importer/nss_...
> chrome/utility/importer/nss_decryptor.cc:289: // TODO(ljagielski) this isn't
> perfect, find out how to
> nit: "TODO(ljagielski)" -> "TODO(ljagielski):"
> 
>
https://codereview.chromium.org/306123004/diff/1/chrome/utility/importer/nss_...
> chrome/utility/importer/nss_decryptor.cc:290: // identify scheme digest
> This TODO is a bit vague on what the problem is.  Please provide slightly more
> detail, so that someone coming across this code out of context would still be
> able to understand what this TODO is all about.

Could You give me a hint to which component's unit tests should be involved?

[vabr (Chromium), 2014-06-03 13:30:28]

Starting from the code you modified, and going up until the caller has a unit
test, yields chrome/utility/importer/firefox_importer_unittest.cc like a good
place to start looking at.

Use your judgement, though. :)

[Lukasz Jagielski, 2014-06-05 09:39:42]

On 2014/06/05 09:36:14, ljagielski wrote:

I had to write utility function NSSDecryptor::Encode in order to generate
encrypted entries in signons.sqlite. I can add this code to the review, if
you're interested.
Regards,
Łukasz

[vabr (Chromium), 2014-06-05 16:36:19]

Thanks for the tests.

It would be indeed good, if you can document how you created the signons.sqlite
file. One way to do it would be to generate it on the fly during the test. That
would spare the check-in of the binary file, and make it much easier to read,
understand, and edit the test if needed.
If that's not practicable, please include reasonably detailed instructions on
how to generate the sqlite file in the comment for the test.

Please also make sure to address isherman's early comments, and get someone from
the corresponding OWNERS to review the added IPC messages.

Cheers,
Vaclav

I have a couple of comments below.

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:48:
TEST(FirefoxImporterTest, MAYBE_NSS(FirefoxNSSDecryptorParseSignons)) {
Please comment on what is the goal of this test.
Is it trying to verify that the schemes are correctly saved and restored? Or
that they are correctly guessed? Or something else?

In particular, it should explain the relation between the realm and the scheme.

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:50: PasswordFormVector
forms_from_signons, forms_template;
nit: It seems to be more common in Chromium to have one variable per
declaration. Please separate:
PasswordFormVector forms_from_signons;
PasswordFormVector forms_template;

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:50: PasswordFormVector
forms_from_signons, forms_template;
nit: Consider renaming forms_template to expected_forms. To me, "template"
indicates that you would be making more forms from the template forms.

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:53: form.action = GURL("");
nit: Please leave out assignments to the fields if you only assign the default
value. That would make it clearer, what are the values this test cares about.

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:63: form.origin =
GURL("http://test2.server.com:1234/");
nit: Please use a separate variable for the second form, and name both
appropriately, so that it's clear what's the difference (mainly the scheme?).

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:90:
ASSERT_EQ(forms_template, forms_from_signons);
Did you mean EXPECT_EQ instead of ASSERT_EQ?
ASSERT should be for conditions which are not relevant directly to the tested
code (like getting the directory path above), EXPECT is what the test is
directly about.

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest_utils.h (right):

https://codereview.chromium.org/306123004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest_utils.h:24: typedef
std::vector<autofill::PasswordForm> PasswordFormVector;
Please don't pollute the global namespace with this typedef. If you really need
it, could you move it inside FFUnitTestDecryptorProxy?

[Lukasz Jagielski, 2014-06-09 12:41:16]

Fixes applied, please review again.

[vabr (Chromium), 2014-06-10 12:27:01]

Thank you,
I really like how you improved the test, it's clear what it does, and helps also
understand the fix!

LGTM for the following files:
chrome/utility/importer/firefox_importer_unittest.cc
chrome/utility/importer/firefox_importer_unittest_utils.h
chrome/utility/importer/firefox_importer_unittest_utils_mac.cc
chrome/utility/importer/nss_decryptor.cc

Please make sure to have approval from dcheng or anybody else on the security
team for the remaining two files with IPC macros.

Thanks!
Vaclav

[vabr (Chromium), 2014-06-10 12:29:04]

Actually,

Please also wait for approval from isherman@ for the files I approved -- I am
not an OWNER of those either.

Sorry about that.

Cheers,
Vaclav

[Ilya Sherman, 2014-06-11 00:19:38]

LGTM % nits.  Thanks!

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:51: // firefox's signons
db. We insert two entries into moz_logins table, first
nit: "table, first has" -> "table. The first has the"

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:53: // PasswordForm should
have SCHEME_BASIC in scheme. Second entry has NULL
nit: "Second entry" -> "The second entry"

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest_messages_internal.h
(right):

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest_messages_internal.h:29:
std::vector<autofill::PasswordForm>)
nit: Please follow the existing pattern in this file (which is common to most
(all?) IPC files), of including a commented-out variable name for each
parameter.

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest_messages_internal.h:31:
IPC_MESSAGE_CONTROL1(Msg_ParseSignons, base::FilePath)
nit: Please add brief documentation for the added messages.

[Lukasz Jagielski, 2014-06-11 16:10:17]

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:51: // firefox's signons
db. We insert two entries into moz_logins table, first
On 2014/06/11 00:19:38, Ilya Sherman wrote:
> nit: "table, first has" -> "table. The first has the"

Done.

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:53: // PasswordForm should
have SCHEME_BASIC in scheme. Second entry has NULL
On 2014/06/11 00:19:38, Ilya Sherman wrote:
> nit: "Second entry" -> "The second entry"

Done.

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest_messages_internal.h
(right):

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest_messages_internal.h:29:
std::vector<autofill::PasswordForm>)
On 2014/06/11 00:19:38, Ilya Sherman wrote:
> nit: Please follow the existing pattern in this file (which is common to most
> (all?) IPC files), of including a commented-out variable name for each
> parameter.

Done.

https://codereview.chromium.org/306123004/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest_messages_internal.h:31:
IPC_MESSAGE_CONTROL1(Msg_ParseSignons, base::FilePath)
On 2014/06/11 00:19:38, Ilya Sherman wrote:
> nit: Please add brief documentation for the added messages.

Done.

[Ilya Sherman, 2014-06-11 17:37:47]

Daniel, PTAL at the IPC changes.  Thanks :)

[dcheng, 2014-06-12 03:24:20]

https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
File components/autofill/content/common/autofill_param_traits_macros.h (right):

https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_param_traits_macros.h:26:
IPC_ENUM_TRAITS(autofill::PasswordForm::Scheme)
Please use IPC_ENUM_TRAITS_MAX_VALUE. Also, where is this IPC message
sent/consumed? With a quick search, the only IPC message I see referencing it is
AutofillMsg_FormNotBlacklisted -- is that the message the importer is using?

https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_param_traits_macros.h:26:
IPC_ENUM_TRAITS(autofill::PasswordForm::Scheme)
Please use IPC_ENUM_TRAITS_MAX_VALUE. Also, where is this IPC message
sent/consumed? With a quick search, the only IPC message I see referencing it is
AutofillMsg_FormNotBlacklisted -- is that the message the importer is using?

[Lukasz Jagielski, 2014-06-12 07:52:38]

https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
File components/autofill/content/common/autofill_param_traits_macros.h (right):

https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_param_traits_macros.h:26:
IPC_ENUM_TRAITS(autofill::PasswordForm::Scheme)
There's a bunch of IPC messages using autofill::PasswordForm in
components/autofill/content/common/autofill_messages.h
Importer is using ProfileImportProcessHostMsg_NotifyPasswordFormReady defined in
chrome/common/importer/profile_import_process_messages.h

On 2014/06/12 03:24:20, dcheng wrote:
> Please use IPC_ENUM_TRAITS_MAX_VALUE. Also, where is this IPC message
> sent/consumed? With a quick search, the only IPC message I see referencing it
is
> AutofillMsg_FormNotBlacklisted -- is that the message the importer is using?

[dcheng, 2014-06-12 19:07:09]

On 2014/06/12 at 07:52:38, ljagielski wrote:
>
https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
> File components/autofill/content/common/autofill_param_traits_macros.h
(right):
> 
>
https://codereview.chromium.org/306123004/diff/60001/components/autofill/cont...
> components/autofill/content/common/autofill_param_traits_macros.h:26:
IPC_ENUM_TRAITS(autofill::PasswordForm::Scheme)
> There's a bunch of IPC messages using autofill::PasswordForm in
components/autofill/content/common/autofill_messages.h
> Importer is using ProfileImportProcessHostMsg_NotifyPasswordFormReady defined
in chrome/common/importer/profile_import_process_messages.h
> 
> On 2014/06/12 03:24:20, dcheng wrote:
> > Please use IPC_ENUM_TRAITS_MAX_VALUE. Also, where is this IPC message
> > sent/consumed? With a quick search, the only IPC message I see referencing
it is
> > AutofillMsg_FormNotBlacklisted -- is that the message the importer is using?

components/autofill/content/common/autofill_param_traits_macros.h LGTM

[Lukasz Jagielski, 2014-06-13 07:27:37]

The CQ bit was checked by ljagielski@opera.com

[commit-bot: I haz the power, 2014-06-13 07:29:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ljagielski@opera.com/306123004/80001

[commit-bot: I haz the power, 2014-06-13 12:00:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-13 12:52:59]

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

[commit-bot: I haz the power, 2014-06-13 12:53:01]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Lukasz Jagielski, 2014-06-13 15:44:47]

Hi,
PTAL at fixes.
Thanks,
Łukasz

[Ilya Sherman, 2014-06-13 21:57:03]

LGTM w/ a nit.  Thanks.

https://codereview.chromium.org/306123004/diff/100001/chrome/utility/importer...
File chrome/utility/importer/firefox_importer_unittest_utils.h (right):

https://codereview.chromium.org/306123004/diff/100001/chrome/utility/importer...
chrome/utility/importer/firefox_importer_unittest_utils.h:94:
std::vector<autofill::PasswordForm> out_vec;
nit: Please avoid abbreviations in variable names.  In this case, "signons" or
even just "result" would be reasonable variable names.

[Lukasz Jagielski, 2014-06-16 07:59:47]

The CQ bit was checked by ljagielski@opera.com

[commit-bot: I haz the power, 2014-06-16 08:00:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ljagielski@opera.com/306123004/120001

[commit-bot: I haz the power, 2014-06-16 10:17:31]

Change committed as 277393