Fix up Owner settings on first load

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 <algorithm>
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.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/tpm_token_loader.h"
 #include "components/ownership/owner_key_util.h"
 #include "components/user_manager/user.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
@@ skipping 59 matching lines @@
 }
 
 void LoadPrivateKey(
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     const std::string username_hash,
     const base::Callback<void(const scoped_refptr<PublicKey>& public_key,
                               const scoped_refptr<PrivateKey>& private_key)>&
         callback) {
   std::vector<uint8> public_key_data;
   scoped_refptr<PublicKey> public_key;
+
   if (!owner_key_util->ImportPublicKey(&public_key_data)) {
     scoped_refptr<PrivateKey> private_key;
     BrowserThread::PostTask(BrowserThread::UI,
                             FROM_HERE,
                             base::Bind(callback, public_key, private_key));
     return;
   }
   public_key = new PublicKey();
   public_key->data().swap(public_key_data);
+
   bool rv = BrowserThread::PostTask(BrowserThread::IO,
                                     FROM_HERE,
                                     base::Bind(&LoadPrivateKeyByPublicKey,
                                                owner_key_util,
                                                public_key,
                                                username_hash,
                                                callback));
   if (!rv) {
     // IO thread doesn't exists in unit tests, but it's safe to use NSS from
     // BlockingPool in unit tests.
@@ skipping 50 matching lines @@
       return false;
 
     case policy::MANAGEMENT_MODE_CONSUMER_MANAGED:
       // For consumer management unenrollment.
       return new_mode == policy::MANAGEMENT_MODE_LOCAL_OWNER;
   }
 
   NOTREACHED();
   return false;
 }
-
 }  // namespace
 
 OwnerSettingsServiceChromeOS::ManagementSettings::ManagementSettings() {
 }
 
 OwnerSettingsServiceChromeOS::ManagementSettings::~ManagementSettings() {
 }
 
 OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
     DeviceSettingsService* device_settings_service,
+    CrosSettings* cros_settings,
     Profile* profile,
     const scoped_refptr<OwnerKeyUtil>& owner_key_util)
     : ownership::OwnerSettingsService(owner_key_util),
       device_settings_service_(device_settings_service),
+      cros_settings_(cros_settings),
       profile_(profile),
       waiting_for_profile_creation_(true),
       waiting_for_tpm_token_(true),
       has_pending_management_settings_(false),
       weak_factory_(this),
       store_settings_factory_(this) {
   if (TPMTokenLoader::IsInitialized()) {
     TPMTokenLoader::TPMTokenStatus tpm_token_status =
         TPMTokenLoader::Get()->IsTPMTokenEnabled(
             base::Bind(&OwnerSettingsServiceChromeOS::OnTPMTokenReady,
@@ skipping 60 matching lines @@
   pending_changes_.add(setting, make_scoped_ptr(value.DeepCopy()));
 
   em::ChromeDeviceSettingsProto settings;
   if (tentative_settings_.get()) {
     settings = *tentative_settings_;
   } else if (device_settings_service_->status() ==
                  DeviceSettingsService::STORE_SUCCESS &&
              device_settings_service_->device_settings()) {
     settings = *device_settings_service_->device_settings();
   }
   UpdateDeviceSettings(setting, value, settings);

[Mattias Nissler (ping if slow), 2015/03/16 08:51:42]

nit: blank line here

+  // Perform fixups required to ensure sensical local-owner device policy:
+  //  1) The owner must be in the username field,
+  //  2) user whitelisting must be explicitly allowed or disallowed, and
+  //  3) the owner user must be on the whitelist, if it's enforced.
+  // We can enforce the first two here, but need to check the whitelist before

[Mattias Nissler (ping if slow), 2015/03/16 08:51:42]

I don't get why we can't update the whitelist here?

[Chris Masone, 2015/03/16 16:45:57]

I don't think we're actually assured that we've loaded the policy from the
session_manager yet at this point. So if there's already a whitelist in play, if
we just set it here, we might clobber it. No? That might allow to the
allow_new_users setting as well, huh?

[Mattias Nissler (ping if slow), 2015/03/17 14:05:53]

Ah, right, the |settings| may have come from elsewhere. However, that's only
true for the first write that uses |tentative_settings| accumulated before we
had a device owner. device_settings_service_->device_settings() as read in line
273 is guaranteed to be a validated settings block coming from session_manager
though. Thus, IMHO we should be fine.

One thing to consider however is the pending_changes_ mechanism. It may revert
any sanitization you apply here. Hence, it might be better to apply your
sanitization in StorePendingChanges() before (or within) the call to
AssemblePolicy, which is used to produce the settings blob that gets passed to
session_manager for writing.

[Chris Masone, 2015/03/17 15:00:48]

Yeah, the pending_changes_ stuff is why I did this how I did it. I figured by
going through (what seemed to be) the normal path, I'd just stack on top of any
pending changes and everything would be fine.

If I munge the protobuf directly in StorePendingChanges or AssemblePolicy (which
is actually what I was doing initially) my concern is that I might not get
called at all unless someone makes a Set() call somewhere. I need to force a
no-op change on key-load to ensure that the fixups get applied, something that
will actually wind up in pending_changes_ otherwise the code will bail early
somewhere.

[Mattias Nissler (ping if slow), 2015/03/17 15:15:57]

Well, I think it's fair to just force an initial write. If I'm not mistaken, 
CommitTentativeDeviceSettings gets called unconditionally after establishing
ownership anyways, and so you could just change StorePendingChanges() to always
continue if |tentative_changes_| is non-null.

+  // modifying it, so that will be taken care of in a separate class.
   em::PolicyData policy_data;
   policy_data.set_username(user_id_);
+  if (!settings.has_allow_new_users())
+    settings.mutable_allow_new_users()->set_allow_new_users(true);
+
   CHECK(settings.SerializeToString(policy_data.mutable_policy_value()));
   FOR_EACH_OBSERVER(OwnerSettingsService::Observer, observers_,
                     OnTentativeChangesInPolicy(policy_data));
   StorePendingChanges();
   return true;
 }
 
 bool OwnerSettingsServiceChromeOS::AppendToList(const std::string& setting,
                                                 const base::Value& value) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  const base::Value* old_value = CrosSettings::Get()->GetPref(setting);
+  const base::Value* old_value = cros_settings_->GetPref(setting);
   if (old_value && !old_value->IsType(base::Value::TYPE_LIST))
     return false;
   scoped_ptr<base::ListValue> new_value(
       old_value ? static_cast<const base::ListValue*>(old_value)->DeepCopy()
                 : new base::ListValue());
   new_value->Append(value.DeepCopy());
   return Set(setting, *new_value);
 }
 
 bool OwnerSettingsServiceChromeOS::RemoveFromList(const std::string& setting,
                                                   const base::Value& value) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  const base::Value* old_value = CrosSettings::Get()->GetPref(setting);
+  const base::Value* old_value = cros_settings_->GetPref(setting);
   if (old_value && !old_value->IsType(base::Value::TYPE_LIST))
     return false;
   scoped_ptr<base::ListValue> new_value(
       old_value ? static_cast<const base::ListValue*>(old_value)->DeepCopy()
                 : new base::ListValue());
   new_value->Remove(value, nullptr);
   return Set(setting, *new_value);
 }
 
 bool OwnerSettingsServiceChromeOS::CommitTentativeDeviceSettings(
@@ skipping 358 matching lines @@
 void OwnerSettingsServiceChromeOS::OnPostKeypairLoadedActions() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const user_manager::User* user =
       ProfileHelper::Get()->GetUserByProfile(profile_);
   user_id_ = user ? user->GetUserID() : std::string();
 
   const bool is_owner = IsOwner() || IsOwnerInTests(user_id_);
   if (is_owner && device_settings_service_)
     device_settings_service_->InitOwner(user_id_, weak_factory_.GetWeakPtr());
+
+  if (is_owner)
+    FixupLocalOwnerPolicy();
+}
+
+void OwnerSettingsServiceChromeOS::FixupLocalOwnerPolicy() {
+  if (CrosSettingsProvider::TRUSTED != cros_settings_->PrepareTrustedValues(
+        base::Bind(&OwnerSettingsServiceChromeOS::FixupLocalOwnerPolicy,
+                   weak_factory_.GetWeakPtr()))) {
+    return;
+  }
+  DCHECK(IsOwner());
+  DCHECK(!user_id_.empty());
+
+  bool wildcard = false;
+  if (!cros_settings_->FindEmailInList(kAccountsPrefUsers, user_id_,
+                                       &wildcard) ||
+      wildcard) {
+    cros_settings_->AppendToList(kAccountsPrefUsers,

[Mattias Nissler (ping if slow), 2015/03/16 08:51:42]

This is a bit of a step backwards, as the write path for owner settings is
supposed to move away from CrosSettings.

[Chris Masone, 2015/03/16 16:45:57]

Ah. How's it supposed to work, then?

[Mattias Nissler (ping if slow), 2015/03/17 14:05:53]

You could just update the protobuf field, i.e. |settings| in
StorePendingChanges().

+                                 new base::StringValue(user_id_));
+  }
 }
 
 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 =
@@ skipping 85 matching lines @@
   std::vector<OnManagementSettingsSetCallback> callbacks;
   pending_management_settings_callbacks_.swap(callbacks);
   for (const auto& callback : callbacks) {
     if (!callback.is_null())
       callback.Run(success);
   }
   StorePendingChanges();
 }
 
 }  // namespace chromeos