[Password manager IOS upsteaming] Upstreaming login database

[Password manager IOS upsteaming] Upstreaming login database

BUG=520437
Committed: https://crrev.com/f1db7f7c45a0e23cafc23f41d50ce607ce87c753
Cr-Commit-Position: refs/heads/master@{#358311}

[melandory, 2015-07-20 14:30:11]

Patchset #2 (id:20001) has been deleted

[melandory, 2015-07-20 14:42:25]

Patchset #1 (id:1) has been deleted

[melandory, 2015-07-20 14:42:32]

Patchset #1 (id:40001) has been deleted

[melandory, 2015-07-22 08:10:37]

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

[melandory, 2015-07-22 08:10:37]

Hi Vaclav,

review this CL, please.

This CL includes diff between upstream and downstream in logindatabase +
registration of histograms.

[vabr (Chromium), 2015-07-22 09:43:06]

On 2015/07/22 08:10:37, melandory wrote:
> Hi Vaclav,
> 
> review this CL, please.
> 
> This CL includes diff between upstream and downstream in logindatabase +
> registration of histograms.

Hi Tanja,
Just heads-up that I'm looking at the review, but will need some more time to
think through whether we want to move the keychain interaction to PasswordStore
instead. I'll get back to you today.

Cheers,
Vaclav

[vabr (Chromium), 2015-07-22 12:44:32]

Hi Tanja,

I started putting in comments, but I think we should do a bigger change --
instead of storing the passwords in Keychain and linking them through IDs, we
should do the same what happens on Mac (see
components/password_manager/core/browser/login_database_mac.cc). I'm told that
OSCrypt from Mac is also available on iOS, so this should be viable.

Now I understand that it is questionable whether we want to do this before or
after moving the iOS LoginDatabase upstream. But because there is no urgent
pressure to move it upstream, and we would need to make sure to make the change
immediately afterwards, I suggest improving the state before the move or during
the move.

I'll be assigning a related bug to you, because you are currently working on
this. Of course, feel free to opt out of this, if the increased time demand does
not fit in your current schedule (in that case, just assign to me). I'd be more
than happy to talk to you about the change we should make, if you find that
useful.

Cheers!
Vaclav

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
File components/password_manager.gypi (right):

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager.gypi:68:
'password_manager/core/browser/login_database_ios.cc',
Does this compile outside iOS? If not, we should move it to the OS==ios section
below.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
File components/password_manager/core/browser/login_database_ios_unittest.cc
(right):

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:17:
using base::ScopedCFTypeRef;
nit: alphabetise

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:52:
NULL, queryKeys, queryValues, arraysize(queryKeys),
nit: Please convert NULL to nullptr in the whole CL, to be consistent with what
we use for new code.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:61: //
Verify that the keychain now contains exactly one item.
This comment looks obsolete.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:87: for
(unsigned int i = 0; i < arraysize(test_passwords); i++) {
nit: Please use size_t instead of unsigned int for size-expressing quantities.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:94:
EXPECT_STREQ(UTF16ToUTF8(test_passwords[i]).c_str(),
Would just
EXPECT_EQ(test_passwords[i], decrypted);
work?

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:101:
form.origin = GURL("http://0.com");
nit: Make the origin equal to the signon realm. This is how production code
creates password forms, so let's mirror that in the tests.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:101:
form.origin = GURL("http://0.com");
Also, to shorten the file and increase readability, I suggest adding a
MakeSimpleForm() helper function, which will return a PasswordForm with some
basic fields set as in here. You can then use that to initialise PasswordForm
variables (and subsequently edit particular fields if necessary).

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:107:
ignore_result(login_db_->AddLogin(form));
Instead of ignoring the result, please let's compare to the expected change
structures (see login_database_unittest.cc).

(Also below.)

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:109:
form.password_value = base::ASCIIToUTF16("secret");
nit: Please store the UTF16 version of "secret" as
base::string16 updated_password = = base::ASCIIToUTF16("secret");

Then you can reference it both here and on line 121, which in turn can be
changed to EXPECT_EQ.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:113:
ASSERT_EQ(1u, changes.size());
This looks like it should be an EXPECT_EQ instead. There are more ASSERTS below
which I would turn into EXPECT. The rule of thumb should be: If the tested
condition is influenced by the tested code, use EXPECT. If it is influenced from
outside of the tested code, use ASSERT. So, for example, the ASSERT on line 57
is a good ASSERT -- the tested code cannot be blamed for the keychain support
missing at all. But here (line 113), |changes| is an output of the
LoginDatabase, which is being tested, so we should use EXPECT.

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:115:
form.password_value = base::string16();
form.password_value.clear();

https://codereview.chromium.org/1237403003/diff/80001/components/password_man...
components/password_manager/core/browser/login_database_ios_unittest.cc:178:
EXPECT_STREQ("login0", UTF16ToUTF8(logins[0]->username_element).c_str());
I would recommend storing the passwords in string16 constants, and using
EXPECT_EQ instead of casting to C-strings.

https://codereview.chromium.org/1237403003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1237403003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:15551: +<histogram
name="LoginDatabase.DecryptFailure">
Please rename the histograms to fit in the PasswordManager group.
Perhaps: PasswordManager.IOSDecryptFailure (and similarly below)?

https://codereview.chromium.org/1237403003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:15551: +<histogram
name="LoginDatabase.DecryptFailure">
I'm not sure if it is a problem that the histogram does not references the enum
it uses. I understand that the enum is not part of Chromium code (to be found in
http://www.opensource.apple.com/source/libsecurity_keychain/libsecurity_keych...),
but maybe whoever reviews the histograms has a good idea what the best way to
handle this is?

https://codereview.chromium.org/1237403003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:15555: +    iOS histogram which count
failed to retrieve the password from the keychain
Suggested rephrasing:
Apple's Security Error Code returned if decryption from iOS keychain failed.

https://codereview.chromium.org/1237403003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:15564: +    iOS histogram which counts
failed to save the password to the keychain
Suggested rephrasing:
Apple's Security Error Code returned if encryption from iOS keychain failed.

[vabr (Chromium), 2015-11-04 11:37:44]

Hi Tanja,

Just confirming what we spoke about: it would be good, if you can actually land
this CL without addressing my comments, just to remove the downstream
difference. Two points, though:

* Please comment-out the UMA macros, and do not include the changes in
histograms.xml. If I understand correctly, those histograms were never reported
(at least I do not see them in UMA), and we would get rid of them in a couple of
weeks anyway, when I switch iOS to use the same LoginDatabase as Linux and CrOS
do.
* Please associate this bug with 520437 instead of 507778.

With those above, this CL LGTM.

Thanks!
Vaclav

[vabr (Chromium), 2015-11-04 11:39:27]

For the record, the decision to first move this to remove the downstream diff,
and then consolidate the LoginDatabase and address the issues in my previous
comments, was made to accommodate chrome-next@'s wish to speed up deforking
components.

Also, Tanja, I'm happy to remove the remaining diff resulting from commenting
out the UMA by copying that downstream.

Cheers,
Vaclav

[melandory, 2015-11-06 08:05:04]

Description was changed from

==========
[Password manager IOS upsteaming] Upstreaming login database

BUG=507778
==========

to

==========
[Password manager IOS upsteaming] Upstreaming login database

BUG=520437
==========

[melandory, 2015-11-06 09:58:52]

On 2015/11/04 11:39:27, vabr (Chromium) wrote:
> For the record, the decision to first move this to remove the downstream diff,
> and then consolidate the LoginDatabase and address the issues in my previous
> comments, was made to accommodate chrome-next@'s wish to speed up deforking
> components.
> 
> Also, Tanja, I'm happy to remove the remaining diff resulting from commenting
> out the UMA by copying that downstream.
> 
> Cheers,
> Vaclav

Hey, Vaclav,

do you want to double check the change, thanks!

[vabr (Chromium), 2015-11-06 10:05:21]

Thanks, Tanja, this LGTM!

(For curious drive-by reviewers: these files are picked from iOS downstream. We
upstream them verbatim in the first phase, and will clean up the style, or more
likely remove them, in the next steps. All of this is tracked by the associated
bug.)

Cheers,
Vaclav

[melandory, 2015-11-06 12:41:44]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2015-11-06 12:41:54]

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

[commit-bot: I haz the power, 2015-11-06 12:46:09]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-11-06 12:46:48]

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