Fix up Owner settings on first load

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

Index: chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
index 48a81cb418a7dca2df0e0da89efb161a3cb84440..e8abe31e4daa20665d4af78531dc4f93f6785325 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 
+#include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ -15,7 +16,6 @@
 #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"
@@ -95,6 +95,7 @@ void LoadPrivateKey(
         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,
@@ -104,6 +105,7 @@ void LoadPrivateKey(
   }
   public_key = new PublicKey();
   public_key->data().swap(public_key_data);
+
   bool rv = BrowserThread::PostTask(BrowserThread::IO,
                                     FROM_HERE,
                                     base::Bind(&LoadPrivateKeyByPublicKey,
@@ -174,7 +176,6 @@ bool CheckManagementModeTransition(policy::ManagementMode current_mode,
   NOTREACHED();
   return false;
 }
-
 }  // namespace
 
 OwnerSettingsServiceChromeOS::ManagementSettings::ManagementSettings() {
@@ -185,10 +186,12 @@ 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),
@@ -270,8 +273,17 @@ bool OwnerSettingsServiceChromeOS::Set(const std::string& setting,
     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));
@@ -282,7 +294,7 @@ bool OwnerSettingsServiceChromeOS::Set(const std::string& setting,
 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(
@@ -295,7 +307,7 @@ bool OwnerSettingsServiceChromeOS::AppendToList(const std::string& setting,
 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(
@@ -674,6 +686,27 @@ void OwnerSettingsServiceChromeOS::OnPostKeypairLoadedActions() {
   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<