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

device_policy.cc

Index: device_policy.cc
diff --git a/device_policy.cc b/device_policy.cc
index bb9840085096b6bfa1c692d7a6ab5c6419fd04bf..46c96b9fde49a2bc44ee8082415b6308154f50e8 100644
--- a/device_policy.cc
+++ b/device_policy.cc
@@ -12,11 +12,20 @@
 #include <base/logging.h>
 
 #include "login_manager/bindings/device_management_backend.pb.h"
+#include "login_manager/bindings/chrome_device_policy.pb.h"

[gauravsh, 2011/04/08 04:58:49]

nit: alpha order of #includes.

[Chris Masone, 2011/04/08 05:57:41]

Done.

 #include "login_manager/system_utils.h"
+#include "login_manager/owner_key.h"

[gauravsh, 2011/04/08 04:58:49]

nit: alpha order of includes.

[Chris Masone, 2011/04/08 05:57:41]

Done.

+
+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 @@ bool DevicePolicy::LoadOrCreate() {
   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 @@ bool DevicePolicy::Persist() {
   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,59 @@ void DevicePolicy::Set(
   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!
+  // Updtae the UserWhitelistProto inside the ChromeDeviceSettingsProto we made.

[gauravsh, 2011/04/08 04:58:49]

Update

[Chris Masone, 2011/04/08 05:57:41]

Done.

+  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) {
+    on_whitelist = (current_user == *it);

[gauravsh, 2011/04/08 04:58:49]

you should break here if on_whitelist gets set to true. Otherwise os_whitelist
is always set to the last comparison.

[Chris Masone, 2011/04/08 05:57:41]

Done.

+  }
+  if (!on_whitelist)
+    whitelist_proto->add_user_whitelist(current_user);
+  bool current_user_is_owner = true;
+
+  if (current_user_is_owner && on_whitelist)

[gauravsh, 2011/04/08 04:58:49]

you set current_user_is_owner in the line above? is that temporary? If so, add a
todo. Otherwise, explain yourself.

[Chris Masone, 2011/04/08 05:57:41]

TODO added

+    return TRUE;  // No changes are needed.

[gauravsh, 2011/04/08 04:58:49]

holy capslock batman! true?

(After going through the other files in the CL, you seem to use TRUE/FALSE
usually. However, in this file, there are some places you use true/false, and
TRUE/FALSE elsewhere. Change to be consistent.)

[Chris Masone, 2011/04/08 05:57:41]

TRUE/FALSE are the gboolean values.  I usually use true/false where I need a
bool, but this originally returned a gboolean.  I've changed to use true/false
now, to match the function prototype.

+
+  // |polval| now has what we want in it.  We need to put it into

[gauravsh, 2011/04/08 04:58:49]

Confusing comment. |polval| was set much further above on line 87. This comment
implies that it happened just now. Maybe get rid of 'now'?

[Chris Masone, 2011/04/08 05:57:41]

Done.

+  // |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->Sign(data, new_data.length(), &sig)) {

[gauravsh, 2011/04/08 04:58:49]

should you NULL-check key?

[Chris Masone, 2011/04/08 05:57:41]

Done.

+    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);

[gauravsh, 2011/04/08 04:58:49]

so a NULL |error| will be handled correctly by SetGError?

[Chris Masone, 2011/04/08 05:57:41]

by g_set_error inside SetGError, actually.

+    return FALSE;

[gauravsh, 2011/04/08 04:58:49]

false?

[Chris Masone, 2011/04/08 05:57:41]

Done.

+  }
+
+  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;

[gauravsh, 2011/04/08 04:58:49]

true?

[Chris Masone, 2011/04/08 05:57:41]

Done.

+}
+
 }  // namespace login_manager