Minimal attestation-based enrollment flow.

chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc

Index: chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
diff --git a/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc b/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
index baec231eacd00abba756fe05d085c03b2f312396..3204d3bc9206381c43f94e8bd9f9dea605f4d602 100644
--- a/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
+++ b/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
@@ -72,10 +72,8 @@ EnterpriseEnrollmentHelperImpl::EnterpriseEnrollmentHelperImpl(
     : EnterpriseEnrollmentHelper(status_consumer),
       enrollment_config_(enrollment_config),
       enrolling_user_domain_(enrolling_user_domain),
-      started_(false),
-      finished_(false),
+      oauth_data_cleared_(false),

[achuithb, 2016/08/23 18:16:44]

Initialize in header

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

Done.

       success_(false),
-      auth_data_cleared_(false),
       weak_ptr_factory_(this) {
   // Init the TPM if it has not been done until now (in debug build we might
   // have not done that yet).
@@ -84,15 +82,17 @@ EnterpriseEnrollmentHelperImpl::EnterpriseEnrollmentHelperImpl(
 }
 
 EnterpriseEnrollmentHelperImpl::~EnterpriseEnrollmentHelperImpl() {
-  DCHECK(g_browser_process->IsShuttingDown() || !started_ ||
-         (finished_ && (success_ || auth_data_cleared_)));
+  DCHECK(
+      g_browser_process->IsShuttingDown() ||
+      oauth_status_ == OAUTH_NOT_STARTED ||
+      (oauth_status_ == OAUTH_FINISHED && (success_ || oauth_data_cleared_)));
 }
 
 void EnterpriseEnrollmentHelperImpl::EnrollUsingAuthCode(
     const std::string& auth_code,
     bool fetch_additional_token) {
-  DCHECK(!started_);
-  started_ = true;
+  DCHECK(oauth_status_ == OAUTH_NOT_STARTED);
+  oauth_status_ = OAUTH_STARTED_WITH_AUTH_CODE;
   oauth_fetcher_.reset(policy::PolicyOAuth2TokenFetcher::CreateInstance());
   oauth_fetcher_->StartWithAuthCode(
       auth_code, g_browser_process->system_request_context(),
@@ -103,28 +103,39 @@ void EnterpriseEnrollmentHelperImpl::EnrollUsingAuthCode(
 
 void EnterpriseEnrollmentHelperImpl::EnrollUsingToken(
     const std::string& token) {
-  DCHECK(!started_);
-  started_ = true;
-  DoEnrollUsingToken(token);
+  DCHECK(oauth_status_ != OAUTH_STARTED_WITH_TOKEN);

[achuithb, 2016/08/23 18:16:44]

Can we have a positive check here instead?
DCHECK(oauth_status == OAUTH_NOT_STARTED || oauth_status ==
OAUTH_STARTED_WITH_AUTHCODE);

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

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

+  if (oauth_status_ == OAUTH_NOT_STARTED) {

[achuithb, 2016/08/23 18:16:44]

Drop {}

+    oauth_status_ = OAUTH_STARTED_WITH_TOKEN;
+  }
+  DoEnroll(token);
+}
+
+void EnterpriseEnrollmentHelperImpl::EnrollUsingAttestation() {
+  CHECK(enrollment_config_.mode == policy::EnrollmentConfig::MODE_ATTESTATION ||
+        enrollment_config_.mode ==
+            policy::EnrollmentConfig::MODE_ATTESTATION_FORCED);

[achuithb, 2016/08/23 18:16:44]

DCHECK for oauth_status_ here?

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

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

+  DoEnroll("");
 }
 
 void EnterpriseEnrollmentHelperImpl::ClearAuth(const base::Closure& callback) {
-  // Do not revoke the additional token if enrollment has finished
-  // successfully.
-  if (!success_ && additional_token_.length())
-    (new TokenRevoker())->Start(additional_token_);
-
-  if (oauth_fetcher_) {
-    if (!oauth_fetcher_->OAuth2AccessToken().empty())
-      (new TokenRevoker())->Start(oauth_fetcher_->OAuth2AccessToken());
-
-    if (!oauth_fetcher_->OAuth2RefreshToken().empty())
-      (new TokenRevoker())->Start(oauth_fetcher_->OAuth2RefreshToken());
-
-    oauth_fetcher_.reset();
-  } else if (oauth_token_.length()) {
-    // EnrollUsingToken was called.
-    (new TokenRevoker())->Start(oauth_token_);
+  if (oauth_status_ != OAUTH_NOT_STARTED) {
+    // Do not revoke the additional token if enrollment has finished
+    // successfully.
+    if (!success_ && additional_token_.length())
+      (new TokenRevoker())->Start(additional_token_);
+
+    if (oauth_fetcher_) {
+      if (!oauth_fetcher_->OAuth2AccessToken().empty())
+        (new TokenRevoker())->Start(oauth_fetcher_->OAuth2AccessToken());
+
+      if (!oauth_fetcher_->OAuth2RefreshToken().empty())
+        (new TokenRevoker())->Start(oauth_fetcher_->OAuth2RefreshToken());
+
+      oauth_fetcher_.reset();
+    } else if (oauth_token_.length()) {
+      // EnrollUsingToken was called.
+      (new TokenRevoker())->Start(oauth_token_);
+    }
   }
 
   chromeos::ProfileHelper::Get()->ClearSigninProfile(
@@ -132,8 +143,7 @@ void EnterpriseEnrollmentHelperImpl::ClearAuth(const base::Closure& callback) {
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
-void EnterpriseEnrollmentHelperImpl::DoEnrollUsingToken(
-    const std::string& token) {
+void EnterpriseEnrollmentHelperImpl::DoEnroll(const std::string& token) {
   DCHECK(token == oauth_token_ || oauth_token_.empty());

[achuithb, 2016/08/23 18:16:44]

Do we need a DCHECK for oauth_status_?

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

Added one.

   oauth_token_ = token;
   policy::BrowserPolicyConnectorChromeOS* connector =
@@ -143,7 +153,9 @@ void EnterpriseEnrollmentHelperImpl::DoEnrollUsingToken(
     LOG(ERROR) << "Trying to re-enroll to a different domain than "
                << connector->GetEnterpriseDomain();
     UMA(policy::kMetricEnrollmentPrecheckDomainMismatch);
-    finished_ = true;
+    if (oauth_status_ != OAUTH_NOT_STARTED) {

[achuithb, 2016/08/23 18:16:44]

drop {}

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

Done.

+      oauth_status_ = OAUTH_FINISHED;
+    }
     status_consumer()->OnOtherError(OTHER_ERROR_DOMAIN_MISMATCH);
     return;
   }
@@ -200,13 +212,13 @@ void EnterpriseEnrollmentHelperImpl::OnTokenFetched(
     const GoogleServiceAuthError& error) {
   if (error.state() != GoogleServiceAuthError::NONE) {
     ReportAuthStatus(error);
-    finished_ = true;
+    oauth_status_ = OAUTH_FINISHED;
     status_consumer()->OnAuthError(error);
     return;
   }
 
   if (!is_additional_token) {
-    DoEnrollUsingToken(token);
+    EnrollUsingToken(token);
     return;
   }
 
@@ -225,7 +237,9 @@ void EnterpriseEnrollmentHelperImpl::OnEnrollmentFinished(
   // TODO(pbond): remove this LOG once http://crbug.com/586961 is fixed.
   LOG(WARNING) << "Enrollment finished";
   ReportEnrollmentStatus(status);
-  finished_ = true;
+  if (oauth_status_ != OAUTH_NOT_STARTED) {

[achuithb, 2016/08/23 18:16:44]

Drop {}

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

Done.

+    oauth_status_ = OAUTH_FINISHED;
+  }
   if (status.status() == policy::EnrollmentStatus::STATUS_SUCCESS) {
     success_ = true;
     StartupUtils::MarkOobeCompleted();
@@ -418,7 +432,7 @@ void EnterpriseEnrollmentHelperImpl::UMA(policy::MetricEnrollment sample) {
 
 void EnterpriseEnrollmentHelperImpl::OnSigninProfileCleared(
     const base::Closure& callback) {
-  auth_data_cleared_ = true;
+  oauth_data_cleared_ = true;
   callback.Run();
 }