Implemented OwnerSettingsService::Set() method.

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.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/ownership/owner_settings_service_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/device_settings_provider.h"
 #include "chrome/browser/chromeos/settings/session_manager_operation.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/tpm_token_loader.h"
 #include "components/ownership/owner_key_util.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/common/content_switches.h"
 #include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
 #include "crypto/rsa_private_key.h"
 #include "crypto/scoped_nss_types.h"
 #include "crypto/signature_creator.h"
 
 namespace em = enterprise_management;
 
 using content::BrowserThread;
 using ownership::OwnerKeyUtil;
 using ownership::PrivateKey;
 using ownership::PublicKey;
 
 namespace chromeos {
 
 namespace {
 
-DeviceSettingsService* g_device_settings_service_for_testing = NULL;
-
 bool IsOwnerInTests(const std::string& user_id) {
   if (user_id.empty() ||
       !CommandLine::ForCurrentProcess()->HasSwitch(::switches::kTestType) ||
       !CrosSettings::IsInitialized()) {
     return false;
   }
   const base::Value* value = CrosSettings::Get()->GetPref(kDeviceOwner);
   if (!value || value->GetType() != base::Value::TYPE_STRING)
     return false;
   return static_cast<const base::StringValue*>(value)->GetString() == user_id;
@@ skipping 71 matching lines @@
   scoped_refptr<base::TaskRunner> task_runner =
       BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   base::PostTaskAndReplyWithResult(
       task_runner.get(),
       FROM_HERE,
       base::Bind(&DoesPrivateKeyExistAsyncHelper, owner_key_util),
       callback);
 }
 
-DeviceSettingsService* GetDeviceSettingsService() {
-  if (g_device_settings_service_for_testing)
-    return g_device_settings_service_for_testing;
-  return DeviceSettingsService::IsInitialized() ? DeviceSettingsService::Get()
-                                                : NULL;
-}
-
 }  // namespace
 
+class OwnerSettingsServiceChromeOS::ProcessingLoopAutoBlocker {
+ public:
+  explicit ProcessingLoopAutoBlocker(
+      DeviceSettingsService* device_settings_service)
+      : device_settings_service_(device_settings_service) {
+    device_settings_service_->EnableProcessingLoop(false /* enabled */);
+  }
+
+  ~ProcessingLoopAutoBlocker() {
+    device_settings_service_->EnableProcessingLoop(true /* enabled */);
+  }
+
+ private:
+  DeviceSettingsService* device_settings_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProcessingLoopAutoBlocker);
+};
+
+OwnerSettingsServiceChromeOS::SetRequest::SetRequest(
+    const std::string& setting,
+    linked_ptr<base::Value> value)
+    : setting(setting), value(value) {
+}
+
+OwnerSettingsServiceChromeOS::SetRequest::~SetRequest() {
+}
+
 OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
+    DeviceSettingsService* device_settings_service,
     Profile* profile,
     const scoped_refptr<OwnerKeyUtil>& owner_key_util)
     : ownership::OwnerSettingsService(owner_key_util),
+      device_settings_service_(device_settings_service),
       profile_(profile),
       waiting_for_profile_creation_(true),
       waiting_for_tpm_token_(true),
-      weak_factory_(this) {
+      weak_factory_(this),
+      set_requests_callback_factory_(this) {
   if (TPMTokenLoader::IsInitialized()) {
     TPMTokenLoader::TPMTokenStatus tpm_token_status =
         TPMTokenLoader::Get()->IsTPMTokenEnabled(
             base::Bind(&OwnerSettingsServiceChromeOS::OnTPMTokenReady,
                        weak_factory_.GetWeakPtr()));
     waiting_for_tpm_token_ =
         tpm_token_status == TPMTokenLoader::TPM_TOKEN_STATUS_UNDETERMINED;
   }
 
   if (DBusThreadManager::IsInitialized() &&
       DBusThreadManager::Get()->GetSessionManagerClient()) {
     DBusThreadManager::Get()->GetSessionManagerClient()->AddObserver(this);
   }
 
+  device_settings_service_->AddObserver(this);
+
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_CREATED,
                  content::Source<Profile>(profile_));
 }
 
 OwnerSettingsServiceChromeOS::~OwnerSettingsServiceChromeOS() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (DBusThreadManager::IsInitialized() &&
       DBusThreadManager::Get()->GetSessionManagerClient()) {
     DBusThreadManager::Get()->GetSessionManagerClient()->RemoveObserver(this);
   }
+
+  device_settings_service_->RemoveObserver(this);
 }
 
 void OwnerSettingsServiceChromeOS::OnTPMTokenReady(
     bool /* tpm_token_enabled */) {
   DCHECK(thread_checker_.CalledOnValidThread());
   waiting_for_tpm_token_ = false;
 
   // TPMTokenLoader initializes the TPM and NSS database which is necessary to
   // determine ownership. Force a reload once we know these are initialized.
   ReloadKeypair();
 }
 
-void OwnerSettingsServiceChromeOS::SignAndStorePolicyAsync(
-    scoped_ptr<em::PolicyData> policy,
-    const base::Closure& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  SignAndStoreSettingsOperation* operation = new SignAndStoreSettingsOperation(
-      base::Bind(&OwnerSettingsServiceChromeOS::HandleCompletedOperation,
-                 weak_factory_.GetWeakPtr(),
-                 callback),
-      policy.Pass());
-  operation->set_owner_settings_service(weak_factory_.GetWeakPtr());
-  pending_operations_.push_back(operation);
-  if (pending_operations_.front() == operation)
-    StartNextOperation();
+bool OwnerSettingsServiceChromeOS::HandlesSetting(const std::string& setting) {
+  return DeviceSettingsProvider::IsDeviceSetting(setting);
+}
+
+void OwnerSettingsServiceChromeOS::Set(const std::string& setting,
+                                       const base::Value& value) {
+  if (!IsOwner() && !IsOwnerInTests(user_id_))
+    return;
+  set_requests_.push(

[Mattias Nissler (ping if slow), 2014/10/20 12:41:30]

It just occurred to me that instead of keeping a queue of individual settings to
updated, we could just as well keep a ChromeDeviceSettingsProto that we write
all tentative device settings to. Then, when we get the chance to call
DeviceSettingsService::Store() we can send the updated device settings in one
go. WDYT?

This would also conveniently give us the right settings to send to the proposed
DeviceSettingsProvider call to update tentative settings.

[ygorshenin1, 2014/10/20 13:35:16]

But we always can call DeviceSettingsService::Store(), right? Or, you mean, that
if DeviceSettingsService is currently busy with load/store, we should wait until
it'll complete it's operations, then merge our device settings with
DeviceSettingsService::device_settings_, sign them and send for storage?

[Mattias Nissler (ping if slow), 2014/10/20 13:46:49]

DeviceSettingsService doesn't impose any restrictions on when we can call Store,
it'll happily queue up multiple Store operations for us. However, that'd make it
hard for OwnerSettingsServiceChromeOS to keep track of what to do in
DeviceSettingsUpdated(), as we'd have to wait for all Store() operations to
finish until we'd get a consistent picture again. And if we'd generate more
Store() operations using the current device settings published by
DeviceSettingsService while we still have Store()s in progress, we might
inadvertently roll back the effect of the pending Store()s because our fresh
Store() would pick up old values.

Hence, I think OwnerSettingsServiceChromeOS should have at most one pending
Store() operation at a given point in time. We can accumulate all changes that
come in via Set() in a local ChromeDeviceSettingsProto instance and fire off a
new Store() operation once the currently-pending Store() has finished. Makes
sense?

[ygorshenin1, 2014/10/20 13:49:06]

Sure, thanks for the explanation!

+      SetRequest(setting, linked_ptr<base::Value>(value.DeepCopy())));
+  ProcessNextSetRequest();
 }
 
 void OwnerSettingsServiceChromeOS::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
     NOTREACHED();
     return;
   }
 
   Profile* profile = content::Source<Profile>(source).ptr();
   if (profile != profile_) {
     NOTREACHED();
     return;
   }
 
   waiting_for_profile_creation_ = false;
   ReloadKeypair();
 }
 
 void OwnerSettingsServiceChromeOS::OwnerKeySet(bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (success)
     ReloadKeypair();
 }
 
+void OwnerSettingsServiceChromeOS::OwnershipStatusChanged() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ProcessNextSetRequestAsync();
+}
+
+void OwnerSettingsServiceChromeOS::DeviceSettingsUpdated() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ProcessNextSetRequestAsync();
+}
+
 // static
 void OwnerSettingsServiceChromeOS::IsOwnerForSafeModeAsync(
     const std::string& user_hash,
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     const IsOwnerCallback& callback) {
   CHECK(chromeos::LoginState::Get()->IsInSafeMode());
 
   // Make sure NSS is initialized and NSS DB is loaded for the user before
   // searching for the owner key.
   BrowserThread::PostTaskAndReply(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(base::IgnoreResult(&crypto::InitializeNSSForChromeOSUser),
                  user_hash,
                  ProfileHelper::GetProfilePathByUserIdHash(user_hash)),
       base::Bind(&DoesPrivateKeyExistAsync, owner_key_util, callback));
 }
 
-// static
-void OwnerSettingsServiceChromeOS::SetDeviceSettingsServiceForTesting(
-    DeviceSettingsService* device_settings_service) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  g_device_settings_service_for_testing = device_settings_service;
+void OwnerSettingsServiceChromeOS::AddObserver(Observer* observer) {
+  if (observer)
+    observers_.AddObserver(observer);
+}
+
+void OwnerSettingsServiceChromeOS::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
 }
 
 void OwnerSettingsServiceChromeOS::OnPostKeypairLoadedActions() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   user_id_ = profile_->GetProfileName();
   const bool is_owner = IsOwner() || IsOwnerInTests(user_id_);
-  if (is_owner && GetDeviceSettingsService())
-    GetDeviceSettingsService()->InitOwner(user_id_, weak_factory_.GetWeakPtr());
+  if (is_owner)
+    device_settings_service_->InitOwner(user_id_, weak_factory_.GetWeakPtr());
 }
 
 void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
     void(const scoped_refptr<PublicKey>& public_key,
          const scoped_refptr<PrivateKey>& private_key)>& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (waiting_for_profile_creation_ || waiting_for_tpm_token_)
     return;
   scoped_refptr<base::TaskRunner> task_runner =
       BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   task_runner->PostTask(
       FROM_HERE,
       base::Bind(&LoadPrivateKey,
                  owner_key_util_,
                  ProfileHelper::GetUserIdHashFromProfile(profile_),
                  callback));
 }
 
-void OwnerSettingsServiceChromeOS::StartNextOperation() {
-  DeviceSettingsService* service = GetDeviceSettingsService();
-  if (!pending_operations_.empty() && service &&
-      service->session_manager_client()) {
-    pending_operations_.front()->Start(
-        service->session_manager_client(), owner_key_util_, public_key_);
-  }
+void OwnerSettingsServiceChromeOS::ProcessNextSetRequestAsync() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&OwnerSettingsServiceChromeOS::ProcessNextSetRequest,
+                 weak_factory_.GetWeakPtr()));
 }
 
-void OwnerSettingsServiceChromeOS::HandleCompletedOperation(
-    const base::Closure& callback,
-    SessionManagerOperation* operation,
-    DeviceSettingsService::Status status) {
-  DCHECK_EQ(operation, pending_operations_.front());
-
-  DeviceSettingsService* service = GetDeviceSettingsService();
-  if (status == DeviceSettingsService::STORE_SUCCESS) {
-    service->set_policy_data(operation->policy_data().Pass());
-    service->set_device_settings(operation->device_settings().Pass());
+void OwnerSettingsServiceChromeOS::ProcessNextSetRequest() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (set_requests_.empty() || set_requests_callback_factory_.HasWeakPtrs() ||
+      device_settings_service_->HasPendingOperations()) {
+    return;
   }
 
-  if ((operation->public_key().get() && !public_key_.get()) ||
-      (operation->public_key().get() && public_key_.get() &&
-       operation->public_key()->data() != public_key_->data())) {
-    // Public part changed so we need to reload private part too.
-    ReloadKeypair();
-    content::NotificationService::current()->Notify(
-        chrome::NOTIFICATION_OWNERSHIP_STATUS_CHANGED,
-        content::Source<OwnerSettingsServiceChromeOS>(this),
-        content::NotificationService::NoDetails());
+  scoped_ptr<ProcessingLoopAutoBlocker> lock(
+      new ProcessingLoopAutoBlocker(device_settings_service_));
+
+  // TODO (ygorshenin@): notify somehow DeviceSettingsProvider about tentative
+  // change.
+  // crbug.com/230018

[Mattias Nissler (ping if slow), 2014/10/20 12:41:30]

Shouldn't this already happen in Set()?

[ygorshenin1, 2014/10/22 09:20:11]

Done.

+  SetRequest request = set_requests_.front();
+  set_requests_.pop();
+
+  em::ChromeDeviceSettingsProto new_settings(
+      *device_settings_service_->device_settings());
+  DeviceSettingsService::UpdateDeviceSettings(request.setting, *request.value,
+                                              new_settings);
+  scoped_ptr<em::PolicyData> new_policy = DeviceSettingsService::AssemblePolicy(
+      user_id_, device_settings_service_->policy_data(), &new_settings);
+  bool rv = AssembleAndSignPolicyAsync(
+      content::BrowserThread::GetBlockingPool(),
+      new_policy.Pass(),
+      base::Bind(&OwnerSettingsServiceChromeOS::OnPolicyAssembledAndSigned,
+                 set_requests_callback_factory_.GetWeakPtr(),
+                 Passed(&lock)));
+  if (!rv)
+    HandleError();
+}
+
+void OwnerSettingsServiceChromeOS::OnPolicyAssembledAndSigned(
+    scoped_ptr<ProcessingLoopAutoBlocker> blocker,
+    scoped_ptr<em::PolicyFetchResponse> policy_response) {
+  blocker.reset();
+  if (!policy_response.get()) {
+    HandleError();
+    return;
   }
-  service->OnSignAndStoreOperationCompleted(status);
-  if (!callback.is_null())
-    callback.Run();
+  device_settings_service_->Store(
+      policy_response.Pass(),
+      base::Bind(&OwnerSettingsServiceChromeOS::OnSignedPolicyStored,
+                 weak_factory_.GetWeakPtr()));
+}
 
-  pending_operations_.pop_front();
-  delete operation;
-  StartNextOperation();
+void OwnerSettingsServiceChromeOS::OnSignedPolicyStored() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSetCompleted(true));
+  ProcessNextSetRequestAsync();
+}
+
+void OwnerSettingsServiceChromeOS::HandleError() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSetCompleted(false));
+  ProcessNextSetRequestAsync();
 }
 
 }  // namespace chromeos