SetManagementSettings() is moved to OwnerSettingsServiceChromeOS.

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 ad963ff0a8149ae14e18e0f5b6a2f8d913d46fcf..5bcb48be3a9e151ef39dcba7d4d2c3c7001d3326 100644
--- a/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
+++ b/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
@@ -10,18 +10,27 @@
 #include "base/message_loop/message_loop.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h"
+#include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
+#include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
 #include "chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h"
 #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/device_oauth2_token_service.h"
 #include "chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chromeos/chromeos_switches.h"
+#include "components/user_manager/user_manager.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/http/http_status_code.h"
 
 namespace em = enterprise_management;
 
+using chromeos::OwnerSettingsServiceChromeOS;
+using chromeos::OwnerSettingsServiceChromeOSFactory;
+using user_manager::UserManager;
+
 namespace policy {
 
 namespace {
@@ -206,10 +215,11 @@ void EnrollmentHandlerChromeOS::OnStoreLoaded(CloudPolicyStore* store) {
 void EnrollmentHandlerChromeOS::OnStoreError(CloudPolicyStore* store) {
   DCHECK_EQ(store_, store);
   if (enrollment_step_ == STEP_STORE_TOKEN_AND_ID) {
-    // Calling DeviceSettingsService::SetManagementSettings() on a non-
-    // enterprise-managed device will trigger OnStoreError(), as
-    // DeviceCloudPolicyStore listens to all changes on DeviceSettingsService,
-    // and it calls OnStoreError() when the device is not enterprise-managed.
+    // Calling OwnerSettingsServiceChromeOS::SetManagementSettings()
+    // on a non- enterprise-managed device will fail as
+    // DeviceCloudPolicyStore listens to all changes on device
+    // settings, and it calls OnStoreError() when the device is not
+    // enterprise-managed.
     return;
   }
   ReportResult(EnrollmentStatus::ForStoreError(store_->status(),
@@ -346,10 +356,23 @@ void EnrollmentHandlerChromeOS::StartLockDevice() {
     // Consumer device enrollment doesn't use install attributes. Instead,
     // we put the information in the owners settings.
     enrollment_step_ = STEP_STORE_TOKEN_AND_ID;
-    device_settings_service_->SetManagementSettings(
-        em::PolicyData::CONSUMER_MANAGED, request_token_, device_id_,
-        base::Bind(&EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone,
-                   weak_ptr_factory_.GetWeakPtr()));
+    const user_manager::User* user = UserManager::Get()->FindUser(username_);

[Mattias Nissler (ping if slow), 2014/12/02 08:52:40]

Question for David: Are there any flows that will require consumer enrollment to
happen during OOBE while we haven't created the owner's user account yet?

[davidyu, 2014/12/02 08:59:04]

No. Consumer manager enrollment during OOBE is currently not in our plan.

+    Profile* profile =
+        user ? chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;

[davidyu, 2014/12/01 16:45:33]

Is there any case that |user| might be null at this point? If not, we probably
should CHECK(user). Same for CHECK(profile) and CHECK(service).

[Mattias Nissler (ping if slow), 2014/12/02 08:52:40]

It might be better to just pass the OwnerSettingsServiceChromeOS in the ctor.
Or, given that we have quite some if-management-mode-something business already
in this file, split into separate classes for consumer and enterprise enrollment
to reduce confusion. Not in this CL, but let's discuss where we want to take
this and document the decision per TODO comments. David, WDYT?

[davidyu, 2014/12/02 08:59:04]

The current logic is wrong for consumer enrollment. I think it needs to be
fixed. SetManagementSettings is not an optional step for consumer management. We
should do some error handling here instead of ignoring it silently.

I think passing OwnerSettingsServiceChromeOS in the ctor is fine. 

[ygorshenin1, 2014/12/02 19:18:58]

OwnerSettingsServiceChromeOS here corresponds to user with user id equals to
username_, which is set in
EnrollmentHandlerChromeOS::HandlePolicyValidationResult(). How to get correct
instance of OwnerSettingsServiceChromeOS before username is known, at the time
of EnrollmentHandlerChromeOS instantiation?

[davidyu, 2014/12/03 08:44:02]

I took another look. Since the service is only used for consumer management, we
can pass the service from
ConsumerEnrollmentHandler::OnOwnerAccessTokenAvailable() to
DeviceCloudPolicyInitializer::StartEnrollment() to the constructor. In
ConsumerEnrollmentHandler, you can just use |profile_| to get the service. For
enterprise flow, you can simply pass nullptr, as it is not used.

I am not sure if we should have a separate enrollment handler class for
consumer, as this is the only one step that needs to be handled differently from
the enterprise flow. Other steps still can be shared.

[Mattias Nissler (ping if slow), 2014/12/03 09:07:54]

This solution seems fine for now.
There are three tests for |management_mode_ == MANAGEMENT_MODE_CONSUMER_MANAGED|
in the code already (and a fourth is missing per my other comment). I'm fine
with not forking just yet, but if we keep adding more special-casing it doesn't
make sense to pretend enterprise and consumer are the same thing IMHO.

[davidyu, 2014/12/03 09:13:53]

Ah. you are right. I miscounted. I think it makes sense to have a separate class
then.

+    OwnerSettingsServiceChromeOS* service =
+        profile
+            ? OwnerSettingsServiceChromeOSFactory::GetForBrowserContext(profile)
+            : nullptr;
+    if (service) {
+      OwnerSettingsServiceChromeOS::ManagementSettingsSetRequest request;
+      request.management_mode = em::PolicyData::CONSUMER_MANAGED;
+      request.request_token = request_token_;
+      request.device_id = device_id_;
+      request.callback = base::Bind(
+          &EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone,
+          weak_ptr_factory_.GetWeakPtr());
+      service->SetManagementSettings(request);
+    }
   } else {
     install_attributes_->LockDevice(
         username_, device_mode_, device_id_,
@@ -358,10 +381,9 @@ void EnrollmentHandlerChromeOS::StartLockDevice() {
   }
 }
 
-void EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone() {
+void EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone(bool success) {
   CHECK_EQ(STEP_STORE_TOKEN_AND_ID, enrollment_step_);
-  if (device_settings_service_->status() !=
-      chromeos::DeviceSettingsService::STORE_SUCCESS) {
+  if (!success) {
     ReportResult(EnrollmentStatus::ForStatus(
         EnrollmentStatus::STATUS_STORE_TOKEN_AND_ID_FAILED));
     return;