Public Sessions: fetch device robot api token during enterprise enrollment.

Modify the enterprise enrollment flow to fetch an optional device robot api token from the policy server.  This token will be stored in the device's Local State for use by Cloud Print across all profiles (including Guest and Public Sessions).  The new steps that fetch the token during enrollment are optional, so that enrollment will still succeed if a policy server doesn't support robot api tokens.

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

[David Roche, 2013-03-18 15:23:19]

Mattias, I still need to add tests, but I wanted to get your feedback before I
continue, in case you want some of the logic moved about.

If this is checked in for R27, I'll have to tweak the logic to make the API
token fetching optional during enrollment.

[Mattias Nissler (ping if slow), 2013-03-19 06:33:08]

This looks pretty good already, only a few minor comments.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:262:
UMAFailure(policy::kMetricEnrollmentOtherFailed);
If this should really use the same UMA code (which I doubt) you could move it to
the block below.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:150: // the
system_request_context internally, an manually return the IO thread
grammar

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:152: // does
DeviceManagementRequestContextGetter change?
It uses a different user agent, and disables all cookie handling. In this case,
system_request_context is probably fine. But please do pass that dependency in
for better testability.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:156:
kRobotRefreshTokenFetchRetryCount,
why would we have to retry at all?

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:179: NOTREACHED()
<< "Unexpected callback invoked";
either need to notify the delegate, or do a LOG(FATAL) (i.e. crash)

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:35: //   5. Verify
the policy blob. Everything up to this point doesn't touch device
I think we should first verify the policy blob and then do the OAuth handshake.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:157: //
registratrion, which is covered by OnRegistrationStateChanged above.
*registration

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_client.cc:19: const char
kAnyApiOAuth2Scope[] = "https://www.googleapis.com/auth/any-api";
This should probably go into gaia_constants.h (or whereever the scope strings
are right now).

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_client.cc:280: // TODO: retry on
transient errors?
Not needed for now - the user can retry manually.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:235:
// for users to do anything about?
We thought so too in the first iteration of this code :) It is actually rather
helpful to have separate error messages, so we can handle bug reports, so please
do add two additional messages here.

[David Roche, 2013-04-02 01:59:25]

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:262:
UMAFailure(policy::kMetricEnrollmentOtherFailed);
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> If this should really use the same UMA code (which I doubt) you could move it
to
> the block below.

Added unique UMA code.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:150: // the
system_request_context internally, an manually return the IO thread
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> grammar

Done.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:152: // does
DeviceManagementRequestContextGetter change?
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> It uses a different user agent, and disables all cookie handling. In this
case,
> system_request_context is probably fine. But please do pass that dependency in
> for better testability.

It would make sense for testability if a test depended on injecting a custom
value.  Grep'ing through the code, I can't find an instance of any tests doing
this.  I also based this direct use of
g_browser_process->system_request_context() on the implementation of
device_management_service.cc, which led me to assume that this usage pattern was
okay.

I can directly pass in the dependency if you feel strongly about it, but it
seems like it won't add any additional value?

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:156:
kRobotRefreshTokenFetchRetryCount,
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> why would we have to retry at all?

I retry b/c other bits of enrollment seem to retry, and it appeared that it was
considered a good thing: https://codereview.chromium.org/11572044

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:179: NOTREACHED()
<< "Unexpected callback invoked";
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> either need to notify the delegate, or do a LOG(FATAL) (i.e. crash)

Done.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:35: //   5. Verify
the policy blob. Everything up to this point doesn't touch device
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> I think we should first verify the policy blob and then do the OAuth
handshake.

Right now the validate step also saves the policy and locks the device.  So, the
reason I put the oauth handshake first was so that it could be treated as a
required part of registration before the device is locked.

If I'm to move it after validation, I'd have to break out the locking into
another step that goes after the handshake.

Do you think it's important that this be after verification, or do you dislike
that it's between the policy fetch and verification steps?  An alternative would
be move this before the policy fetch.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:157: //
registratrion, which is covered by OnRegistrationStateChanged above.
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> *registration

Fixed, thx.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_client.cc:19: const char
kAnyApiOAuth2Scope[] = "https://www.googleapis.com/auth/any-api";
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> This should probably go into gaia_constants.h (or whereever the scope strings
> are right now).

Done.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_client.cc:280: // TODO: retry on
transient errors?
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> Not needed for now - the user can retry manually.

Done.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:235:
// for users to do anything about?
On 2013/03/19 06:33:08, Mattias Nissler wrote:
> We thought so too in the first iteration of this code :) It is actually rather
> helpful to have separate error messages, so we can handle bug reports, so
please
> do add two additional messages here.

Okay.  I didn't want to clutter it up with extra messages, but I'd certainly
like to know if this is the step that's failing, so sounds good to me.

Let me know if you have any feedback on the wording of the messages.

[Mattias Nissler (ping if slow), 2013-04-02 14:16:46]

Some more responses / comments.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:152: // does
DeviceManagementRequestContextGetter change?
On 2013/04/02 01:59:25, David Roche wrote:
> On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > It uses a different user agent, and disables all cookie handling. In this
> case,
> > system_request_context is probably fine. But please do pass that dependency
in
> > for better testability.
> 
> It would make sense for testability if a test depended on injecting a custom
> value.  Grep'ing through the code, I can't find an instance of any tests doing
> this.  I also based this direct use of
> g_browser_process->system_request_context() on the implementation of
> device_management_service.cc, which led me to assume that this usage pattern
was
> okay.
> 
> I can directly pass in the dependency if you feel strongly about it, but it
> seems like it won't add any additional value?

OK, tell you what: You make sure the test for this (i.e.
DeviceCloudPolicyManagerChromeOSEnrollmentTest) has proper coverage and passes.
If the reference to the global here is the best way to achieve that, I'm not to
complain :)

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:156:
kRobotRefreshTokenFetchRetryCount,
On 2013/04/02 01:59:25, David Roche wrote:
> On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > why would we have to retry at all?
> 
> I retry b/c other bits of enrollment seem to retry, and it appeared that it
was
> considered a good thing: https://codereview.chromium.org/11572044

That's an entirely separate issue that covers our back when the network config
changes while we're registering or fetching policy. That's not the case here
though, so unless we have a good reason why we need this, my vote is to not
retry - chances are retrying doesn't help in this case.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:35: //   5. Verify
the policy blob. Everything up to this point doesn't touch device
On 2013/04/02 01:59:25, David Roche wrote:
> On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > I think we should first verify the policy blob and then do the OAuth
> handshake.
> 
> Right now the validate step also saves the policy and locks the device.  So,
the
> reason I put the oauth handshake first was so that it could be treated as a
> required part of registration before the device is locked.
> 
> If I'm to move it after validation, I'd have to break out the locking into
> another step that goes after the handshake.

Validation and locking already have their separate steps? Maybe I'm missing
something?

> 
> Do you think it's important that this be after verification, or do you dislike
> that it's between the policy fetch and verification steps?  An alternative
would
> be move this before the policy fetch.

The rationale here is that there might be a policy in the future for configuring
whether we actually want the device-level robot (it surely does have security
implications). If you need the device policy, the first step you can use it is
after policy verification.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:235:
// for users to do anything about?
On 2013/04/02 01:59:25, David Roche wrote:
> On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > We thought so too in the first iteration of this code :) It is actually
rather
> > helpful to have separate error messages, so we can handle bug reports, so
> please
> > do add two additional messages here.
> 
> Okay.  I didn't want to clutter it up with extra messages, but I'd certainly
> like to know if this is the step that's failing, so sounds good to me.
> 
> Let me know if you have any feedback on the wording of the messages.

We'll have to tell chrome-ui-review@ about the strings. I have a long thread
about enrollment error messages with Glen already that didn't come to a
conclusion :)

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:146: // Use the
system request context to avoid sending user cookies.
Is that actually accurate? The system request context does have a cookie store.

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc:94: void
CloudPolicyRefreshScheduler::OnRobotAuthCodesFetched(
How about we make this an optional override by providing an empty default
implementation?

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/proto/device_management_backend.proto (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/proto/device_management_backend.proto:492: optional
DeviceServiceApiAccessRequest service_api_access_request = 7;
note that the server version already has used this tag for a different purpose,
can you make sure we don't collide?

[David Roche, 2013-04-04 01:39:53]

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:152: // does
DeviceManagementRequestContextGetter change?
On 2013/04/02 14:16:46, Mattias Nissler wrote:
> On 2013/04/02 01:59:25, David Roche wrote:
> > On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > > It uses a different user agent, and disables all cookie handling. In this
> > case,
> > > system_request_context is probably fine. But please do pass that
dependency
> in
> > > for better testability.
> > 
> > It would make sense for testability if a test depended on injecting a custom
> > value.  Grep'ing through the code, I can't find an instance of any tests
doing
> > this.  I also based this direct use of
> > g_browser_process->system_request_context() on the implementation of
> > device_management_service.cc, which led me to assume that this usage pattern
> was
> > okay.
> > 
> > I can directly pass in the dependency if you feel strongly about it, but it
> > seems like it won't add any additional value?
> 
> OK, tell you what: You make sure the test for this (i.e.
> DeviceCloudPolicyManagerChromeOSEnrollmentTest) has proper coverage and
passes.
> If the reference to the global here is the best way to achieve that, I'm not
to
> complain :)

Ah, yes, updating tests.  That is on the agenda for tomorrow.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:156:
kRobotRefreshTokenFetchRetryCount,
On 2013/04/02 14:16:46, Mattias Nissler wrote:
> On 2013/04/02 01:59:25, David Roche wrote:
> > On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > > why would we have to retry at all?
> > 
> > I retry b/c other bits of enrollment seem to retry, and it appeared that it
> was
> > considered a good thing: https://codereview.chromium.org/11572044
> 
> That's an entirely separate issue that covers our back when the network config
> changes while we're registering or fetching policy. That's not the case here
> though, so unless we have a good reason why we need this, my vote is to not
> retry - chances are retrying doesn't help in this case. 

Done.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:235:
// for users to do anything about?
On 2013/04/02 14:16:46, Mattias Nissler wrote:
> On 2013/04/02 01:59:25, David Roche wrote:
> > On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > > We thought so too in the first iteration of this code :) It is actually
> rather
> > > helpful to have separate error messages, so we can handle bug reports, so
> > please
> > > do add two additional messages here.
> > 
> > Okay.  I didn't want to clutter it up with extra messages, but I'd certainly
> > like to know if this is the step that's failing, so sounds good to me.
> > 
> > Let me know if you have any feedback on the wording of the messages.
> 
> We'll have to tell chrome-ui-review@ about the strings. I have a long thread
> about enrollment error messages with Glen already that didn't come to a
> conclusion :)

How should I go about this?  Should I start a new thread, or continue the one
you have with Glen?

Should we somehow separate the strings issue from this CL, perhaps by reverting
to the generic error message, and pushing the strings into another CL?  I'd
rather see the code checked in soon, so end-to-end testing can be easily
started, than have the code get old while we iterate on the details of two error
strings.

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:146: // Use the
system request context to avoid sending user cookies.
On 2013/04/02 14:16:46, Mattias Nissler wrote:
> Is that actually accurate? The system request context does have a cookie
store.

I think it is, from my reading of the code.  The important distinction, I think,
is that we're not sending the _user_ cookies for the enrolling user.

In IOThread::Init(), I see
  // In-memory cookie store.
  globals_->system_cookie_store = new net::CookieMonster(NULL, NULL);

.. so it looks as if it's using a short-lived, in-memory cookie store that is
not associated with any user, and dies when the browser shuts down.

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc:94: void
CloudPolicyRefreshScheduler::OnRobotAuthCodesFetched(
On 2013/04/02 14:16:46, Mattias Nissler wrote:
> How about we make this an optional override by providing an empty default
> implementation?

Done.

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/proto/device_management_backend.proto (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/proto/device_management_backend.proto:492: optional
DeviceServiceApiAccessRequest service_api_access_request = 7;
On 2013/04/02 14:16:46, Mattias Nissler wrote:
> note that the server version already has used this tag for a different
purpose,
> can you make sure we don't collide?

Done.  Will field #7 be added to the client proto?  If not, and it is
server-only, it should start at 100 to make keeping these in sync easier....

[Mattias Nissler (ping if slow), 2013-04-04 13:18:34]

Answers to your comments, next step is to fix tests I guess.

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/12538009/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:235:
// for users to do anything about?
On 2013/04/04 01:39:53, David Roche wrote:
> On 2013/04/02 14:16:46, Mattias Nissler wrote:
> > On 2013/04/02 01:59:25, David Roche wrote:
> > > On 2013/03/19 06:33:08, Mattias Nissler wrote:
> > > > We thought so too in the first iteration of this code :) It is actually
> > rather
> > > > helpful to have separate error messages, so we can handle bug reports,
so
> > > please
> > > > do add two additional messages here.
> > > 
> > > Okay.  I didn't want to clutter it up with extra messages, but I'd
certainly
> > > like to know if this is the step that's failing, so sounds good to me.
> > > 
> > > Let me know if you have any feedback on the wording of the messages.
> > 
> > We'll have to tell chrome-ui-review@ about the strings. I have a long thread
> > about enrollment error messages with Glen already that didn't come to a
> > conclusion :)
> 
> How should I go about this?  Should I start a new thread, or continue the one
> you have with Glen?
> 
> Should we somehow separate the strings issue from this CL, perhaps by
reverting
> to the generic error message, and pushing the strings into another CL?  I'd
> rather see the code checked in soon, so end-to-end testing can be easily
> started, than have the code get old while we iterate on the details of two
error
> strings.

In this case (only error messsages) I think it's OK to just commit and then send
a mail saying "btw. there are new strings here". We can then fix up the strings
when changes are requested.

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:146: // Use the
system request context to avoid sending user cookies.
On 2013/04/04 01:39:53, David Roche wrote:
> On 2013/04/02 14:16:46, Mattias Nissler wrote:
> > Is that actually accurate? The system request context does have a cookie
> store.
> 
> I think it is, from my reading of the code.  The important distinction, I
think,
> is that we're not sending the _user_ cookies for the enrolling user.
> 
> In IOThread::Init(), I see
>   // In-memory cookie store.
>   globals_->system_cookie_store = new net::CookieMonster(NULL, NULL);
> 
> .. so it looks as if it's using a short-lived, in-memory cookie store that is
> not associated with any user, and dies when the browser shuts down.

You're right, I didn't missed the "user" part in the sentence.

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/proto/device_management_backend.proto (right):

https://codereview.chromium.org/12538009/diff/14001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/proto/device_management_backend.proto:492: optional
DeviceServiceApiAccessRequest service_api_access_request = 7;
On 2013/04/04 01:39:53, David Roche wrote:
> On 2013/04/02 14:16:46, Mattias Nissler wrote:
> > note that the server version already has used this tag for a different
> purpose,
> > can you make sure we don't collide?
> 
> Done.  Will field #7 be added to the client proto?  If not, and it is
> server-only, it should start at 100 to make keeping these in sync easier....

Field 7 will appear on the Chromium side as well, in fact there's a pending CL
that adds it.

[David Roche, 2013-04-22 04:10:00]

Mattias,

Tests updated, PTAL.

[Mattias Nissler (ping if slow), 2013-04-22 10:59:00]

Looks like you need to rebase.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:436:
}
We should probably have a couple test cases that check whether failed auth token
fetches behave as expected.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:153:
enrollment_step_ = STEP_STORE_ROBOT_AUTH;
Is this used anywhere? If not, just remove that step constant.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:31: //      it.
away.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:53: friend class
policy::test::DeviceCloudPolicyManagerChromeOSEnrollmentTest;
Is there anything wrong with making GetRefreshToken() public so we can avoid
these friend declarations?

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:31:
friend class policy::test::DeviceCloudPolicyManagerChromeOSTest;
I'd jus tmake Initialize() and Shutdown() public.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_test_helper.h:126: PolicyState
policy_state_;
I see where you're coming from, but if you feel strong to rename this, then
please do add a qualifier to indicate this is device policy (as opposed to
device-local account policy below).

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.h:57: // returns successfully. 
Only occurs during enrollment.  Optional
nit: no 2 spaces after period in this file.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.h:59: virtual void
OnRobotAuthCodesFetched(CloudPolicyClient* client);
"auth codes" here vs. "auth tokens" below - please consolidate.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/tes...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/tes...
chrome/browser/policy/test/policy_testserver.py:310:
dm.DeviceServiceApiAccessResponse())
Don't you need to put an access code string here to make the tests pass?

[David Roche, 2013-04-23 01:36:08]

I'll add the additional failure tests tomorrow.

I'll also rebase tomorrow, in a separate patchset upload so as not to clutter up
your review like I did last time (sorry about that).

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:153:
enrollment_step_ = STEP_STORE_ROBOT_AUTH;
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> Is this used anywhere? If not, just remove that step constant.

Done.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:31: //      it.
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> away.

Oops, should have moved down with other lines.   Thanks for catching.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:53: friend class
policy::test::DeviceCloudPolicyManagerChromeOSEnrollmentTest;
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> Is there anything wrong with making GetRefreshToken() public so we can avoid
> these friend declarations?

I guess not.  It seemed like clients of these classes "shouldn't" call
GetRefreshToken (or Initialize/Shutdown on the factory), but perhaps it's not
worth the hoops and extra LOC to keep them protected.  The only downside is that
any code could call Shutdown on the factory and cause it to return null to other
clients, but I suppose that sort of thing will quickly fail in testing anyway
(and hopefully code review too).

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:31:
friend class policy::test::DeviceCloudPolicyManagerChromeOSTest;
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> I'd jus tmake Initialize() and Shutdown() public.

Done.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_test_helper.h:126: PolicyState
policy_state_;
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> I see where you're coming from, but if you feel strong to rename this, then
> please do add a qualifier to indicate this is device policy (as opposed to
> device-local account policy below).

I was actually confused by the fact that there was another device_policy_ in
DeviceSettingsTestBase below, that was of a different type.  Perhaps a better
thing to do is just to rename that other variable to device_policy_builder_ and
leave this as-is, so it remains consistent with the method names that manipulate
it, etc.  I won't do that in this CL though, since the device_policy_ member
below is used in ~100 places in 6 files, so it would really clutter up this CL
with confusing, unrelated diffs.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.h:57: // returns successfully. 
Only occurs during enrollment.  Optional
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> nit: no 2 spaces after period in this file.

That's not entirely true.  UploadCertificate() and StatusCallback use
double-space.  So, I was being consistently inconsistent, with a preference to
spacing The Right Way (tm) when in doubt.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.h:59: virtual void
OnRobotAuthCodesFetched(CloudPolicyClient* client);
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> "auth codes" here vs. "auth tokens" below - please consolidate.

Ah, thanks for noticing this.  Per the OAuth2 docs, the sequence should be

auth code
refresh token
access token

I definitely want to be precise in the naming throughout.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/tes...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/tes...
chrome/browser/policy/test/policy_testserver.py:310:
dm.DeviceServiceApiAccessResponse())
On 2013/04/22 10:59:00, Mattias Nissler wrote:
> Don't you need to put an access code string here to make the tests pass?

Done.

[Mattias Nissler (ping if slow), 2013-04-23 13:15:12]

Looks good thus far, will take another look after rebase / adding tests.

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/12538009/diff/35001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.h:57: // returns successfully. 
Only occurs during enrollment.  Optional
On 2013/04/23 01:36:08, David Roche wrote:
> On 2013/04/22 10:59:00, Mattias Nissler wrote:
> > nit: no 2 spaces after period in this file.
> 
> That's not entirely true.  UploadCertificate() and StatusCallback use
> double-space.  So, I was being consistently inconsistent, with a preference to
> spacing The Right Way (tm) when in doubt.

Ha, so you're saying I should have payed more attention in the other review due
to which these slipped in recently? ;)

Regarding "The Right Way", note that the style guide doesn't seem to cover this
and authorities don't provide conclusive guidance either:
http://en.wikipedia.org/wiki/Sentence_spacing. Anyhow, I'll not spend too much
time arguing this :)

[David Roche, 2013-04-24 02:44:10]

Added test cases for the auth code and refresh token fetch failures.  Currently
the auth fetches need to be optional since the prod DMServer doesn't support the
api method yet.  We should also discuss whether we'll want it to stay optional
for M28, or be required from the get-go.

Tomorrow I'll manually test enrollment against the test server and a test
instance of DMServer.

[Mattias Nissler (ping if slow), 2013-04-24 13:43:13]

Thinking more about this, I'm not really convinced the auth token should become
a hard requirement. The reason here is that potential 3rd-party implementations
of the protocol will have a hard time supplying valid auth tokens. 
Also, forcing our test server implementations return fake tokens would probably
do more harm than good as well. Maybe we want to include a flag in the register
response or the policy that indicates we should do auth token fetching?

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:327:
// its num_retries_ > source->GetMaxRetriesOn5xx() should have a >=?
You shouldn't just dump this in a comment, but pass this on to the author and/or
file a bug :)

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:420:
// producion, thes next 4 tests will be changed.
production
the

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:420:
// producion, thes next 4 tests will be changed.
Maybe avoid Google-isms like "production" and "DMServer" here? Something like
"Policy server implementations are not required support robot auth tokens, so
allow enrollment to succeed if the robot auth token fetch fails."

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:140: // token and
jump directly to the lock device step.
same here.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:114: void
WriteInstallAttributesAndLockDevice();
The naming is a bit unfortunate here, WriteInstallAttributes actually does lock
the device. Maybe rename to InitiateDeviceLock or something else?

[David Roche, 2013-04-24 15:34:21]

I'll discuss with Bin about adding a must-get-auth-token flag to the register
response.  I think that can be added in a subsequent CL, and it left purely
optional, for now.

Note that I added a CHECK_EQ(STEP_LOCK_DEVICE) to HandleLockDeviceResult in this
patch -- I think this is correct/safe.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:327:
// its num_retries_ > source->GetMaxRetriesOn5xx() should have a >=?
On 2013/04/24 13:43:13, Mattias Nissler wrote:
> You shouldn't just dump this in a comment, but pass this on to the author
and/or
> file a bug :)

I added this to my todo list to do in another CL.  Sorry, I was going to mention
that in the codereview mail and I forgot.

I just created another branch with the retry fix in it, so I won't forget to
follow up on this (can't stand to have old branches with changes in them lying
about forever).  I'm running the unit tests now and will ping an OWNER to see if
they're worried about changing this accidental default from retrying to
[properly] not retrying.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:420:
// producion, thes next 4 tests will be changed.
On 2013/04/24 13:43:13, Mattias Nissler wrote:
> Maybe avoid Google-isms like "production" and "DMServer" here? Something like
> "Policy server implementations are not required support robot auth tokens, so
> allow enrollment to succeed if the robot auth token fetch fails."

Done.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:140: // token and
jump directly to the lock device step.
On 2013/04/24 13:43:13, Mattias Nissler wrote:
> same here.

I can't puzzle out what "same here" means in this context.  Your other context
are about google-isms and method naming.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:114: void
WriteInstallAttributesAndLockDevice();
On 2013/04/24 13:43:13, Mattias Nissler wrote:
> The naming is a bit unfortunate here, WriteInstallAttributes actually does
lock
> the device. Maybe rename to InitiateDeviceLock or something else?

I renamed this and WriteInstallAttributes, so that the step enym name
(STEP_LOCK_DEVICE) and the various method names are consistent.  There are now:

DoLockDeviceStep (flip state machine enum and begin step)
StartLockDevice (start async activity, similar to
StartEnrollment/StartValidation)
HandleLockDeviceResponse

Let me know what you think.

[Mattias Nissler (ping if slow), 2013-04-24 17:58:52]

All right, this LGTM pending getting an answer on the GaiaOAuthClient question
and updating the corresponding comment accordingly.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:327:
// its num_retries_ > source->GetMaxRetriesOn5xx() should have a >=?
On 2013/04/24 15:34:21, David Roche wrote:
> On 2013/04/24 13:43:13, Mattias Nissler wrote:
> > You shouldn't just dump this in a comment, but pass this on to the author
> and/or
> > file a bug :)
> 
> I added this to my todo list to do in another CL.  Sorry, I was going to
mention
> that in the codereview mail and I forgot.
> 
> I just created another branch with the retry fix in it, so I won't forget to
> follow up on this (can't stand to have old branches with changes in them lying
> about forever).  I'm running the unit tests now and will ping an OWNER to see
if
> they're worried about changing this accidental default from retrying to
> [properly] not retrying.

Can you just ask the author of that line whether it's wrong or not and update
the comment accordingly here?

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:140: // token and
jump directly to the lock device step.
On 2013/04/24 15:34:21, David Roche wrote:
> On 2013/04/24 13:43:13, Mattias Nissler wrote:
> > same here.
> 
> I can't puzzle out what "same here" means in this context.  Your other context
> are about google-isms and method naming.

I'm not sure either? Maybe this is an artifact of another comment I removed.
Please disregard.

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/12538009/diff/68001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:114: void
WriteInstallAttributesAndLockDevice();
On 2013/04/24 15:34:21, David Roche wrote:
> On 2013/04/24 13:43:13, Mattias Nissler wrote:
> > The naming is a bit unfortunate here, WriteInstallAttributes actually does
> lock
> > the device. Maybe rename to InitiateDeviceLock or something else?
> 
> I renamed this and WriteInstallAttributes, so that the step enym name
> (STEP_LOCK_DEVICE) and the various method names are consistent.  There are
now:
> 
> DoLockDeviceStep (flip state machine enum and begin step)
> StartLockDevice (start async activity, similar to
> StartEnrollment/StartValidation)
> HandleLockDeviceResponse
> 
> Let me know what you think.

Looks good.

https://codereview.chromium.org/12538009/diff/79001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/12538009/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:419:
// Policy server implementations are not required support robot auth tokens,
required _to_

[Mattias Nissler (ping if slow), 2013-04-24 18:00:43]

One last thing: Can you update the CL description to say "Public Sessions"
(instead of "Public Accounts"), and can you elaborate a bit on what code changes
this CL makes (so if there's tree breakage, sheriffs get a first idea whether
the CL is a suspect or not).

[David Roche, 2013-04-27 17:27:03]

+Zel for approval of an additional gaia_constants value for the any_api scope.

[zel, 2013-04-28 19:17:01]

lgtm

[commit-bot: I haz the power, 2013-04-29 13:54:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/12538009/84002

[commit-bot: I haz the power, 2013-04-29 17:51:55]

Change committed as 197075