Added auto-enrollment request support to the device_management_backend.

Added auto-enrollment request support to the device_management_backend.

Also extended the testserver's device_management.py handler to serve these
requests.

BUG=chromium-os:23063
TEST=unit_tests and browser_tests all pass


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

[Joao da Silva, 2011-11-30 15:31:19]

Please review, thanks!

[pastarmovj, 2011-12-01 09:30:04]

LGTM with a few minor comments.

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:462: class
DeviceManagementAutoEnrollmentJob : public DeviceManagementJobBase {
A short comment for the class will be good although the name is pretty self
explanatory but still does not reveal what does it really mean to auto-enroll.

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:482: virtual void
OnError(DeviceManagementBackend::ErrorCode error) OVERRIDE {
Don't you want to put some UMA metrics here as well?

http://codereview.chromium.org/8741014/diff/1/net/tools/testserver/device_man...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/8741014/diff/1/net/tools/testserver/device_man...
net/tools/testserver/device_management.py:76: SHA256_0 =
hashlib.sha256('0').digest()
If you only use haslib for this call why don't you just put the string
representation instead (possibly better escaped than what i pasted here :)
"_\xec\xebf\xff\xc8o8\xd9Rxlmily\xc2\xdb\xc29\xddN\x91\xb4g)\xd7:'\xfbW\xe9"

[Joao da Silva, 2011-12-01 09:42:06]

Thanks for the review. Please see comments inline and let me know if you're
still OK with the CL as is now :-)

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:462: class
DeviceManagementAutoEnrollmentJob : public DeviceManagementJobBase {
On 2011/12/01 09:30:04, pastarmovj wrote:
> A short comment for the class will be good although the name is pretty self
> explanatory but still does not reveal what does it really mean to auto-enroll.

Done.

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:482: virtual void
OnError(DeviceManagementBackend::ErrorCode error) OVERRIDE {
On 2011/12/01 09:30:04, pastarmovj wrote:
> Don't you want to put some UMA metrics here as well?

Yes, we want metrics on how auto-enrollment behaves. The metrics will likely be
recorded at the login-flow code side, in a later CL.

http://codereview.chromium.org/8741014/diff/1/net/tools/testserver/device_man...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/8741014/diff/1/net/tools/testserver/device_man...
net/tools/testserver/device_management.py:76: SHA256_0 =
hashlib.sha256('0').digest()
On 2011/12/01 09:30:04, pastarmovj wrote:
> If you only use haslib for this call why don't you just put the string
> representation instead (possibly better escaped than what i pasted here :)
> "_\xec\xebf\xff\xc8o8\xd9Rxlmily\xc2\xdb\xc29\xddN\x91\xb4g)\xd7:'\xfbW\xe9"

I prefer this format since it's less magical, and it's easier to change the
serial number and add more, if the need arises. wdyt?

[pastarmovj, 2011-12-01 09:49:03]

LGTM.

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8741014/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:482: virtual void
OnError(DeviceManagementBackend::ErrorCode error) OVERRIDE {
On 2011/12/01 09:42:07, Joao da Silva wrote:
> On 2011/12/01 09:30:04, pastarmovj wrote:
> > Don't you want to put some UMA metrics here as well?
> 
> Yes, we want metrics on how auto-enrollment behaves. The metrics will likely
be
> recorded at the login-flow code side, in a later CL.

Good.

http://codereview.chromium.org/8741014/diff/1/net/tools/testserver/device_man...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/8741014/diff/1/net/tools/testserver/device_man...
net/tools/testserver/device_management.py:76: SHA256_0 =
hashlib.sha256('0').digest()
On 2011/12/01 09:42:07, Joao da Silva wrote:
> On 2011/12/01 09:30:04, pastarmovj wrote:
> > If you only use haslib for this call why don't you just put the string
> > representation instead (possibly better escaped than what i pasted here :)
> > "_\xec\xebf\xff\xc8o8\xd9Rxlmily\xc2\xdb\xc29\xddN\x91\xb4g)\xd7:'\xfbW\xe9"
> 
> I prefer this format since it's less magical, and it's easier to change the
> serial number and add more, if the need arises. wdyt?

Ok I am fine with that.

[commit-bot: I haz the power, 2011-12-01 09:54:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8741014/5001

[commit-bot: I haz the power, 2011-12-01 09:54:49]

Presubmit check for 8741014-5001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: net/tools/testserver/device_management.py

Presubmit checks took 2.1s to calculate.

[Joao da Silva, 2011-12-01 09:57:44]

@akalin: hey Fred, can you stamp the changes to testserver/device_management.py?
Thanks!

[akalin, 2011-12-02 18:59:51]

I'm actually only in the owners file for that directory for sync stuff (notice
that the owners file doesn't have set noparent).  I looked through the logs, but
I'm not sure who is the right person to OWNERS-review.  passing to willchan?

[willchan no longer on Chromium, 2011-12-02 21:05:08]

I defer to mnissler. I will rubberstamp it if he approves.

[Mattias Nissler (ping if slow), 2011-12-05 09:44:15]

Looks rubberstampable to me :)

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend.h:140: const std::string&
device_id,
Ah, we'll have to make sure that the device_id passed here is thrown away after
the request. Nothing for this CL, but worthwile mentioning.

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.cc:463: // ChromeOS device
should automatically enter the enterprise enrollment screen
nit: ChromiumOS

[Joao da Silva, 2011-12-05 10:00:14]

Thanks for having a look!

@will: can I have a stamp? :-)

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend.h:140: const std::string&
device_id,
On 2011/12/05 09:44:15, Mattias Nissler wrote:
> Ah, we'll have to make sure that the device_id passed here is thrown away
after
> the request. Nothing for this CL, but worthwile mentioning.

Will do.

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.cc:463: // ChromeOS device
should automatically enter the enterprise enrollment screen
On 2011/12/05 09:44:15, Mattias Nissler wrote:
> nit: ChromiumOS

Done.

[willchan no longer on Chromium, 2011-12-05 17:35:51]

LGTM

On Mon, Dec 5, 2011 at 2:00 AM, <joaodasilva@chromium.org> wrote:

> Thanks for having a look!
>
> @will: can I have a stamp? :-)
>
>
>
>
> http://codereview.chromium.**org/8741014/diff/5001/chrome/**
>
browser/policy/device_**management_backend.h<http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device_management_backend.h>
> File chrome/browser/policy/device_**management_backend.h (right):
>
> http://codereview.chromium.**org/8741014/diff/5001/chrome/**
>
browser/policy/device_**management_backend.h#**newcode140<http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device_management_backend.h#newcode140>
> chrome/browser/policy/device_**management_backend.h:140: const
> std::string& device_id,
> On 2011/12/05 09:44:15, Mattias Nissler wrote:
>
>> Ah, we'll have to make sure that the device_id passed here is thrown
>>
> away after
>
>> the request. Nothing for this CL, but worthwile mentioning.
>>
>
> Will do.
>
>
> http://codereview.chromium.**org/8741014/diff/5001/chrome/**
>
browser/policy/device_**management_backend_impl.cc<http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device_management_backend_impl.cc>
> File chrome/browser/policy/device_**management_backend_impl.cc (right):
>
> http://codereview.chromium.**org/8741014/diff/5001/chrome/**
>
browser/policy/device_**management_backend_impl.cc#**newcode463<http://codereview.chromium.org/8741014/diff/5001/chrome/browser/policy/device_management_backend_impl.cc#newcode463>
> chrome/browser/policy/device_**management_backend_impl.cc:**463: //
> ChromeOS
> device should automatically enter the enterprise enrollment screen
> On 2011/12/05 09:44:15, Mattias Nissler wrote:
>
>> nit: ChromiumOS
>>
>
> Done.
>
>
http://codereview.chromium.**org/8741014/<http://codereview.chromium.org/8741...
>

[commit-bot: I haz the power, 2011-12-05 17:42:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8741014/13001

[commit-bot: I haz the power, 2011-12-05 20:30:11]

Change committed as 113009