Add unit tests for easyUnlockPrivate API - crypto methods

Add unit tests for easyUnlockPrivate API - crypto methods

Also, adds some logic to FakeEasyUnlockClient.

BUG=407996
(note that these would have caught the crashes in bugs 404798 and 405976 :/)

Committed: https://crrev.com/90148379eb43c46fbd6c6923c6cfa83d0d6465b1
Cr-Commit-Position: refs/heads/master@{#292692}

[tbarzic, 2014-08-28 18:36:28]

tbarzic@chromium.org changed reviewers:
+ tengs@chromium.org

[tbarzic, 2014-08-29 00:36:52]

tbarzic@chromium.org changed reviewers:
+ satorux@chromium.org

[tbarzic, 2014-08-29 00:36:52]

adding Satoru for OWNER approval in chromeos/

[satorux1, 2014-08-29 00:56:00]

Will take a look now. Meanwhile, could you test if the ASAN browser_tests can
detect the double-free issue with https://codereview.chromium.org/486933002
reverted?

I'd upload a new patch set with the fix reverted and run the ASAN trybot.

[tbarzic, 2014-08-29 00:57:11]

On 2014/08/29 00:56:00, satorux1 wrote:
> Will take a look now. Meanwhile, could you test if the ASAN browser_tests can
> detect the double-free issue with https://codereview.chromium.org/486933002
> reverted?
> 
> I'd upload a new patch set with the fix reverted and run the ASAN trybot.

these are actually unit_tests, and they crash with that cl reverted..

[satorux1, 2014-08-29 01:10:50]

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc
(right):

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:26:
base::BinaryValue* StringToBinaryValue(const std::string value) {
function comment is missing.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:27:
// NOTE: Trailing '\0' is intentionally not included.
you can make it a part of the function comment.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:63:
void GetSingleBinaryResultAsString(UIThreadExtensionFunction* function,
function comment is missing.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:74:
result_binary_value->GetSize());
I think you can just return the result string.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
File chromeos/dbus/fake_easy_unlock_client.cc (right):

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client.cc:125:
callback.Run(base::StringPrintf(kSecureMessageFormat,
I'd suggest inlining the format string here because otherwise it's hard to check
if the %s specifiers and parameters match correctly.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client.cc:145:
callback.Run(base::StringPrintf(kUnwrappedSecureMessageFormat,
ditto.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
File chromeos/dbus/fake_easy_unlock_client_unittest.cc (right):

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:14: void
RecordKeyPair(std::string* private_key_target,
function comment is missing.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:22: void
RecordData(std::string* data_target,
ditto. just a one liner would suffice

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:106: std::string shared_key_1;
could you add some comments in the test? it was hard for me to understand.

[satorux1, 2014-08-29 01:11:22]

On 2014/08/29 00:57:11, tbarzic wrote:
> On 2014/08/29 00:56:00, satorux1 wrote:
> > Will take a look now. Meanwhile, could you test if the ASAN browser_tests
can
> > detect the double-free issue with https://codereview.chromium.org/486933002
> > reverted?
> > 
> > I'd upload a new patch set with the fix reverted and run the ASAN trybot.
> 
> these are actually unit_tests, and they crash with that cl reverted..

That's nice. Please mention it in the patch description.

[tbarzic, 2014-08-29 02:47:05]

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc
(right):

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:26:
base::BinaryValue* StringToBinaryValue(const std::string value) {
On 2014/08/29 01:10:49, satorux1 wrote:
> function comment is missing.

Done.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:27:
// NOTE: Trailing '\0' is intentionally not included.
On 2014/08/29 01:10:50, satorux1 wrote:
> you can make it a part of the function comment.

Done.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:63:
void GetSingleBinaryResultAsString(UIThreadExtensionFunction* function,
On 2014/08/29 01:10:50, satorux1 wrote:
> function comment is missing.

Done.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:74:
result_binary_value->GetSize());
On 2014/08/29 01:10:49, satorux1 wrote:
> I think you can just return the result string.

The reason I didn't return result is to  be able to use ASSERTs.
Let me know what you think about the new approach.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
File chromeos/dbus/fake_easy_unlock_client.cc (right):

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client.cc:125:
callback.Run(base::StringPrintf(kSecureMessageFormat,
On 2014/08/29 01:10:50, satorux1 wrote:
> I'd suggest inlining the format string here because otherwise it's hard to
check
> if the %s specifiers and parameters match correctly.

Done.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client.cc:145:
callback.Run(base::StringPrintf(kUnwrappedSecureMessageFormat,
On 2014/08/29 01:10:50, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
File chromeos/dbus/fake_easy_unlock_client_unittest.cc (right):

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:14: void
RecordKeyPair(std::string* private_key_target,
On 2014/08/29 01:10:50, satorux1 wrote:
> function comment is missing.

Done.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:22: void
RecordData(std::string* data_target,
On 2014/08/29 01:10:50, satorux1 wrote:
> ditto. just a one liner would suffice

Done.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:106: std::string shared_key_1;
On 2014/08/29 01:10:50, satorux1 wrote:
> could you add some comments in the test? it was hard for me to understand.

Done.

[satorux1, 2014-08-29 03:48:14]

lgtm, but please have this reviewed by tengs@ too as I'm not very familiar with
the code.

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc
(right):

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:74:
result_binary_value->GetSize());
On 2014/08/29 02:47:04, tbarzic wrote:
> On 2014/08/29 01:10:49, satorux1 wrote:
> > I think you can just return the result string.
> 
> The reason I didn't return result is to  be able to use ASSERTs.
> Let me know what you think about the new approach.

Oh I see. I generally prefer not to have ASSERT/EXPECT in helper functions,
because it's hard to identify where it fails. For instance:

TEST(...) {
  Helper();
  ...
  Helper();
  ...
  Helper();
}

If EXPECT/ASSERT fails in Helper(), it's hard to tell which caller of Helper()
caused the failure. The tests in this file does not have this problem because
ExpectSingleBinaryResultEquals() is only called once inside a test function,
though.

That said, I'm fine with this version, but I'd remove ASSERT/EXPECT from this
function and returns "", with an error message.

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
File chromeos/dbus/fake_easy_unlock_client_unittest.cc (right):

https://codereview.chromium.org/513013003/diff/60001/chromeos/dbus/fake_easy_...
chromeos/dbus/fake_easy_unlock_client_unittest.cc:106: std::string shared_key_1;
On 2014/08/29 02:47:05, tbarzic wrote:
> On 2014/08/29 01:10:50, satorux1 wrote:
> > could you add some comments in the test? it was hard for me to understand.
> 
> Done.

Thank you for adding. These are helpful.

https://codereview.chromium.org/513013003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc
(right):

https://codereview.chromium.org/513013003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:29:
return base::BinaryValue::CreateWithCopiedBuffer(value.c_str(),
maybe data() as we don't care about trailing '\0' here

[tbarzic, 2014-08-29 04:30:27]

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc
(right):

https://codereview.chromium.org/513013003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:74:
result_binary_value->GetSize());
On 2014/08/29 03:48:14, satorux1 wrote:
> On 2014/08/29 02:47:04, tbarzic wrote:
> > On 2014/08/29 01:10:49, satorux1 wrote:
> > > I think you can just return the result string.
> > 
> > The reason I didn't return result is to  be able to use ASSERTs.
> > Let me know what you think about the new approach.
> 
> Oh I see. I generally prefer not to have ASSERT/EXPECT in helper functions,
> because it's hard to identify where it fails. For instance:
> 
> TEST(...) {
>   Helper();
>   ...
>   Helper();
>   ...
>   Helper();
> }
> 
> If EXPECT/ASSERT fails in Helper(), it's hard to tell which caller of Helper()
> caused the failure. The tests in this file does not have this problem because
> ExpectSingleBinaryResultEquals() is only called once inside a test function,
> though.
> 
> That said, I'm fine with this version, but I'd remove ASSERT/EXPECT from this
> function and returns "", with an error message.

Yes, I agree with everything you said. I generally try to avoid these unless
it's completely clear where the failure is (as in this case).
Since I'm half way between the approaches, I'll go with your suggestion.

https://codereview.chromium.org/513013003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc
(right):

https://codereview.chromium.org/513013003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc:29:
return base::BinaryValue::CreateWithCopiedBuffer(value.c_str(),
On 2014/08/29 03:48:14, satorux1 wrote:
> maybe data() as we don't care about trailing '\0' here

Done.

[Tim Song, 2014-08-29 17:38:58]

lgtm

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
File chromeos/dbus/fake_easy_unlock_client.cc (right):

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
chromeos/dbus/fake_easy_unlock_client.cc:89: "{\"secret_key\": [%d, %d]}",
Why not just [private_key, public_key]? Is the ECDH operation commutative?

[tbarzic, 2014-08-29 17:43:39]

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
File chromeos/dbus/fake_easy_unlock_client.cc (right):

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
chromeos/dbus/fake_easy_unlock_client.cc:89: "{\"secret_key\": [%d, %d]}",
On 2014/08/29 17:38:58, Tim Song wrote:
> Why not just [private_key, public_key]? Is the ECDH operation commutative?

Yes, in respect to the key pairs the used keys come from.
(e.g. [key_pair1.private_key, key_pair2.public_key] and [key_pair2.private_key,
key_pair1.public_key] should generate the same result).

[Tim Song, 2014-08-29 17:46:00]

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
File chromeos/dbus/fake_easy_unlock_client.cc (right):

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
chromeos/dbus/fake_easy_unlock_client.cc:89: "{\"secret_key\": [%d, %d]}",
On 2014/08/29 17:43:39, tbarzic wrote:
> On 2014/08/29 17:38:58, Tim Song wrote:
> > Why not just [private_key, public_key]? Is the ECDH operation commutative?
> 
> Yes, in respect to the key pairs the used keys come from.
> (e.g. [key_pair1.private_key, key_pair2.public_key] and
[key_pair2.private_key,
> key_pair1.public_key] should generate the same result).

Good to know. Can you mention this explicitly in the comment above. It wasn't
really clear to me.

[tbarzic, 2014-08-29 18:29:46]

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
File chromeos/dbus/fake_easy_unlock_client.cc (right):

https://codereview.chromium.org/513013003/diff/140001/chromeos/dbus/fake_easy...
chromeos/dbus/fake_easy_unlock_client.cc:89: "{\"secret_key\": [%d, %d]}",
On 2014/08/29 17:46:00, Tim Song wrote:
> On 2014/08/29 17:43:39, tbarzic wrote:
> > On 2014/08/29 17:38:58, Tim Song wrote:
> > > Why not just [private_key, public_key]? Is the ECDH operation commutative?
> > 
> > Yes, in respect to the key pairs the used keys come from.
> > (e.g. [key_pair1.private_key, key_pair2.public_key] and
> [key_pair2.private_key,
> > key_pair1.public_key] should generate the same result).
> 
> Good to know. Can you mention this explicitly in the comment above. It wasn't
> really clear to me.

Done.

[tbarzic, 2014-08-29 18:29:52]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-08-29 18:31:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tbarzic@chromium.org/513013003/160001

[commit-bot: I haz the power, 2014-08-29 22:30:19]

Committed patchset #9 (id:160001) as 9ce8aeb520eaf6224a4508b4ef2534ad7a3e567c

[commit-bot: I haz the power, 2014-09-10 03:10:51]

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