OwnerManager, allows use of OwnerKeyUtils to take ownership of a device

OwnerManager, allows use of OwnerKeyUtils to take ownership of a device

    BUG=chromium-os:4485
TEST=unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54199

[gauravsh, 2010-07-29 08:05:05]

Mostly nits. I would recommend getting the test stuff reviewed by someone else
as I am not that familiar and comfortable with the testing framework (being a
"firmware" guy, you know).

http://codereview.chromium.org/3058021/diff/3001/4001
File chrome/browser/chromeos/login/owner_manager.cc (right):

http://codereview.chromium.org/3058021/diff/3001/4001#newcode45
chrome/browser/chromeos/login/owner_manager.cc:45: NewRunnableMethod(this,
&OwnerManager::GenerateKeys));
Would GenerateKeys() be better called TakeOwnersip()? Or maybe
StartTakeOwnership...() should be called StartGenerateKeys()?

http://codereview.chromium.org/3058021/diff/3001/4001#newcode103
chrome/browser/chromeos/login/owner_manager.cc:103: ChromeThread::PostTask(
question: a comment about what's happening here, maybe? - from the looks of it
if generate key pair fails, you send a notification that the fetch key attempt
completed. How do you communicate that getting the owner key failed?

http://codereview.chromium.org/3058021/diff/3001/4001#newcode165
chrome/browser/chromeos/login/owner_manager.cc:165: d, SUCCESS, data));
can signing fail here in any case?

http://codereview.chromium.org/3058021/diff/3001/4001#newcode184
chrome/browser/chromeos/login/owner_manager.cc:184: // TODO(cmasone): Verify
|signature| over |data| with |public_key_|.
if verify fails, you should be returning that info to the CallDelegate - should
you add that to the TODO?

http://codereview.chromium.org/3058021/diff/3001/4002
File chrome/browser/chromeos/login/owner_manager.h (right):

http://codereview.chromium.org/3058021/diff/3001/4002#newcode52
chrome/browser/chromeos/login/owner_manager.h:52: // Details are a boolean
indicating whether the attempt succeeded or not.
Nit/Curious: "Details are a" Why not - "Returns a"?

http://codereview.chromium.org/3058021/diff/3001/4002#newcode74
chrome/browser/chromeos/login/owner_manager.h:74: // thread.  Otherwise, you'll
get called back on the UI thread.
nit: description of what this function does (it verifies, but what exactly is
that supposed to entail?)

http://codereview.chromium.org/3058021/diff/3001/4002#newcode80
chrome/browser/chromeos/login/owner_manager.h:80: // Call this on the FILE
thread.
nit: maybe it would be better to mention the function semantics/call-suggestions
after you describe what it does (like you do in the public functions above?)

http://codereview.chromium.org/3058021/diff/3001/4002#newcode98
chrome/browser/chromeos/login/owner_manager.h:98: // ensures that we have the
keys we need.  Then, does the signature.
nitty nit: s/does/computes/

http://codereview.chromium.org/3058021/diff/3001/4002#newcode101
chrome/browser/chromeos/login/owner_manager.h:101: // successful return code and
the signature blob.
nit: assuming the signature blob is returned in |data|, mention that the
signature blob is returned in |data|

http://codereview.chromium.org/3058021/diff/3001/4002#newcode103
chrome/browser/chromeos/login/owner_manager.h:103: // error and passes an empty
string.
nit: empty string in |data|

http://codereview.chromium.org/3058021/diff/3001/4002#newcode113
chrome/browser/chromeos/login/owner_manager.h:113: // successful return code and
an empty string.
empty string in what? both |data| and |signature| are passed by reference.

http://codereview.chromium.org/3058021/diff/3001/4003
File chrome/browser/chromeos/login/owner_manager_unittest.cc (right):

http://codereview.chromium.org/3058021/diff/3001/4003#newcode26
chrome/browser/chromeos/login/owner_manager_unittest.cc:26: #include
"testing/gmock/include/gmock/gmock.h"
nit: alpha order, gmock goes before gtest :)

[Chris Masone, 2010-07-29 18:40:17]

http://codereview.chromium.org/3058021/diff/3001/4001
File chrome/browser/chromeos/login/owner_manager.cc (right):

http://codereview.chromium.org/3058021/diff/3001/4001#newcode45
chrome/browser/chromeos/login/owner_manager.cc:45: NewRunnableMethod(this,
&OwnerManager::GenerateKeys));
On 2010/07/29 08:05:05, gauravsh wrote:
> Would GenerateKeys() be better called TakeOwnersip()? Or maybe
> StartTakeOwnership...() should be called StartGenerateKeys()?

I named it this way because taking ownership is the high-level thing you're
doing, while GenerateKeys just generates a key and then posts a task to export
it...maybe the name should reflect that?

http://codereview.chromium.org/3058021/diff/3001/4001#newcode103
chrome/browser/chromeos/login/owner_manager.cc:103: ChromeThread::PostTask(
On 2010/07/29 08:05:05, gauravsh wrote:
> question: a comment about what's happening here, maybe? - from the looks of it
> if generate key pair fails, you send a notification that the fetch key attempt
> completed. How do you communicate that getting the owner key failed? 

I send NULL in the details object instead of a pointer to the key.

Added a comment to this effect, in LoadOwnerKey as well.

http://codereview.chromium.org/3058021/diff/3001/4001#newcode165
chrome/browser/chromeos/login/owner_manager.cc:165: d, SUCCESS, data));
On 2010/07/29 08:05:05, gauravsh wrote:
> can signing fail here in any case?

Probably, and once I actually do any signing, I'll totally add logic to report
those errors :-)

http://codereview.chromium.org/3058021/diff/3001/4001#newcode184
chrome/browser/chromeos/login/owner_manager.cc:184: // TODO(cmasone): Verify
|signature| over |data| with |public_key_|.
On 2010/07/29 08:05:05, gauravsh wrote:
> if verify fails, you should be returning that info to the CallDelegate -
should
> you add that to the TODO?

Done.

http://codereview.chromium.org/3058021/diff/3001/4002
File chrome/browser/chromeos/login/owner_manager.h (right):

http://codereview.chromium.org/3058021/diff/3001/4002#newcode52
chrome/browser/chromeos/login/owner_manager.h:52: // Details are a boolean
indicating whether the attempt succeeded or not.
On 2010/07/29 08:05:05, gauravsh wrote:
> Nit/Curious: "Details are a" Why not - "Returns a"?

I talk about the actual method's return value a few lines up.  

This comment refers to the payload of the notification I send upon completing
the fetch attempt.  Notifications in chromium, when they are delivered to
observers, come with a templatized Details object.  I'm saying that the
notification I send comes with a Details object (as opposed to a special
NoDetails value), and saying what the semantics of the value inside are.  The
comment here is inaccurate, though, so I've fixed it.

http://codereview.chromium.org/3058021/diff/3001/4002#newcode74
chrome/browser/chromeos/login/owner_manager.h:74: // thread.  Otherwise, you'll
get called back on the UI thread.
On 2010/07/29 08:05:05, gauravsh wrote:
> nit: description of what this function does (it verifies, but what exactly is
> that supposed to entail?)

Done.

http://codereview.chromium.org/3058021/diff/3001/4002#newcode80
chrome/browser/chromeos/login/owner_manager.h:80: // Call this on the FILE
thread.
On 2010/07/29 08:05:05, gauravsh wrote:
> nit: maybe it would be better to mention the function
semantics/call-suggestions
> after you describe what it does (like you do in the public functions above?)

Done.

http://codereview.chromium.org/3058021/diff/3001/4002#newcode98
chrome/browser/chromeos/login/owner_manager.h:98: // ensures that we have the
keys we need.  Then, does the signature.
On 2010/07/29 08:05:05, gauravsh wrote:
> nitty nit: s/does/computes/

Done.

http://codereview.chromium.org/3058021/diff/3001/4002#newcode101
chrome/browser/chromeos/login/owner_manager.h:101: // successful return code and
the signature blob.
On 2010/07/29 08:05:05, gauravsh wrote:
> nit: assuming the signature blob is returned in |data|, mention that the
> signature blob is returned in |data|

it's actually passed to d->OnKeyOpComplete as the |payload| arg.  updating the
comment.

http://codereview.chromium.org/3058021/diff/3001/4002#newcode103
chrome/browser/chromeos/login/owner_manager.h:103: // error and passes an empty
string.
On 2010/07/29 08:05:05, gauravsh wrote:
> nit: empty string in |data|

ditto

http://codereview.chromium.org/3058021/diff/3001/4002#newcode113
chrome/browser/chromeos/login/owner_manager.h:113: // successful return code and
an empty string.
On 2010/07/29 08:05:05, gauravsh wrote:
> empty string in what? both |data| and |signature| are passed by reference

When the function is done, it posts a task to thread |thread_id| that calls
d->OnKeyOpComplete() with these values as arguments.  Updating the comment here
as well

http://codereview.chromium.org/3058021/diff/3001/4003
File chrome/browser/chromeos/login/owner_manager_unittest.cc (right):

http://codereview.chromium.org/3058021/diff/3001/4003#newcode26
chrome/browser/chromeos/login/owner_manager_unittest.cc:26: #include
"testing/gmock/include/gmock/gmock.h"
On 2010/07/29 08:05:05, gauravsh wrote:
> nit: alpha order, gmock goes before gtest :)

Done.

[gauravsh, 2010-07-29 21:17:50]

Lgtm

On Jul 29, 2010 11:40 AM, <cmasone@chromium.org> wrote:
>
> http://codereview.chromium.org/3058021/diff/3001/4001
> File chrome/browser/chromeos/login/owner_manager.cc (right):
>
> http://codereview.chromium.org/3058021/diff/3001/4001#newcode45
> chrome/browser/chromeos/login/owner_manager.cc:45:
> NewRunnableMethod(this, &OwnerManager::GenerateKeys));
> On 2010/07/29 08:05:05, gauravsh wrote:
>> Would GenerateKeys() be better called TakeOwnersip()? Or maybe
>> StartTakeOwnership...() should be called StartGenerateKeys()?
>
> I named it this way because taking ownership is the high-level thing
> you're doing, while GenerateKeys just generates a key and then posts a
> task to export it...maybe the name should reflect that?
>
> http://codereview.chromium.org/3058021/diff/3001/4001#newcode103
> chrome/browser/chromeos/login/owner_manager.cc:103:
> ChromeThread::PostTask(
> On 2010/07/29 08:05:05, gauravsh wrote:
>> question: a comment about what's happening here, maybe? - from the
> looks of it
>> if generate key pair fails, you send a notification that the fetch key
> attempt
>> completed. How do you communicate that getting the owner key failed?
>
> I send NULL in the details object instead of a pointer to the key.
>
> Added a comment to this effect, in LoadOwnerKey as well.
>
> http://codereview.chromium.org/3058021/diff/3001/4001#newcode165
> chrome/browser/chromeos/login/owner_manager.cc:165: d, SUCCESS, data));
> On 2010/07/29 08:05:05, gauravsh wrote:
>> can signing fail here in any case?
>
> Probably, and once I actually do any signing, I'll totally add logic to
> report those errors :-)
>
> http://codereview.chromium.org/3058021/diff/3001/4001#newcode184
> chrome/browser/chromeos/login/owner_manager.cc:184: // TODO(cmasone):
> Verify |signature| over |data| with |public_key_|.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> if verify fails, you should be returning that info to the CallDelegate
> - should
>> you add that to the TODO?
>
> Done.
>
> http://codereview.chromium.org/3058021/diff/3001/4002
> File chrome/browser/chromeos/login/owner_manager.h (right):
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode52
> chrome/browser/chromeos/login/owner_manager.h:52: // Details are a
> boolean indicating whether the attempt succeeded or not.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> Nit/Curious: "Details are a" Why not - "Returns a"?
>
> I talk about the actual method's return value a few lines up.
>
> This comment refers to the payload of the notification I send upon
> completing the fetch attempt. Notifications in chromium, when they are
> delivered to observers, come with a templatized Details object. I'm
> saying that the notification I send comes with a Details object (as
> opposed to a special NoDetails value), and saying what the semantics of
> the value inside are. The comment here is inaccurate, though, so I've
> fixed it.
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode74
> chrome/browser/chromeos/login/owner_manager.h:74: // thread. Otherwise,
> you'll get called back on the UI thread.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> nit: description of what this function does (it verifies, but what
> exactly is
>> that supposed to entail?)
>
> Done.
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode80
> chrome/browser/chromeos/login/owner_manager.h:80: // Call this on the
> FILE thread.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> nit: maybe it would be better to mention the function
> semantics/call-suggestions
>> after you describe what it does (like you do in the public functions
> above?)
>
> Done.
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode98
> chrome/browser/chromeos/login/owner_manager.h:98: // ensures that we
> have the keys we need. Then, does the signature.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> nitty nit: s/does/computes/
>
> Done.
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode101
> chrome/browser/chromeos/login/owner_manager.h:101: // successful return
> code and the signature blob.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> nit: assuming the signature blob is returned in |data|, mention that
> the
>> signature blob is returned in |data|
>
> it's actually passed to d->OnKeyOpComplete as the |payload| arg.
> updating the comment.
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode103
> chrome/browser/chromeos/login/owner_manager.h:103: // error and passes
> an empty string.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> nit: empty string in |data|
>
> ditto
>
> http://codereview.chromium.org/3058021/diff/3001/4002#newcode113
> chrome/browser/chromeos/login/owner_manager.h:113: // successful return
> code and an empty string.
> On 2010/07/29 08:05:05, gauravsh wrote:
>> empty string in what? both |data| and |signature| are passed by
> reference
>
> When the function is done, it posts a task to thread |thread_id| that
> calls d->OnKeyOpComplete() with these values as arguments. Updating the
> comment here as well
>
> http://codereview.chromium.org/3058021/diff/3001/4003
> File chrome/browser/chromeos/login/owner_manager_unittest.cc (right):
>
> http://codereview.chromium.org/3058021/diff/3001/4003#newcode26
> chrome/browser/chromeos/login/owner_manager_unittest.cc:26: #include
> "testing/gmock/include/gmock/gmock.h"
> On 2010/07/29 08:05:05, gauravsh wrote:
>> nit: alpha order, gmock goes before gtest :)
>
> Done.
>
> http://codereview.chromium.org/3058021/show