[login_manager] Code to add the owner to the whitelist in a device policy

Today, every time the owner logs in, we ensure he is on the whitelist and
that we have him recorded in the signed settings store as the owner.

Add code that can do the same thing for a device policy.

BUG=13842
TEST=unit tests.

Change-Id: I0073c7b0216879fb021f2319906ffd51bd54233f

Review URL: http://codereview.chromium.org/6815021
diff --git a/Makefile b/Makefile
index 4498f27..96aa5ef 100644
--- a/Makefile
+++ b/Makefile
@@ -22,10 +22,13 @@
 DBUS_CLIENT = $(BINDINGS)/client.h
 
 PROTO_PATH = $(ROOT)/usr/include/proto
-PROTO_DEFS = $(PROTO_PATH)/device_management_backend.proto
-PROTO_BINDINGS = $(BINDINGS)/device_management_backend.pb.cc
+PROTO_DEFS = $(PROTO_PATH)/chrome_device_policy.proto \
+	$(PROTO_PATH)/device_management_backend.proto
+PROTO_BINDINGS = $(BINDINGS)/chrome_device_policy.pb.cc \
+	 $(BINDINGS)/device_management_backend.pb.cc
 PROTO_HEADERS = $(patsubst %.cc,%.h,$(PROTO_BINDINGS))
-PROTO_OBJS = $(BINDINGS)/device_management_backend.pb.o
+PROTO_OBJS = $(BINDINGS)/chrome_device_policy.pb.o \
+	 $(BINDINGS)/device_management_backend.pb.o
 
 COMMON_OBJS = child_job.o interface.o nss_util.o owner_key.o \
 	owner_key_loss_mitigator.o pref_store.o system_utils.o \
diff --git a/device_policy.cc b/device_policy.cc
index bb98400..4b8f59e 100644
--- a/device_policy.cc
+++ b/device_policy.cc
@@ -11,12 +11,21 @@
 #include <base/file_util.h>
 #include <base/logging.h>
 
+#include "login_manager/bindings/chrome_device_policy.pb.h"
 #include "login_manager/bindings/device_management_backend.pb.h"
+#include "login_manager/owner_key.h"
 #include "login_manager/system_utils.h"
 
+namespace em = enterprise_management;
+
 namespace login_manager {
+using google::protobuf::RepeatedPtrField;
+using std::string;
+
 // static
 const char DevicePolicy::kDefaultPath[] = "/var/lib/whitelist/policy";
+// static
+const char DevicePolicy::kDevicePolicyType[] = "google/chromeos/device";
 
 DevicePolicy::DevicePolicy(const FilePath& policy_path)
     : policy_path_(policy_path) {
@@ -40,8 +49,8 @@
   return true;
 }
 
-bool DevicePolicy::Get(std::string* output) const {
-  return policy_.SerializeToString(output);
+const enterprise_management::PolicyFetchResponse& DevicePolicy::Get() const {
+  return policy_;
 }
 
 bool DevicePolicy::Persist() {
@@ -54,6 +63,10 @@
   return utils.AtomicFileWrite(policy_path_, polstr.c_str(), polstr.length());
 }
 
+bool DevicePolicy::SerializeToString(std::string* output) const {
+  return policy_.SerializeToString(output);
+}
+
 void DevicePolicy::Set(
     const enterprise_management::PolicyFetchResponse& policy) {
   policy_.Clear();
@@ -61,4 +74,62 @@
   policy_.CheckTypeAndMergeFrom(policy);
 }
 
+bool DevicePolicy::StoreOwnerProperties(OwnerKey* key,
+                                        const std::string& current_user,
+                                        GError** error) {
+  em::PolicyData poldata;
+  if (policy_.has_policy_data())
+    poldata.ParseFromString(policy_.policy_data());
+  em::ChromeDeviceSettingsProto polval;
+  if (poldata.has_policy_type() &&
+      poldata.policy_type() == kDevicePolicyType) {
+    if (poldata.has_policy_value())
+      polval.ParseFromString(poldata.policy_value());
+  } else {
+    poldata.set_policy_type(kDevicePolicyType);
+  }
+  // If there existed some device policy, we've got it now!
+  // Update the UserWhitelistProto inside the ChromeDeviceSettingsProto we made.
+  em::UserWhitelistProto* whitelist_proto = polval.mutable_user_whitelist();
+  bool on_whitelist = false;
+  const RepeatedPtrField<string>& whitelist = whitelist_proto->user_whitelist();
+  for (RepeatedPtrField<string>::const_iterator it = whitelist.begin();
+       it != whitelist.end();
+       ++it) {
+    if (on_whitelist = (current_user == *it))
+      break;
+  }
+  if (!on_whitelist)
+    whitelist_proto->add_user_whitelist(current_user);
+  bool current_user_is_owner = true;
+  // TODO(cmasone): once ChromeDeviceSettingsProto contains an owner name field
+  //                check that against |current_user| here.
+  if (current_user_is_owner && on_whitelist)
+    return true;  // No changes are needed.
+
+  // We have now updated the whitelist and owner setting in |polval|.
+  // We need to put it into |poldata|, serialize that, sign it, and
+  // put both into |policy_|.
+  poldata.set_policy_value(polval.SerializeAsString());
+  std::string new_data = poldata.SerializeAsString();
+  std::vector<uint8> sig;
+  const uint8* data = reinterpret_cast<const uint8*>(new_data.c_str());
+  if (!key || !key->Sign(data, new_data.length(), &sig)) {
+    SystemUtils utils;
+    const char err_msg[] = "Could not sign policy containing new owner data.";
+    LOG_IF(ERROR, error) << err_msg;
+    LOG_IF(WARNING, !error) << err_msg;
+    utils.SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, err_msg);
+    return false;
+  }
+
+  em::PolicyFetchResponse new_policy;
+  new_policy.CheckTypeAndMergeFrom(policy_);
+  new_policy.set_policy_data(new_data);
+  new_policy.set_policy_data_signature(
+      std::string(reinterpret_cast<const char*>(&sig[0]), sig.size()));
+  Set(new_policy);
+  return true;
+}
+
 }  // namespace login_manager
diff --git a/device_policy.h b/device_policy.h
index 9d1c705..c58760f 100644
--- a/device_policy.h
+++ b/device_policy.h
@@ -5,6 +5,7 @@
 #ifndef LOGIN_MANAGER_DEVICE_POLICY_H_
 #define LOGIN_MANAGER_DEVICE_POLICY_H_
 
+#include <glib.h>
 #include <string>
 
 #include <base/basictypes.h>
@@ -13,6 +14,7 @@
 #include "login_manager/bindings/device_management_backend.pb.h"
 
 namespace login_manager {
+class OwnerKey;
 
 // This class holds device settings that are to be enforced across all users.
 //
@@ -30,16 +32,30 @@
   // Returns true unless there is a policy on disk and loading it fails.
   virtual bool LoadOrCreate();
 
-  virtual bool Get(std::string* output) const;
+  virtual const enterprise_management::PolicyFetchResponse& Get() const;
 
   // Persist |policy_| to disk at |policy_file_|
   // Returns false if there's an error while writing data.
   virtual bool Persist();
 
+  virtual bool SerializeToString(std::string* output) const;
+
   // Clobber the stored policy with new data.
   virtual void Set(const enterprise_management::PolicyFetchResponse& policy);
 
+  // Assuming the current user has access to the owner private key
+  // (read: is the owner), this call whitelists |current_user_| and sets a
+  // property indicating |current_user_| is the owner in the current policy
+  // and schedules a PersistPolicy().
+  // Returns false on failure, with |error| set appropriately.
+  // |error| can be NULL, should you wish to ignore the particulars.
+  bool StoreOwnerProperties(OwnerKey* key,
+                            const std::string& current_user,
+                            GError** error);
+
   static const char kDefaultPath[];
+  // Format of this string is documented in device_management_backend.proto.
+  static const char kDevicePolicyType[];
 
  private:
   enterprise_management::PolicyFetchResponse policy_;
diff --git a/device_policy_unittest.cc b/device_policy_unittest.cc
index 9c195d6..dc391c4 100644
--- a/device_policy_unittest.cc
+++ b/device_policy_unittest.cc
@@ -10,9 +10,17 @@
 #include <base/scoped_temp_dir.h>
 #include <gtest/gtest.h>
 
+#include "login_manager/bindings/chrome_device_policy.pb.h"
 #include "login_manager/bindings/device_management_backend.pb.h"
+#include "login_manager/mock_owner_key.h"
+
+namespace em = enterprise_management;
 
 namespace login_manager {
+using google::protobuf::RepeatedPtrField;
+using std::string;
+using ::testing::Return;
+using ::testing::_;
 
 class DevicePolicyTest : public ::testing::Test {
  public:
@@ -49,10 +57,63 @@
     std::string serialized;
     ASSERT_TRUE(policy_.SerializeToString(&serialized));
     std::string serialized_from;
-    ASSERT_TRUE(store->Get(&serialized_from));
+    ASSERT_TRUE(store->SerializeToString(&serialized_from));
     EXPECT_EQ(serialized, serialized_from);
   }
 
+  void ExtractPolicyValue(const DevicePolicy& pol,
+                          em::ChromeDeviceSettingsProto* polval) {
+    em::PolicyData poldata;
+    ASSERT_TRUE(pol.Get().has_policy_data());
+    ASSERT_TRUE(poldata.ParseFromString(pol.Get().policy_data()));
+    ASSERT_TRUE(poldata.has_policy_type());
+    ASSERT_EQ(poldata.policy_type(), DevicePolicy::kDevicePolicyType);
+    ASSERT_TRUE(poldata.has_policy_value());
+    ASSERT_TRUE(polval->ParseFromString(poldata.policy_value()));
+  }
+
+  int CountOwnerInWhitelist(const DevicePolicy& pol, const std::string& owner) {
+    em::ChromeDeviceSettingsProto polval;
+    ExtractPolicyValue(pol, &polval);
+    const em::UserWhitelistProto& whitelist_proto = polval.user_whitelist();
+    int whitelist_count = 0;
+    const RepeatedPtrField<std::string>& whitelist =
+        whitelist_proto.user_whitelist();
+    for (RepeatedPtrField<std::string>::const_iterator it = whitelist.begin();
+         it != whitelist.end();
+         ++it) {
+      whitelist_count += (owner == *it ? 1 : 0);
+    }
+    return whitelist_count;
+  }
+
+  em::PolicyFetchResponse Wrap(const em::ChromeDeviceSettingsProto& polval) {
+    em::PolicyData new_poldata;
+    new_poldata.set_policy_type(DevicePolicy::kDevicePolicyType);
+    new_poldata.set_policy_value(polval.SerializeAsString());
+    em::PolicyFetchResponse new_policy;
+    new_policy.set_policy_data(new_poldata.SerializeAsString());
+    return new_policy;
+  }
+
+  em::PolicyFetchResponse CreateWithOwner(const std::string& owner) {
+    em::ChromeDeviceSettingsProto new_polval;
+    new_polval.mutable_user_whitelist()->add_user_whitelist(owner);
+    return Wrap(new_polval);
+  }
+
+  em::PolicyFetchResponse CreateWithWhitelist(
+      const std::vector<std::string>& users) {
+    em::ChromeDeviceSettingsProto polval;
+    em::UserWhitelistProto* whitelist_proto = polval.mutable_user_whitelist();
+    for(std::vector<std::string>::const_iterator it = users.begin();
+        it != users.end();
+        ++it) {
+      whitelist_proto->add_user_whitelist(*it);
+    }
+    return Wrap(polval);
+  }
+
   static const char kDefaultPolicy[];
 
   ScopedTempDir tmpdir_;
@@ -70,9 +131,9 @@
 TEST_F(DevicePolicyTest, CreateEmptyStore) {
   StartFresh();
   DevicePolicy store(tmpfile_);
-  ASSERT_TRUE(store.LoadOrCreate());  // Should create an empty DictionaryValue.
+  ASSERT_TRUE(store.LoadOrCreate());  // Should create an empty policy.
   std::string serialized;
-  EXPECT_TRUE(store.Get(&serialized));
+  EXPECT_TRUE(store.SerializeToString(&serialized));
   EXPECT_TRUE(serialized.empty());
 }
 
@@ -95,7 +156,7 @@
   store_->Set(new_policy);
 
   std::string new_out;
-  ASSERT_TRUE(store_->Get(&new_out));
+  ASSERT_TRUE(store_->SerializeToString(&new_out));
   std::string new_value;
   ASSERT_TRUE(new_policy.SerializeToString(&new_value));
   EXPECT_EQ(new_value, new_out);
@@ -107,4 +168,50 @@
   CheckExpectedPolicy(&store2);
 }
 
+TEST_F(DevicePolicyTest, FreshPolicy) {
+  StartFresh();
+  DevicePolicy pol(tmpfile_);
+  ASSERT_TRUE(pol.LoadOrCreate());  // Should create an empty policy.
+
+  std::string current_user("me");
+  scoped_ptr<MockOwnerKey> key(new MockOwnerKey);
+  EXPECT_CALL(*key.get(), Sign(_, _, _))
+      .WillOnce(Return(true));
+  pol.StoreOwnerProperties(key.get(), current_user, NULL);
+
+  ASSERT_EQ(CountOwnerInWhitelist(pol, current_user), 1);
+}
+
+TEST_F(DevicePolicyTest, OwnerAlreadyInPolicy) {
+  StartFresh();
+  DevicePolicy pol(tmpfile_);
+  ASSERT_TRUE(pol.LoadOrCreate());  // Should create an empty policy.
+
+  std::string current_user("me");
+  pol.Set(CreateWithOwner(current_user));
+
+  scoped_ptr<MockOwnerKey> key(new MockOwnerKey);
+  pol.StoreOwnerProperties(key.get(), current_user, NULL);
+
+  ASSERT_EQ(CountOwnerInWhitelist(pol, current_user), 1);
+}
+
+TEST_F(DevicePolicyTest, ExistingPolicy) {
+  StartFresh();
+  DevicePolicy pol(tmpfile_);
+  ASSERT_TRUE(pol.LoadOrCreate());  // Should create an empty policy.
+
+  std::string current_user("me");
+  const char* users[] = { "you", "him", "her" };
+  std::vector<std::string> default_whitelist(users, users + arraysize(users));
+  pol.Set(CreateWithWhitelist(default_whitelist));
+
+  scoped_ptr<MockOwnerKey> key(new MockOwnerKey);
+  EXPECT_CALL(*key.get(), Sign(_, _, _))
+      .WillOnce(Return(true));
+  pol.StoreOwnerProperties(key.get(), current_user, NULL);
+
+  ASSERT_EQ(CountOwnerInWhitelist(pol, current_user), 1);
+}
+
 }  // namespace login_manager
diff --git a/mock_device_policy.h b/mock_device_policy.h
index 75ebff5..7b70f3a 100644
--- a/mock_device_policy.h
+++ b/mock_device_policy.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -17,7 +17,8 @@
   virtual ~MockDevicePolicy() {}
   MOCK_METHOD0(LoadOrCreate, bool(void));
   MOCK_METHOD0(Persist, bool(void));
-  MOCK_CONST_METHOD1(Get, bool(std::string*));
+  MOCK_CONST_METHOD0(Get, const enterprise_management::PolicyFetchResponse&());
+  MOCK_CONST_METHOD1(SerializeToString, bool(std::string*));
   MOCK_METHOD1(Set, void(const enterprise_management::PolicyFetchResponse&));
 };
 }  // namespace login_manager
diff --git a/mock_owner_key.h b/mock_owner_key.h
index 824da9c..6ba270d 100644
--- a/mock_owner_key.h
+++ b/mock_owner_key.h
@@ -21,8 +21,8 @@
   virtual ~MockOwnerKey() {}
   MOCK_CONST_METHOD1(Equals, bool(const std::string&));
   MOCK_CONST_METHOD1(VEquals, bool(const std::vector<uint8>&));
-  MOCK_METHOD0(HaveCheckedDisk, bool());
-  MOCK_METHOD0(IsPopulated, bool());
+  MOCK_CONST_METHOD0(HaveCheckedDisk, bool());
+  MOCK_CONST_METHOD0(IsPopulated, bool());
   MOCK_METHOD0(PopulateFromDiskIfPossible, bool());
   MOCK_METHOD1(PopulateFromBuffer, bool(const std::vector<uint8>&));
   MOCK_METHOD1(PopulateFromKeypair, bool(base::RSAPrivateKey*));
diff --git a/owner_key.cc b/owner_key.cc
index b06ecdd..369ee63 100644
--- a/owner_key.cc
+++ b/owner_key.cc
@@ -44,9 +44,9 @@
           memcmp(&key_der[0], &key_[0], key_.size()) == 0);
 }
 
-bool OwnerKey::HaveCheckedDisk() { return have_checked_disk_; }
+bool OwnerKey::HaveCheckedDisk() const { return have_checked_disk_; }
 
-bool OwnerKey::IsPopulated() { return !key_.empty(); }
+bool OwnerKey::IsPopulated() const { return !key_.empty(); }
 
 bool OwnerKey::PopulateFromDiskIfPossible() {
   have_checked_disk_ = true;
diff --git a/owner_key.h b/owner_key.h
index c35112f..303aca4 100644
--- a/owner_key.h
+++ b/owner_key.h
@@ -34,8 +34,8 @@
 
   virtual bool Equals(const std::string& key_der) const;
   virtual bool VEquals(const std::vector<uint8>& key_der) const;
-  virtual bool HaveCheckedDisk();
-  virtual bool IsPopulated();
+  virtual bool HaveCheckedDisk() const;
+  virtual bool IsPopulated() const;
 
   // If |key_file_| exists, populate the object with the contents of the file.
   // If the file isn't there, that's ok.
diff --git a/session_manager_service.cc b/session_manager_service.cc
index 25afa85..e02b5f5 100644
--- a/session_manager_service.cc
+++ b/session_manager_service.cc
@@ -49,6 +49,7 @@
 }  // namespace gobject
 }  // namespace login_manager
 
+namespace em = enterprise_management;
 namespace login_manager {
 
 using std::make_pair;
@@ -478,7 +479,7 @@
   if (session_started_) {
     const char msg[] = "Can't start session while session is already active.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_SESSION_EXISTS, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_SESSION_EXISTS, msg);
     return *OUT_done = FALSE;
   }
   if (!ValidateAndCacheUserEmail(email_address, error)) {
@@ -486,15 +487,14 @@
     return FALSE;
   }
   // If the current user is the owner, and isn't whitelisted or set
-  // as the cros.device.owner pref, then do so.  This attempt only succeeds
-  // if the current user has access to the private half of the owner's
-  // registered public key.
-  StoreOwnerProperties(NULL);
+  // as the cros.device.owner pref, then do so.
+  bool can_access_key = CurrentUserHasOwnerKey(key_->public_key_der(), error);
+  if (can_access_key)
+    StoreOwnerProperties(NULL);
   // Now, the flip side...if we believe the current user to be the owner
   // based on the cros.owner.device setting, and he DOESN'T have the private
   // half of the public key, we must mitigate.
-  if (CurrentUserIsOwner(error) &&
-      !CurrentUserHasOwnerKey(key_->public_key_der(), error)) {
+  if (CurrentUserIsOwner() && !can_access_key) {
     if (!(*OUT_done = mitigator_->Mitigate()))
       return FALSE;
   }
@@ -610,7 +610,7 @@
                                             GError** error) {
   const char msg[] = "The session_manager now sets the Owner's public key.";
   LOG(ERROR) << msg;
-  SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, msg);
+  system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, msg);
   // Just to be safe, send back a nACK in addition to returning an error.
   SendSignal(chromium::kOwnerKeySetSignal, false);
   return FALSE;
@@ -625,12 +625,12 @@
   if (verify_result == NO_KEY) {
     const char msg[] = "Attempt to unwhitelist before owner's key is set.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_NO_OWNER_KEY, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_NO_OWNER_KEY, msg);
     return FALSE;
   } else if (verify_result == SIGNATURE_FAIL) {
     const char msg[] = "Signature could not be verified in Unwhitelist.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
     return FALSE;
   }
   store_->Unwhitelist(email_address);
@@ -647,14 +647,14 @@
   if (!store_->GetFromWhitelist(email_address, &encoded)) {
     const char msg[] = "The user is not whitelisted.";
     LOG(INFO) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_USER, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_USER, msg);
     return FALSE;
   }
   std::string decoded;
   if (!base::Base64Decode(encoded, &decoded)) {
     const char msg[] = "Signature could not be decoded in CheckWhitelist.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_DECODE_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_DECODE_FAIL, msg);
     return FALSE;
   }
 
@@ -687,12 +687,12 @@
   if (verify_result == NO_KEY) {
     const char msg[] = "Attempt to whitelist before owner's key is set.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_NO_OWNER_KEY, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_NO_OWNER_KEY, msg);
     return FALSE;
   } else if (verify_result == SIGNATURE_FAIL) {
     const char msg[] = "Signature could not be verified in Whitelist.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
     return FALSE;
   }
   std::string data(signature->data, signature->len);
@@ -709,12 +709,12 @@
   if (verify_result == NO_KEY) {
     const char msg[] = "Attempt to store property before owner's key is set.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_NO_OWNER_KEY, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_NO_OWNER_KEY, msg);
     return FALSE;
   } else if (verify_result == SIGNATURE_FAIL) {
     const char msg[] = "Signature could not be verified in StoreProperty.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
     return FALSE;
   }
   std::string data(signature->data, signature->len);
@@ -745,13 +745,13 @@
 gboolean SessionManagerService::StorePolicy(GArray* policy_blob,
                                             DBusGMethodInvocation* context) {
   std::string policy_str(policy_blob->data, policy_blob->len);
-  enterprise_management::PolicyFetchResponse policy;
+  em::PolicyFetchResponse policy;
   if (!policy.ParseFromString(policy_str) ||
       !policy.has_policy_data() ||
       !policy.has_policy_data_signature()) {
     const char msg[] = "Unable to parse policy protobuf.";
     LOG(ERROR) << msg;
-    SetAndSendGError(CHROMEOS_LOGIN_ERROR_DECODE_FAIL, context, msg);
+    system_->SetAndSendGError(CHROMEOS_LOGIN_ERROR_DECODE_FAIL, context, msg);
     return FALSE;
   }
 
@@ -773,7 +773,9 @@
       if (!rotated) {
         const char msg[] = "Failed attempted key rotation!";
         LOG(ERROR) << msg;
-        SetAndSendGError(CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, context, msg);
+        system_->SetAndSendGError(CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY,
+                                 context,
+                                 msg);
         return FALSE;
       }
     } else {
@@ -801,7 +803,7 @@
   } else if (verify_result == SIGNATURE_FAIL) {
     const char msg[] = "Signature could not be verified in StorePolicy.";
     LOG(ERROR) << msg;
-    SetAndSendGError(CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, context, msg);
+    system_->SetAndSendGError(CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, context, msg);
     return FALSE;
   }
   policy_->Set(policy);
@@ -814,17 +816,17 @@
 gboolean SessionManagerService::RetrievePolicy(GArray** OUT_policy_blob,
                                                GError** error) {
   std::string polstr;
-  if (!policy_->Get(&polstr)) {
+  if (!policy_->SerializeToString(&polstr)) {
     const char msg[] = "Unable to serialize policy protobuf.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ENCODE_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ENCODE_FAIL, msg);
     return FALSE;
   }
   *OUT_policy_blob = g_array_sized_new(FALSE, FALSE, 1, polstr.length());
   if (!*OUT_policy_blob) {
     const char msg[] = "Unable to allocate memory for response.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_DECODE_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_DECODE_FAIL, msg);
     return FALSE;
   }
   g_array_append_vals(*OUT_policy_blob, polstr.c_str(), polstr.length());
@@ -860,7 +862,7 @@
     *OUT_done = FALSE;
     const char msg[] = "Provided pid is unknown.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_UNKNOWN_PID, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_UNKNOWN_PID, msg);
     return FALSE;
   }
 
@@ -1026,23 +1028,6 @@
                                    what_happened));
 }
 
-// static
-void SessionManagerService::SetGError(GError** error,
-                                      ChromeOSLoginError code,
-                                      const char* message) {
-  g_set_error(error, CHROMEOS_LOGIN_ERROR, code, "Login error: %s", message);
-}
-
-// static
-void SessionManagerService::SetAndSendGError(ChromeOSLoginError code,
-                                             DBusGMethodInvocation* context,
-                                             const char* msg) {
-  GError* error = NULL;
-  SetGError(&error, code, msg);
-  dbus_g_method_return_error(context, error);
-  g_error_free(error);
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // Utility Methods
 
@@ -1136,18 +1121,16 @@
   CHECK(sigaction(SIGHUP, &action, NULL) == 0);
 }
 
-gboolean SessionManagerService::CurrentUserIsOwner(GError** error) {
+gboolean SessionManagerService::CurrentUserIsOwner() {
   std::string value;
   std::string decoded;
-  if (!GetPropertyHelper(kDeviceOwnerPref, &value, &decoded, error))
+  if (!GetPropertyHelper(kDeviceOwnerPref, &value, &decoded, NULL))
     return FALSE;
   std::string was_signed = base::StringPrintf("%s=%s",
                                               kDeviceOwnerPref,
                                               value.c_str());
   if (VerifyHelper(was_signed, decoded.c_str(), decoded.length()) != SUCCESS) {
-    const char msg[] = "Owner pref signature could not be verified.";
-    LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_VERIFY_FAIL, msg);
+    LOG(ERROR) << "Owner pref signature could not be verified.";
     return FALSE;
   }
   return value == current_user_;
@@ -1159,13 +1142,13 @@
   if (!nss_->OpenUserDB()) {
     const char msg[] = "Could not open the current user's NSS database.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_NO_USER_NSSDB, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_NO_USER_NSSDB, msg);
     return FALSE;
   }
   if (!nss_->GetPrivateKey(pub_key)) {
     const char msg[] = "Could not verify that public key belongs to the owner.";
     LOG(WARNING) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, msg);
     return FALSE;
   }
   return TRUE;
@@ -1183,7 +1166,7 @@
   if (email_string != kIncognitoUser && !ValidateEmail(email_string)) {
     const char msg[] = "Provided email address is not valid.  ASCII only.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_INVALID_EMAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_INVALID_EMAIL, msg);
     return FALSE;
   }
   current_user_ = StringToLowerASCII(email_string);
@@ -1236,7 +1219,7 @@
 
 gboolean SessionManagerService::SignAndStoreProperty(const std::string& name,
                                                      const std::string& value,
-                                                     const std::string& err_msg,
+                                                     const std::string& msg,
                                                      GError** error) {
   std::vector<uint8> signature;
   std::string to_sign = base::StringPrintf("%s=%s",
@@ -1244,9 +1227,9 @@
                                            current_user_.c_str());
   const uint8* data = reinterpret_cast<const uint8*>(to_sign.c_str());
   if (!key_->Sign(data, to_sign.length(), &signature)) {
-    LOG_IF(ERROR, error) << err_msg;
-    LOG_IF(WARNING, !error) << err_msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, err_msg.c_str());
+    LOG_IF(ERROR, error) << msg;
+    LOG_IF(WARNING, !error) << msg;
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, msg.c_str());
     return FALSE;
   }
   std::string signature_string(reinterpret_cast<const char*>(&signature[0]),
@@ -1258,14 +1241,14 @@
 }
 
 gboolean SessionManagerService::SignAndWhitelist(const std::string& email,
-                                                 const std::string& err_msg,
+                                                 const std::string& msg,
                                                  GError** error) {
   std::vector<uint8> signature;
   const uint8* data = reinterpret_cast<const uint8*>(current_user_.c_str());
   if (!key_->Sign(data, current_user_.length(), &signature)) {
-    LOG_IF(ERROR, error) << err_msg;
-    LOG_IF(WARNING, !error) << err_msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, err_msg.c_str());
+    LOG_IF(ERROR, error) << msg;
+    LOG_IF(WARNING, !error) << msg;
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, msg.c_str());
     return FALSE;
   }
   std::string signature_string(reinterpret_cast<const char*>(&signature[0]),
@@ -1281,7 +1264,7 @@
   if (!base::Base64Encode(signature, &encoded)) {
     const char msg[] = "Signature could not be encoded.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ENCODE_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ENCODE_FAIL, msg);
     return FALSE;
   }
   store_->Set(name, value, encoded);
@@ -1317,7 +1300,7 @@
   if (!base::Base64Encode(signature, &encoded)) {
     const char msg[] = "Signature could not be encoded.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_ENCODE_FAIL, msg);
+    system_->SetGError(error, CHROMEOS_LOGIN_ERROR_ENCODE_FAIL, msg);
     return FALSE;
   }
   store_->Whitelist(email, encoded);
@@ -1330,20 +1313,20 @@
 gboolean SessionManagerService::GetPropertyHelper(const std::string& name,
                                                   std::string* OUT_value,
                                                   std::string* OUT_signature,
-                                                  GError** error) {
+                                                  GError** err) {
   std::string encoded;
   if (!store_->Get(name, OUT_value, &encoded)) {
-    std::string error_msg =
+    std::string msg =
         base::StringPrintf("The requested property %s is unknown.",
                            name.c_str());
-    LOG(WARNING) << error_msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_UNKNOWN_PROPERTY, error_msg.c_str());
+    LOG(WARNING) << msg;
+    system_->SetGError(err, CHROMEOS_LOGIN_ERROR_UNKNOWN_PROPERTY, msg.c_str());
     return FALSE;
   }
   if (!base::Base64Decode(encoded, OUT_signature)) {
     const char msg[] = "Signature could not be decoded.";
     LOG(ERROR) << msg;
-    SetGError(error, CHROMEOS_LOGIN_ERROR_DECODE_FAIL, msg);
+    system_->SetGError(err, CHROMEOS_LOGIN_ERROR_DECODE_FAIL, msg);
     return FALSE;
   }
   return TRUE;
diff --git a/session_manager_service.h b/session_manager_service.h
index 6f4fba4..46f772c 100644
--- a/session_manager_service.h
+++ b/session_manager_service.h
@@ -374,23 +374,12 @@
   // |data| is a SessionManagerService*
   static gboolean ServiceShutdown(gpointer data);
 
-  // Initializes |error| with |code| and |message|.
-  static void SetGError(GError** error,
-                        ChromeOSLoginError code,
-                        const char* message);
-
-  // Initializes |error| with |code| and |message|.
-  static void SetAndSendGError(ChromeOSLoginError code,
-                               DBusGMethodInvocation* context,
-                               const char* message);
-
   // Setup any necessary signal handlers.
   void SetupHandlers();
 
   // Returns true if the current user is listed in |store_| as the
   // kDeviceOwner.  Returns false if not, or if that cannot be determined.
-  // |error| is set appropriately on failure.
-  gboolean CurrentUserIsOwner(GError** error);
+  gboolean CurrentUserIsOwner();
 
   // Returns true if the current user has the private half of |pub_key|
   // in his nssdb.  Returns false if not, or if that cannot be determined.
@@ -415,6 +404,7 @@
   // property indicating |current_user_| is the owner, and schedules both
   // a PersistWhitelist() and a PersistStore().
   // Returns false on failure, with |error| set appropriately.
+  // |error| can be NULL, should you wish to ignore the particulars.
   gboolean StoreOwnerProperties(GError** error);
 
   // Signs and stores |name|=|value|, and schedules a PersistStore().
diff --git a/session_manager_unittest.cc b/session_manager_unittest.cc
index daa6343..be966ea 100644
--- a/session_manager_unittest.cc
+++ b/session_manager_unittest.cc
@@ -164,12 +164,7 @@
     MockOwnerKey* key = new MockOwnerKey;
     EXPECT_CALL(*key, PopulateFromDiskIfPossible())
         .WillRepeatedly(Return(true));
-    // First, expect an attempt to set the device owner property, but
-    // act like this user isn't the owner.
-    EXPECT_CALL(*key, Sign(_, _, _))
-        .WillOnce(Return(false));
-    manager_->test_api().set_ownerkey(key);
-    // Now, expect an attempt to check whether this user is the owner; respond
+    // Expect an attempt to check whether this user is the owner; respond
     // as though he is not.
     std::string other_user("notme");
     EXPECT_CALL(*store, Get(_, _, _))
@@ -183,6 +178,7 @@
     EXPECT_CALL(*key, IsPopulated())
         .WillOnce(Return(true))
         .WillOnce(Return(true));
+    manager_->test_api().set_ownerkey(key);
   }
 
   void ExpectStartSessionUnowned(const std::string& email_string,
@@ -208,9 +204,6 @@
         .WillRepeatedly(Return(true));
     EXPECT_CALL(*key, StartGeneration(k_job))
         .WillOnce(Return(keygen_pid));
-    // act like this user isn't the owner.
-    EXPECT_CALL(*key, Sign(_, _, _))
-        .WillOnce(Return(false));
 
     // Now, expect an attempt to check whether this user is the owner; respond
     // as though there isn't one.
@@ -236,25 +229,30 @@
 
   void ExpectStartSessionForOwner(const std::string& email_string,
                                   MockOwnerKey* key,
-                                  MockPrefStore* store) {
+                                  MockPrefStore* store,
+                                  bool has_key) {
     ON_CALL(*key, PopulateFromDiskIfPossible())
         .WillByDefault(Return(true));
-    // First, mimic attempt to whitelist the owner and set a the
-    // device owner pref.
-    EXPECT_CALL(*key, Sign(_, _, _))
-        .WillOnce(Return(true))
-        .RetiresOnSaturation();
-    EXPECT_CALL(*store, Set(_, email_string, _))
-        .Times(1);
-    EXPECT_CALL(*key, Sign(CastEq(email_string), email_string.length(), _))
-        .WillOnce(Return(true))
-        .RetiresOnSaturation();
-    EXPECT_CALL(*store, Whitelist(email_string, _))
-        .Times(1);
+    int persist_times = 1;
+    if (has_key) {
+      // First, mimic attempt to whitelist the owner and set the
+      // device owner pref.
+      EXPECT_CALL(*key, Sign(_, _, _))
+          .WillOnce(Return(true))
+          .RetiresOnSaturation();
+      EXPECT_CALL(*store, Set(_, email_string, _))
+          .Times(1);
+      EXPECT_CALL(*key, Sign(CastEq(email_string), email_string.length(), _))
+          .WillOnce(Return(true))
+          .RetiresOnSaturation();
+      EXPECT_CALL(*store, Whitelist(email_string, _))
+          .Times(1);
+      persist_times = 3;
+    }
     EXPECT_CALL(*store, Persist())
-        .WillOnce(Return(true))
-        .WillOnce(Return(true))
-        .WillOnce(Return(true));
+        .Times(persist_times)
+        .WillRepeatedly(Return(true));
+
     // Now, expect an attempt to check whether this user is the owner;
     // respond as though he is.
     EXPECT_CALL(*store, Get(_, _, _))
@@ -506,7 +504,7 @@
   int pid = fork();
   if (pid == 0) {
     execl("./keygen", "./keygen", key_file_path.value().c_str(), NULL);
-    exit(1);
+    exit(255);
   }
   int status;
   while (waitpid(pid, &status, 0) == -1 && errno == EINTR)
@@ -518,13 +516,13 @@
             << "  WIFEXITED is " << WIFEXITED(status) << "\n"
             << "  WEXITSTATUS is " << WEXITSTATUS(status);
 
-  ASSERT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0);
-  ASSERT_TRUE(file_util::PathExists(key_file_path));
+  EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0);
+  EXPECT_TRUE(file_util::PathExists(key_file_path));
 
   SystemUtils utils;
   int32 file_size = 0;
-  ASSERT_TRUE(utils.EnsureAndReturnSafeFileSize(key_file_path, &file_size));
-  ASSERT_GT(file_size, 0);
+  EXPECT_TRUE(utils.EnsureAndReturnSafeFileSize(key_file_path, &file_size));
+  EXPECT_GT(file_size, 0);
 }
 
 TEST_F(SessionManagerTest, SessionNotStartedCleanup) {
@@ -639,6 +637,9 @@
 }
 
 TEST_F(SessionManagerTest, StartSession) {
+  MockFactory<KeyFailUtil> factory;
+  NssUtil::set_factory(&factory);
+
   MockChildJob* job = CreateTrivialMockJob(MAYBE_NEVER);
 
   gboolean out;
@@ -651,6 +652,9 @@
 }
 
 TEST_F(SessionManagerTest, StartSessionNew) {
+  MockFactory<KeyFailUtil> factory;
+  NssUtil::set_factory(&factory);
+
   gboolean out;
   gchar email[] = "user@somewhere";
   gchar nothing[] = "";
@@ -689,7 +693,7 @@
   MockOwnerKey* key = new MockOwnerKey;
   MockPrefStore* store = new MockPrefStore;
   MockDevicePolicy* policy = new MockDevicePolicy;
-  ExpectStartSessionForOwner(email, key, store);
+  ExpectStartSessionForOwner(email, key, store, true);
   EXPECT_CALL(*policy, Persist())
       .WillOnce(Return(true));
 
@@ -712,22 +716,12 @@
   gchar nothing[] = "";
 
   MockChildJob* job = CreateTrivialMockJob(MAYBE_NEVER);
-  EXPECT_CALL(*(utils_.get()),
-              SendSignalToChromium(chromium::kPropertyChangeCompleteSignal,
-                                   StrEq("success")))
-      .Times(1);
-  EXPECT_CALL(*(utils_.get()),
-              SendSignalToChromium(chromium::kWhitelistChangeCompleteSignal,
-                                   StrEq("success")))
-      .Times(1);
-  MockUtils();
-
   EXPECT_CALL(*mitigator_, Mitigate())
       .WillOnce(Return(false));
   MockOwnerKey* key = new MockOwnerKey;
   MockPrefStore* store = new MockPrefStore;
   MockDevicePolicy* policy = new MockDevicePolicy;
-  ExpectStartSessionForOwner(email, key, store);
+  ExpectStartSessionForOwner(email, key, store, false);
   EXPECT_CALL(*policy, Persist())
       .WillOnce(Return(true));
 
diff --git a/system_utils.cc b/system_utils.cc
index eee0e14..181f4fd 100644
--- a/system_utils.cc
+++ b/system_utils.cc
@@ -148,4 +148,19 @@
   appender.Run();
 }
 
+void SystemUtils::SetGError(GError** error,
+                            ChromeOSLoginError code,
+                            const char* message) {
+  g_set_error(error, CHROMEOS_LOGIN_ERROR, code, "Login error: %s", message);
+}
+
+void SystemUtils::SetAndSendGError(ChromeOSLoginError code,
+                                   DBusGMethodInvocation* context,
+                                   const char* msg) {
+  GError* error = NULL;
+  SetGError(&error, code, msg);
+  dbus_g_method_return_error(context, error);
+  g_error_free(error);
+}
+
 }  // namespace login_manager
diff --git a/system_utils.h b/system_utils.h
index cd4479b..3369ddc 100644
--- a/system_utils.h
+++ b/system_utils.h
@@ -5,11 +5,15 @@
 #ifndef LOGIN_MANAGER_SYSTEM_UTILS_H_
 #define LOGIN_MANAGER_SYSTEM_UTILS_H_
 
+#include <dbus/dbus.h>
+#include <dbus/dbus-glib.h>
+#include <glib.h>
 #include <unistd.h>
 #include <string>
 
 #include <base/basictypes.h>
 #include <base/stringprintf.h>
+#include <chromeos/dbus/service_constants.h>
 
 class FilePath;
 
@@ -62,6 +66,16 @@
   // persisted across stateful partition wipes.
   virtual void AppendToClobberLog(const char* msg) const;
 
+  // Initializes |error| with |code| and |message|.
+  virtual void SetGError(GError** error,
+                         ChromeOSLoginError code,
+                         const char* message);
+
+  // Initializes |error| with |code| and |message|.
+  virtual void SetAndSendGError(ChromeOSLoginError code,
+                                DBusGMethodInvocation* context,
+                                const char* message);
+
  private:
   // If this file exists on the next boot, the stateful partition will be wiped.
   static const char kResetFile[];