Minimal attestation-based enrollment flow.

A minimal attestation-based enrollment flow during OOBE.

If --enterprise-enable-zero-touch-enrollment is present on the command line, attestation-based will be tried. If it fails, the user will be allowed to fall back to the regular flow unless the flag was passed as --enterprise-enable-zero-touch-enrollment=forced, in which case they cannot exit from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the system or not, the user who exits from the attestation-based enrollment flow will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a registration certificate. An error is forced at that point until we also wire in the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0f9078a5de400aee06acfa4787e41222b51fae16
Cr-Commit-Position: refs/heads/master@{#413992}

[The one and only Dr. Crash, 2016-07-26 16:37:23]

Description was changed from

==========
Beginning of the attestation-based enrollment flow.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
==========

to

==========
Beginning of the attestation-based enrollment flow.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
==========

[The one and only Dr. Crash, 2016-07-26 16:37:23]

drcrash@chromium.org changed reviewers:
+ mnissler@chromium.org, tnagel@chromium.org

[The one and only Dr. Crash, 2016-07-27 10:53:06]

Please have a look. Thanks!

[The one and only Dr. Crash, 2016-07-27 16:29:33]

Patchset #3 (id:40001) has been deleted

[The one and only Dr. Crash, 2016-08-09 08:19:55]

Description was changed from

==========
Beginning of the attestation-based enrollment flow.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
==========

to

==========
Beginning of the attestation-based enrollment flow.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Thiemo Nagel, 2016-08-11 08:58:07]

Could you please do a rebase once the other CL has landed?  Also it seems that
commit message needs fixing. (There's too much "In progress".)

[The one and only Dr. Crash, 2016-08-16 00:57:06]

Description was changed from

==========
Beginning of the attestation-based enrollment flow.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Beginning of the attestation-based enrollment flow during OOBE.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[The one and only Dr. Crash, 2016-08-16 01:11:35]

Description was changed from

==========
Beginning of the attestation-based enrollment flow during OOBE.

Very much a work in progress and just some thoughts to get some
initial feedback.

BUG=624187
TEST=none

In progress.


In progress.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Beginning of the attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

Additionally, wires in the call to the attestation flow class to request an
enrollment certificate.

BUG=624187
TEST=none

In progress.


In progress.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[The one and only Dr. Crash, 2016-08-17 19:59:19]

Description was changed from

==========
Beginning of the attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

Additionally, wires in the call to the attestation flow class to request an
enrollment certificate.

BUG=624187
TEST=none

In progress.


In progress.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Beginning of the attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

Additionally, wires in the call to the attestation flow class to request an
enrollment certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[The one and only Dr. Crash, 2016-08-17 20:02:04]

drcrash@chromium.org changed reviewers:
+ achuith@chromium.org

[The one and only Dr. Crash, 2016-08-17 20:03:30]

Flow is complete and I added browser tests.

[The one and only Dr. Crash, 2016-08-17 21:02:46]

Description was changed from

==========
Beginning of the attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

Additionally, wires in the call to the attestation flow class to request an
enrollment certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
A minimal attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

Additionally, wires in the call to the attestation flow class to request an
enrollment certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[The one and only Dr. Crash, 2016-08-18 04:22:43]

After the most recent changes I made to allow ZTE to work whether
interactive enrollment is a fallback or not (i.e. if it's not, then a login
screen will be displayed and not an enrollment one), I think that it would
be cleaner, instead of having an auth_mechanism and a mode, to have an
attestation_mode and an oauth_mode (both of which having some sort of NONE
option). However, I'd like to defer that to another CL.

On Wed, Aug 17, 2016 at 9:17 PM, <drcrash@chromium.org> wrote:

> https://codereview.chromium.org/2186623002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[The one and only Dr. Crash, 2016-08-18 17:20:35]

drcrash@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[The one and only Dr. Crash, 2016-08-18 18:18:07]

Description was changed from

==========
A minimal attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

Additionally, wires in the call to the attestation flow class to request an
enrollment certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
A minimal attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

If --enterprise-enable-zero-touch-enrollment is present, attestation-based will
be tried. If it fails, the user will be allowed to fall back to the regular flow
unless the flag was passed the value "forced" in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[The one and only Dr. Crash, 2016-08-18 18:19:53]

Description was changed from

==========
A minimal attestation-based enrollment flow during OOBE.

This provides a UX flow driven by the --enterprise-enable-zero-touch-enrollment
flag that will try attestation-based enrollment first, and, depending on whether
zero-touch is forced or not, will block there in case of error or allow the user
to proceed to the next authentication for enrollment (i.e. enterprise enrollment
or login).

If --enterprise-enable-zero-touch-enrollment is present, attestation-based will
be tried. If it fails, the user will be allowed to fall back to the regular flow
unless the flag was passed the value "forced" in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
A minimal attestation-based enrollment flow during OOBE.

If --enterprise-enable-zero-touch-enrollment is present on the command line,
attestation-based will be tried. If it fails, the user will be allowed to fall
back to the regular flow unless the flag was passed as
--enterprise-enable-zero-touch-enrollment=forced, in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[pastarmovj, 2016-08-19 10:29:18]

some comments from me, but I think Thiemo should give the final approval seal
because I haven't touched that code for too long now.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:112: if
(current_auth_ == last_auth_) {
This looks like too much of a boilerplate for juggling only 2 cases. Are you
expecting this to grow more in the near future? If not I would embed the
knowledge of that in the logic directly for now. E.g.:

if (current_auth_ != last_auth_ && current_auth_ == AUTH_ATTESTATION) {
  current_auth_ = AUTH_OAUTH;
  if (enrollment_config_.should_enroll_interactively()) {
    SetConfig();
    return true;
  }
}
return false;

And if you plan for more then maybe having an array of the options would be a
better idea in that case.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:112:
DCHECK(enrollment_config_.mode ==
I wonder if this should not even be a CHECK. Could a bug cause this function to
be called with the wrong mode and cause the wrong type of enrollment? I am not
sure how much of an issue this really is but better safe then sorry :)

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.cc:1395: (force_interactive &&
!effective_config.should_enroll_interactively())) {
Can't quite follow the logic here - why do you want force_iteractive to be true
but at the same time should_enroll_interactively to be false?

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.h:336: //
|prescribed_enrollment_config_|.
Please document the new flag.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:128: switch
(zte_mode) {
Seems like this change can be undone you don't use the zte_mode var anymore.

[The one and only Dr. Crash, 2016-08-19 17:49:30]

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:112: if
(current_auth_ == last_auth_) {
On 2016/08/19 10:29:18, pastarmovj wrote:
> This looks like too much of a boilerplate for juggling only 2 cases. Are you
> expecting this to grow more in the near future? If not I would embed the
> knowledge of that in the logic directly for now. E.g.:
> 
> if (current_auth_ != last_auth_ && current_auth_ == AUTH_ATTESTATION) {
>   current_auth_ = AUTH_OAUTH;
>   if (enrollment_config_.should_enroll_interactively()) {
>     SetConfig();
>     return true;
>   }
> }
> return false;
> 
> And if you plan for more then maybe having an array of the options would be a
> better idea in that case.

I like that.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:112:
DCHECK(enrollment_config_.mode ==
On 2016/08/19 10:29:18, pastarmovj wrote:
> I wonder if this should not even be a CHECK. Could a bug cause this function
to
> be called with the wrong mode and cause the wrong type of enrollment? I am not
> sure how much of an issue this really is but better safe then sorry :)

Sounds reasonable.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.cc:1395: (force_interactive &&
!effective_config.should_enroll_interactively())) {
On 2016/08/19 10:29:18, pastarmovj wrote:
> Can't quite follow the logic here - why do you want force_iteractive to be
true
> but at the same time should_enroll_interactively to be false?

effective_config is set from environmental factors by the DCP initializer. So
that represents what the device thinks it should do. The force_interactive flag,
on the other hand, represents that the user has asked for enrollment by typing
CTRL+ALT+E. So if you take an out of the box Chromebook or Chromebox for
example, effective_config.should_enroll_interactively() will be false (and
therefore we would get there only if the user typed CTRL+ALT+E). If you take
anything that sets a requisition through the OEM partition or other means it, or
if the AutoEnrollmentScreen has determined that one needs to re-enroll forcibly
for example, it will be true.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.h:336: //
|prescribed_enrollment_config_|.
On 2016/08/19 10:29:18, pastarmovj wrote:
> Please document the new flag.

Done.

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2186623002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:128: switch
(zte_mode) {
On 2016/08/19 10:29:18, pastarmovj wrote:
> Seems like this change can be undone you don't use the zte_mode var anymore.

Yes.

[achuithb, 2016-08-19 18:44:25]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 18:44:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 18:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 18:51:30]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[The one and only Dr. Crash, 2016-08-21 20:24:25]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 20:24:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-21 20:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-21 20:33:37]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pastarmovj, 2016-08-22 11:44:15]

lgtm

[The one and only Dr. Crash, 2016-08-22 14:42:03]

The CQ bit was checked by drcrash@chromium.org

[commit-bot: I haz the power, 2016-08-22 14:49:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 14:58:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 14:58:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[The one and only Dr. Crash, 2016-08-22 15:36:45]

On 2016/08/22 11:44:15, pastarmovj wrote:
> lgtm

Thanks. FIY I have extracted the changes to the .js file that gjslint doesn't
like into a tiny CL (https://codereview.chromium.org/2265163002/) to land by
hand, so I can then use the CQ for this one. There will be another patchset to
merge that CL in after it's submitted.

[The one and only Dr. Crash, 2016-08-23 15:17:39]

+achuithb, could you have a look at the CL?

[achuithb, 2016-08-23 18:16:45]

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:65:
shark_controller_(NULL),
We prefer in-class initialization. Also, change this NULL to nullptr

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:112: if
(current_auth_ != last_auth_ && current_auth_ == AUTH_ATTESTATION) {
I would reverse this and do an early exit with return false

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:155: }
Add default with NOTREACHED() to future proof

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:327: // 
TODO(drcrash): Maybe create multiple metrics for attestation vs oauth?
File a bug and reference it here.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:149:
EnrollmentScreenActor* actor_;
These should be initialized here.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_browsertest.cc
(right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_browsertest.cc:100:
ASSERT_TRUE(WizardController::default_controller() != NULL);
either nullptr, or drop the comparison. Here and below.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:75:
oauth_data_cleared_(false),
Initialize in header

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:106:
DCHECK(oauth_status_ != OAUTH_STARTED_WITH_TOKEN);
Can we have a positive check here instead?
DCHECK(oauth_status == OAUTH_NOT_STARTED || oauth_status ==
OAUTH_STARTED_WITH_AUTHCODE);

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:107:
if (oauth_status_ == OAUTH_NOT_STARTED) {
Drop {}

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:116:
policy::EnrollmentConfig::MODE_ATTESTATION_FORCED);
DCHECK for oauth_status_ here?

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:147:
DCHECK(token == oauth_token_ || oauth_token_.empty());
Do we need a DCHECK for oauth_status_?

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:156:
if (oauth_status_ != OAUTH_NOT_STARTED) {
drop {}

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:240:
if (oauth_status_ != OAUTH_NOT_STARTED) {
Drop {}

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h
(right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:81:
bool fetch_additional_token_;
Initialize these here.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.cc:1388: VLOG(1) << "Showing
enrollment screen.";
Add the boolean to logging

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_config.h (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_config.h:57: bool should_enroll()
const {
Why are these underscore_case? Shouldn't they be CamelCase?

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:292:
chromeos::attestation::AttestationFlow::CertificateCallback callback =
const

[The one and only Dr. Crash, 2016-08-23 21:24:19]

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:65:
shark_controller_(NULL),
On 2016/08/23 18:16:44, achuithb wrote:
> We prefer in-class initialization. Also, change this NULL to nullptr

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:155: }
On 2016/08/23 18:16:44, achuithb wrote:
> Add default with NOTREACHED() to future proof

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:327: // 
TODO(drcrash): Maybe create multiple metrics for attestation vs oauth?
On 2016/08/23 18:16:44, achuithb wrote:
> File a bug and reference it here. 

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:149:
EnrollmentScreenActor* actor_;
On 2016/08/23 18:16:44, achuithb wrote:
> These should be initialized here.
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_browsertest.cc
(right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_browsertest.cc:100:
ASSERT_TRUE(WizardController::default_controller() != NULL);
On 2016/08/23 18:16:44, achuithb wrote:
> either nullptr, or drop the comparison. Here and below.

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:75:
oauth_data_cleared_(false),
On 2016/08/23 18:16:44, achuithb wrote:
> Initialize in header

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:106:
DCHECK(oauth_status_ != OAUTH_STARTED_WITH_TOKEN);
On 2016/08/23 18:16:44, achuithb wrote:
> Can we have a positive check here instead?
> DCHECK(oauth_status == OAUTH_NOT_STARTED || oauth_status ==
> OAUTH_STARTED_WITH_AUTHCODE);

That's more fragile if the enum gets extended than the one I have right now,
which is why I picked that way.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:116:
policy::EnrollmentConfig::MODE_ATTESTATION_FORCED);
On 2016/08/23 18:16:44, achuithb wrote:
> DCHECK for oauth_status_ here?

No. The attestation enrollment is independent and the oauth_status_ only track
OAuth enrollment.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:147:
DCHECK(token == oauth_token_ || oauth_token_.empty());
Added one.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:156:
if (oauth_status_ != OAUTH_NOT_STARTED) {
On 2016/08/23 18:16:44, achuithb wrote:
> drop {}

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:240:
if (oauth_status_ != OAUTH_NOT_STARTED) {
On 2016/08/23 18:16:44, achuithb wrote:
> Drop {}

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.cc:1388: VLOG(1) << "Showing
enrollment screen.";
On 2016/08/23 18:16:45, achuithb wrote:
> Add the boolean to logging

Done.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_config.h (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_config.h:57: bool should_enroll()
const {
On 2016/08/23 18:16:45, achuithb wrote:
> Why are these underscore_case? Shouldn't they be CamelCase?

Because is_forced() was and I am respecting the style of the file. I am fine
ChangingItToCamelCase.

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2186623002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:292:
chromeos::attestation::AttestationFlow::CertificateCallback callback =
On 2016/08/23 18:16:45, achuithb wrote:
> const

Done.

[achuithb, 2016-08-23 21:26:16]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[achuithb, 2016-08-23 21:26:33]

lgtm

[commit-bot: I haz the power, 2016-08-23 21:27:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 22:16:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 22:16:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[The one and only Dr. Crash, 2016-08-24 05:16:42]

The CQ bit was checked by drcrash@chromium.org

[The one and only Dr. Crash, 2016-08-24 05:16:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pastarmovj@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2186623002/#ps240001
(title: "Addressed achuithb's feedback.")

[commit-bot: I haz the power, 2016-08-24 05:17:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 06:13:20]

Description was changed from

==========
A minimal attestation-based enrollment flow during OOBE.

If --enterprise-enable-zero-touch-enrollment is present on the command line,
attestation-based will be tried. If it fails, the user will be allowed to fall
back to the regular flow unless the flag was passed as
--enterprise-enable-zero-touch-enrollment=forced, in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
A minimal attestation-based enrollment flow during OOBE.

If --enterprise-enable-zero-touch-enrollment is present on the command line,
attestation-based will be tried. If it fails, the user will be allowed to fall
back to the regular flow unless the flag was passed as
--enterprise-enable-zero-touch-enrollment=forced, in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-24 06:13:22]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-08-24 06:14:50]

Description was changed from

==========
A minimal attestation-based enrollment flow during OOBE.

If --enterprise-enable-zero-touch-enrollment is present on the command line,
attestation-based will be tried. If it fails, the user will be allowed to fall
back to the regular flow unless the flag was passed as
--enterprise-enable-zero-touch-enrollment=forced, in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
A minimal attestation-based enrollment flow during OOBE.

If --enterprise-enable-zero-touch-enrollment is present on the command line,
attestation-based will be tried. If it fails, the user will be allowed to fall
back to the regular flow unless the flag was passed as
--enterprise-enable-zero-touch-enrollment=forced, in which case they cannot exit
from the attestation-based enrollment unless it is successful.

Depending on whether enrollment was requested by the user (CTRL+ALT+E) or the
system or not, the user who exits from the attestation-based enrollment flow
will be presented the enrollment screen or the login screen, as before.

Additionally, wires in the call to the attestation flow class to request a
registration certificate. An error is forced at that point until we also wire in
the new DM server registration call needed to use the certificate.

BUG=624187
TEST=browser tests

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0f9078a5de400aee06acfa4787e41222b51fae16
Cr-Commit-Position: refs/heads/master@{#413992}
==========

[commit-bot: I haz the power, 2016-08-24 06:14:51]

Patchset 12 (id:??) landed as
https://crrev.com/0f9078a5de400aee06acfa4787e41222b51fae16
Cr-Commit-Position: refs/heads/master@{#413992}