Don't start FRE from the login screen

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 61 matching lines @@
   bool fre_flag_found = provider->GetMachineStatistic(
       system::kCheckEnrollmentKey, &check_enrollment_value);
 
   if (fre_flag_found) {
     if (check_enrollment_value == "0")
       return AutoEnrollmentController::EXPLICITLY_NOT_REQUIRED;
     if (check_enrollment_value == "1")
       return AutoEnrollmentController::EXPLICITLY_REQUIRED;
   }
   if (!provider->GetMachineStatistic(system::kActivateDateKey, nullptr) &&
-      !provider->GetEnterpriseMachineID().empty())
+      !provider->GetEnterpriseMachineID().empty()) {
     return AutoEnrollmentController::NOT_REQUIRED;
+  }
   return AutoEnrollmentController::REQUIRED;
 }
 
 std::string FRERequirementToString(
     AutoEnrollmentController::FRERequirement requirement) {
   switch (requirement) {
     case AutoEnrollmentController::REQUIRED:
       return "Auto-enrollment required.";
     case AutoEnrollmentController::NOT_REQUIRED:
       return "Auto-enrollment disabled: first setup.";
@@ skipping 39 matching lines @@
 
   LOG(FATAL) << "Unknown auto-enrollment mode " << command_line_mode;
   return MODE_NONE;
 }
 
 AutoEnrollmentController::AutoEnrollmentController() {}
 
 AutoEnrollmentController::~AutoEnrollmentController() {}
 
 void AutoEnrollmentController::Start() {
   // This method is called at the point in the OOBE/login flow at which the

[Thiemo Nagel, 2017/04/20 13:24:00]

Deleting this comment since it's incomplete (doesn't mention the Retry() case). 
In general, commenting on things that are happening in a different part of the
code will be outdated or wrong sooner or later and it's probably better to not
do that.

   // auto-enrollment check can start. This happens either after the EULA is
   // accepted, or right after a reboot if the EULA has already been accepted.
 
   // Skip if GAIA is disabled or modulus configuration is not present.
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(chromeos::switches::kDisableGaiaServices) ||
       (!command_line->HasSwitch(
            chromeos::switches::kEnterpriseEnrollmentInitialModulus) &&
        !command_line->HasSwitch(
            chromeos::switches::kEnterpriseEnrollmentModulusLimit))) {
     VLOG(1) << "Auto-enrollment disabled: command line.";
     UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
     return;
   }
 
   // Skip if mode comes up as none.
   if (GetMode() == MODE_NONE) {

[achuithb, 2017/04/19 19:04:43]

Maybe add the state check here?

[Thiemo Nagel, 2017/04/20 13:24:00]

I've put it right at the start of the method.

     VLOG(1) << "Auto-enrollment disabled: no mode.";
     UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
     return;
   }
 
   fre_requirement_ = GetFRERequirement();
   VLOG(1) << FRERequirementToString(fre_requirement_);
   if (fre_requirement_ == EXPLICITLY_NOT_REQUIRED ||
       fre_requirement_ == NOT_REQUIRED) {
     UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
@@ skipping 12 matching lines @@
                          base::Bind(&AutoEnrollmentController::Timeout,
                                     weak_ptr_factory_.GetWeakPtr()));
 
   // Start by checking if the device has already been owned.
   UpdateState(policy::AUTO_ENROLLMENT_STATE_PENDING);
   DeviceSettingsService::Get()->GetOwnershipStatusAsync(
       base::Bind(&AutoEnrollmentController::OnOwnershipStatusCheckDone,
                  client_start_weak_factory_.GetWeakPtr()));
 }
 
-void AutoEnrollmentController::Cancel() {
-  if (client_) {
-    // Cancelling the |client_| allows it to determine whether
-    // its protocol finished before login was complete.
-    client_.release()->CancelAndDeleteSoon();
-  }
-
-  // Make sure to nuke pending |client_| start sequences.
-  client_start_weak_factory_.InvalidateWeakPtrs();
-
-  safeguard_timer_.Stop();
-}
-
 void AutoEnrollmentController::Retry() {
   if (client_)
     client_->Retry();
   else
     Start();
 }
 
 std::unique_ptr<AutoEnrollmentController::ProgressCallbackList::Subscription>
 AutoEnrollmentController::RegisterProgressCallback(
     const ProgressCallbackList::CallbackType& callback) {
   return progress_callbacks_.Add(callback);
 }
 
 void AutoEnrollmentController::OnOwnershipStatusCheckDone(
     DeviceSettingsService::OwnershipStatus status) {
-  policy::ServerBackedStateKeysBroker* state_keys_broker =
+  switch (status) {
+    case DeviceSettingsService::OWNERSHIP_NONE:
       g_browser_process->platform_part()
           ->browser_policy_connector_chromeos()
-          ->GetStateKeysBroker();
-  switch (status) {
-    case DeviceSettingsService::OWNERSHIP_NONE:
-      // TODO(tnagel): Prevent missing state keys broker in the first place.
-      // https://crbug.com/703658
-      if (!state_keys_broker) {
-        LOG(ERROR) << "State keys broker missing.";
-        UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
-        return;
-      }
-      state_keys_broker->RequestStateKeys(
-          base::Bind(&AutoEnrollmentController::StartClient,
-                     client_start_weak_factory_.GetWeakPtr()));
+          ->GetStateKeysBroker()
+          ->RequestStateKeys(
+              base::Bind(&AutoEnrollmentController::StartClient,
+                         client_start_weak_factory_.GetWeakPtr()));
       return;
     case DeviceSettingsService::OWNERSHIP_TAKEN:
       VLOG(1) << "Device already owned, skipping auto-enrollment check.";
       UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
       return;
     case DeviceSettingsService::OWNERSHIP_UNKNOWN:
       LOG(ERROR) << "Ownership unknown, skipping auto-enrollment check.";
       UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
       return;
   }
@@ skipping 95 matching lines @@
     UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
   } else {
     // This can actually happen in some cases, for example when state key
     // generation is waiting for time sync or the server just doesn't reply and
     // keeps the connection open.
     LOG(ERROR) << "AutoEnrollmentClient didn't complete within time limit.";
     UpdateState(policy::AUTO_ENROLLMENT_STATE_CONNECTION_ERROR);
   }
 
   // Reset state.
-  Cancel();
+  if (client_) {
+    // Cancelling the |client_| allows it to determine whether
+    // its protocol finished before login was complete.
+    client_.release()->CancelAndDeleteSoon();
+  }
+
+  // Make sure to nuke pending |client_| start sequences.
+  client_start_weak_factory_.InvalidateWeakPtrs();
 }
 
 }  // namespace chromeos