Fetch policy from Active Directory at the end of enrollment

chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc

Index: chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
diff --git a/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc b/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
index 3338b0d42dd4db4edfe558703e15990b91a3ecec..4fe06e1eac87f73d06264b1cc1a132f9f2571172 100644
--- a/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
+++ b/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
@@ -25,6 +25,8 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/attestation/attestation_flow.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/auth_policy_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/http/http_status_code.h"
@@ -250,6 +252,7 @@ void EnrollmentHandlerChromeOS::OnStoreError(CloudPolicyStore* store) {
     // enterprise-managed.
     return;
   }
+  LOG(ERROR) << "Error in device policy store.";

[Roman Sorokin (ftl), 2016/12/01 12:43:19]

Do we need that LOG? ReportResult should log the error.

[Thiemo Nagel, 2016/12/01 14:03:11]

You're right, but imho "Error in device policy store." is much clearer and
easier to understand than "Enrollment failed: 12, client: 0, validation: 0,
store: 6, lock: 0".

[Roman Sorokin (ftl), 2016/12/01 14:17:17]

ok, but "this is bloating the Chrome executable by" 29 "Bytes"

   ReportResult(EnrollmentStatus::ForStoreError(store_->status(),
                                                store_->validation_status()));
 }
@@ -488,14 +491,38 @@ void EnrollmentHandlerChromeOS::HandleStoreRobotAuthTokenResult(bool result) {
     return;
   }
 
+  SetStep(STEP_STORE_POLICY);
   if (device_mode_ == policy::DEVICE_MODE_ENTERPRISE_AD) {
-    ReportResult(EnrollmentStatus::ForStatus(EnrollmentStatus::STATUS_SUCCESS));
+    CHECK_EQ(true, install_attributes_->IsActiveDirectoryManaged());

[Roman Sorokin (ftl), 2016/12/01 12:43:19]

s/CHECK_EQ(true, /CHECK(

[Thiemo Nagel, 2016/12/01 14:03:11]

What's wrong with using CHECK_EQ()?

[Roman Sorokin (ftl), 2016/12/01 14:17:18]

looks like if (<smthg> == true) which should be just if(<smthg>)

[Thiemo Nagel, 2016/12/01 17:05:59]

I've changed it.  It seems that CHECK_EQ(true, ...) is rarely used across the
codebase.

+    // Update device settings so that in case of Active Directory unsigned
+    // policy is accepted.
+    chromeos::DeviceSettingsService::Get()->SetDeviceMode(
+        install_attributes_->GetMode());
+    chromeos::DBusThreadManager::Get()
+        ->GetAuthPolicyClient()
+        ->RefreshDevicePolicy(base::Bind(
+            &EnrollmentHandlerChromeOS::HandleActiveDirectoryPolicyRefreshed,
+            weak_ptr_factory_.GetWeakPtr()));
   } else {
-    SetStep(STEP_STORE_POLICY);
     store_->InstallInitialPolicy(*policy_);
   }
 }
 
+void EnrollmentHandlerChromeOS::HandleActiveDirectoryPolicyRefreshed(
+    bool success) {
+  DCHECK_EQ(STEP_STORE_POLICY, enrollment_step_);
+
+  if (!success) {
+    LOG(ERROR) << "Failed to load Active Directory policy.";
+    ReportResult(EnrollmentStatus::ForStatus(
+        EnrollmentStatus::STATUS_ACTIVE_DIRECTORY_POLICY_FETCH_FAILED));
+    return;
+  }
+
+  // After that, the enrollment flow continues in one of the OnStore* observers.
+  store_->Load();
+}
+
 void EnrollmentHandlerChromeOS::Stop() {
   if (client_.get())
     client_->RemoveObserver(this);