Be more picky in triggering enrollment recovery.

Restrict triggering of enrollment recovery to a smaller subset of
DeviceSettingsService status codes just to be on the safe side.

BUG=389481

Review URL: https://codereview.chromium.org/400623002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284083 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc

@@ -26,7 +26,7 @@ DeviceCloudPolicyStoreChromeOS::DeviceCloudPolicyStoreChromeOS(
     : device_settings_service_(device_settings_service),
       install_attributes_(install_attributes),
       background_task_runner_(background_task_runner),
-      first_update_(true),
+      enrollment_validation_done_(false),
       weak_factory_(this) {
   device_settings_service_->AddObserver(this);
 }
@@ -138,32 +138,47 @@ void DeviceCloudPolicyStoreChromeOS::UpdateFromService() {
     return;
   }
 
-  // Fill UMA histogram once per session.  Skip temp validation error because it
-  // is not a definitive result (policy load will be retried).
+  // Once per session, validate internal consistency of enrollment state (DM
+  // token must be present on enrolled devices) and in case of failure set flag
+  // to indicate that recovery is required.
   const chromeos::DeviceSettingsService::Status status =
       device_settings_service_->status();
-  if (first_update_ &&
-      status != chromeos::DeviceSettingsService::STORE_TEMP_VALIDATION_ERROR) {
-    first_update_ = false;
-    const bool has_dm_token =
-        status == chromeos::DeviceSettingsService::STORE_SUCCESS &&
-        device_settings_service_->policy_data() &&
-        device_settings_service_->policy_data()->has_request_token();
-
-    // At the time LoginDisplayHostImpl decides whether enrollment flow is to be
-    // started, policy hasn't been read yet, so LoginDisplayHostImpl is not in a
-    // position to decide whether recovery is required.  To work around this,
-    // upon policy load on machines requiring recovery, a flag is stored in
-    // prefs which is accessed by LoginDisplayHostImpl early during (next) boot.
-    if (!has_dm_token) {
-      LOG(ERROR) << "Policy read on enrolled device yields no DM token! "
-                 << "Status: " << status << ".";
-      chromeos::StartupUtils::MarkEnrollmentRecoveryRequired();
+  switch (status) {
+    case chromeos::DeviceSettingsService::STORE_SUCCESS:
+    case chromeos::DeviceSettingsService::STORE_KEY_UNAVAILABLE:
+    case chromeos::DeviceSettingsService::STORE_NO_POLICY:
+    case chromeos::DeviceSettingsService::STORE_INVALID_POLICY:
+    case chromeos::DeviceSettingsService::STORE_VALIDATION_ERROR: {
+      if (!enrollment_validation_done_) {
+        enrollment_validation_done_ = true;
+        const bool has_dm_token =
+            status == chromeos::DeviceSettingsService::STORE_SUCCESS &&
+            device_settings_service_->policy_data() &&
+            device_settings_service_->policy_data()->has_request_token();
+
+        // At the time LoginDisplayHostImpl decides whether enrollment flow is
+        // to be started, policy hasn't been read yet.  To work around this,
+        // once the need for recovery is detected upon policy load, a flag is
+        // stored in prefs which is accessed by LoginDisplayHostImpl early
+        // during (next) boot.
+        if (!has_dm_token) {
+          LOG(ERROR) << "Device policy read on enrolled device yields "
+                     << "no DM token! Status: " << status << ".";
+          chromeos::StartupUtils::MarkEnrollmentRecoveryRequired();
+        }
+        UMA_HISTOGRAM_BOOLEAN("Enterprise.EnrolledPolicyHasDMToken",
+                              has_dm_token);
+      }
+      break;
     }
-    UMA_HISTOGRAM_BOOLEAN("Enterprise.EnrolledPolicyHasDMToken", has_dm_token);
+    case chromeos::DeviceSettingsService::STORE_POLICY_ERROR:
+    case chromeos::DeviceSettingsService::STORE_OPERATION_FAILED:
+    case chromeos::DeviceSettingsService::STORE_TEMP_VALIDATION_ERROR:
+      // Do nothing for write errors or transient read errors.
+      break;
   }
 
-  switch (device_settings_service_->status()) {
+  switch (status) {
     case chromeos::DeviceSettingsService::STORE_SUCCESS: {
       status_ = STATUS_OK;
       policy_.reset(new em::PolicyData());

chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h

@@ -76,9 +76,8 @@ class DeviceCloudPolicyStoreChromeOS
 
   scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
 
-  // Run enrollment sanity check and UMA stats only upon the first policy
-  // read/update.
-  bool first_update_;
+  // Whether enterprise enrollment validation has yet been done.
+  bool enrollment_validation_done_;
 
   base::WeakPtrFactory<DeviceCloudPolicyStoreChromeOS> weak_factory_;