PART2: Make SignedSettings use proper Value types instead of string all around the place.

chrome/browser/chromeos/login/signed_settings.cc

 // Copyright (c) 2011 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/login/signed_settings.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 143 matching lines @@
   const bool add_to_whitelist_;
   SignedSettings::Delegate<bool>* d_;
   em::PolicyFetchResponse to_store_;
   scoped_refptr<SignedSettings> store_op_;
 };
 
 class StorePropertyOp : public SignedSettings,
                         public SignedSettings::Delegate<bool> {
  public:
   StorePropertyOp(const std::string& name,
-                  const std::string& value,
+                  const base::Value& value,
                   SignedSettings::Delegate<bool>* d);
   virtual ~StorePropertyOp();
   void Execute();
   void Fail(SignedSettings::ReturnCode code);
   void Succeed(bool value);
   // Implementation of OwnerManager::Delegate
   void OnKeyOpComplete(const OwnerManager::KeyOpCode return_code,
                        const std::vector<uint8>& payload);
   // Implementation of SignedSettings::Delegate
   void OnSettingsOpCompleted(ReturnCode code, bool value);
 
  private:
   void SetInPolicy(const std::string& prop,
-                   const std::string& value,
+                   const base::Value& value,
                    em::PolicyData* poldata);
   // Always call d_->OnSettingOpCompleted() via this call.
   // It guarantees that the callback will not be triggered until _after_
   // Execute() returns, which is implicitly assumed by SignedSettingsHelper
   // in some cases.
   void PerformCallback(SignedSettings::ReturnCode code, bool value);
 
   std::string name_;
-  std::string value_;
+  scoped_ptr<base::Value> value_;
   SignedSettings::Delegate<bool>* d_;
   em::PolicyFetchResponse to_store_;
   scoped_refptr<SignedSettings> store_op_;
 };
 
 class RetrievePropertyOp : public SignedSettings {
  public:
   RetrievePropertyOp(const std::string& name,
-                     SignedSettings::Delegate<std::string>* d);
+                     SignedSettings::Delegate<const base::Value&>* d);
   virtual ~RetrievePropertyOp();
   void Execute();
   void Fail(SignedSettings::ReturnCode code);
-  void Succeed(const std::string& value);
+  void Succeed(const base::Value& value);
   // Implementation of OwnerManager::Delegate::OnKeyOpComplete()
   void OnKeyOpComplete(const OwnerManager::KeyOpCode return_code,
                        const std::vector<uint8>& payload);
 
  private:
-  static const char* kVeritas[];
-
-  std::string LookUpInPolicy(const std::string& prop);
+  base::Value* LookUpInPolicy(const std::string& prop);
   // Always call d_->OnSettingOpCompleted() via this call.
   // It guarantees that the callback will not be triggered until _after_
   // Execute() returns, which is implicitly assumed by SignedSettingsHelper
   // in some cases.
   void PerformCallback(SignedSettings::ReturnCode code,
-                       const std::string& value);
+                       const base::Value& value);
 
   std::string name_;
-  std::string value_;
-  SignedSettings::Delegate<std::string>* d_;
+  scoped_ptr<base::Value> value_;
+  SignedSettings::Delegate<const base::Value&>* d_;
 };
 
 class StorePolicyOp : public SignedSettings {
  public:
   StorePolicyOp(em::PolicyFetchResponse* policy,
                 SignedSettings::Delegate<bool>* d);
   virtual ~StorePolicyOp();
   void Execute();
   void Fail(SignedSettings::ReturnCode code);
   void Succeed(bool value);
@@ skipping 59 matching lines @@
     SignedSettings::Delegate<bool>* d) {
   DCHECK(d != NULL);
   return new WhitelistOp(Authenticator::Canonicalize(email),
                          add_to_whitelist,
                          d);
 }
 
 // static
 SignedSettings* SignedSettings::CreateStorePropertyOp(
     const std::string& name,
-    const std::string& value,
+    const base::Value& value,
     SignedSettings::Delegate<bool>* d) {
   DCHECK(d != NULL);
   return new StorePropertyOp(name, value, d);
 }
 
 // static
 SignedSettings* SignedSettings::CreateRetrievePropertyOp(
     const std::string& name,
-    SignedSettings::Delegate<std::string>* d) {
+    SignedSettings::Delegate<const base::Value&>* d) {
   DCHECK(d != NULL);
   return new RetrievePropertyOp(name, d);
 }
 
 // static
 SignedSettings* SignedSettings::CreateStorePolicyOp(
     em::PolicyFetchResponse* policy,
     SignedSettings::Delegate<bool>* d) {
   DCHECK(d != NULL);
   DCHECK(policy != NULL);
@@ skipping 196 matching lines @@
     return;
   }
   LOG(WARNING) << "Whitelist modification no-op: " << email;
 }
 
 void WhitelistOp::PerformCallback(SignedSettings::ReturnCode code, bool value) {
   d_->OnSettingsOpCompleted(code, value);
 }
 
 StorePropertyOp::StorePropertyOp(const std::string& name,
-                                 const std::string& value,
+                                 const base::Value& value,
                                  SignedSettings::Delegate<bool>* d)
     : name_(name),
-      value_(value),
+      value_(value.DeepCopy()),
       d_(d),
       store_op_(NULL) {
 }
 
 StorePropertyOp::~StorePropertyOp() {}
 
 void StorePropertyOp::Execute() {
   if (service_->GetStatus(true) != OwnershipService::OWNERSHIP_TAKEN) {
     if (g_browser_process &&
         g_browser_process->local_state() &&
-        SignedSettingsTempStorage::Store(name_, value_,
+        SignedSettingsTempStorage::Store(name_, *value_,
                                          g_browser_process->local_state())) {
       Succeed(true);
       return;
     }
   }
   if (!service_->has_cached_policy()) {
     TryToFetchPolicyAndCallBack();
     return;
   }
   // Posts a task to the FILE thread to sign policy.
   em::PolicyData to_sign;
   to_sign.CheckTypeAndMergeFrom(service_->cached_policy());
-  SetInPolicy(name_, value_, &to_sign);
+  SetInPolicy(name_, *value_, &to_sign);
   to_store_.set_policy_data(to_sign.SerializeAsString());
   service_->StartSigningAttempt(to_store_.policy_data(), this);
 }
 
 void StorePropertyOp::Fail(SignedSettings::ReturnCode code) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&StorePropertyOp::PerformCallback, this, code, false));
 }
 
@@ skipping 30 matching lines @@
     em::PolicyData poldata;
     poldata.ParseFromString(to_store_.policy_data());
     service_->set_cached_policy(poldata);
     Succeed(value);
     return;
   }
   Fail(NOT_FOUND);
 }
 
 void StorePropertyOp::SetInPolicy(const std::string& prop,
-                                  const std::string& value,
+                                  const base::Value& value,
                                   em::PolicyData* poldata) {
   em::ChromeDeviceSettingsProto pol;
   pol.ParseFromString(poldata->policy_value());
   if (prop == kAccountsPrefAllowNewUser) {
     em::AllowNewUsersProto* allow = pol.mutable_allow_new_users();
-    allow->set_allow_new_users(value == "true");
-
+    bool allow_value;
+    if (value.GetAsBoolean(&allow_value))
+      allow->set_allow_new_users(allow_value);
+    else
+      NOTREACHED();
   } else if (prop == kAccountsPrefAllowGuest) {
     em::GuestModeEnabledProto* guest = pol.mutable_guest_mode_enabled();
-    guest->set_guest_mode_enabled(value == "true");
-
+    bool guest_value;
+    if (value.GetAsBoolean(&guest_value))
+      guest->set_guest_mode_enabled(guest_value);
+    else
+      NOTREACHED();
   } else if (prop == kAccountsPrefShowUserNamesOnSignIn) {
     em::ShowUserNamesOnSigninProto* show = pol.mutable_show_user_names();
-    show->set_show_user_names(value == "true");
-
+    bool show_value;
+    if (value.GetAsBoolean(&show_value))
+      show->set_show_user_names(show_value);
+    else
+      NOTREACHED();
   } else if (prop == kSignedDataRoamingEnabled) {
     em::DataRoamingEnabledProto* roam = pol.mutable_data_roaming_enabled();
-    roam->set_data_roaming_enabled(value == "true");
-
+    bool roaming_value;
+    if (value.GetAsBoolean(&roaming_value))
+      roam->set_data_roaming_enabled(roaming_value);
+    else
+      NOTREACHED();
   } else if (prop == kSettingProxyEverywhere) {
     // TODO(cmasone): NOTIMPLEMENTED() once http://crosbug.com/13052 is fixed.
-    bool success = pol.mutable_device_proxy_settings()->ParseFromString(value);
-    DCHECK(success);
-
+    std::string proxy_value;
+    if (value.GetAsString(&proxy_value)) {
+      bool success =
+          pol.mutable_device_proxy_settings()->ParseFromString(proxy_value);
+      DCHECK(success);
+    } else {
+      NOTREACHED();
+    }
   } else if (prop == kReleaseChannel) {
     em::ReleaseChannelProto* release_channel = pol.mutable_release_channel();
-    release_channel->set_release_channel(value);
-
+    std::string channel_value;
+    if (value.GetAsString(&channel_value))
+      release_channel->set_release_channel(channel_value);
+    else
+      NOTREACHED();
   } else if (prop == kStatsReportingPref) {
     em::MetricsEnabledProto* metrics = pol.mutable_metrics_enabled();
-    metrics->set_metrics_enabled(value == "true");
-
+    bool metrics_value;
+    if (value.GetAsBoolean(&metrics_value))
+      metrics->set_metrics_enabled(metrics_value);
+    else
+      NOTREACHED();
+  } else if (prop == kAccountsPrefUsers) {
+    em::UserWhitelistProto* whitelist_proto = pol.mutable_user_whitelist();
+    whitelist_proto->clear_user_whitelist();
+    const base::ListValue& users = static_cast<const base::ListValue&>(value);
+    for (base::ListValue::const_iterator i = users.begin();
+         i != users.end(); ++i) {
+      std::string email;
+      if ((*i)->GetAsString(&email))
+        whitelist_proto->add_user_whitelist(email.c_str());
+    }
   } else {
     NOTREACHED();
   }
   poldata->set_policy_value(pol.SerializeAsString());
 }
 
 void StorePropertyOp::PerformCallback(SignedSettings::ReturnCode code,
                                       bool value) {
   d_->OnSettingsOpCompleted(code, value);
 }
 
-// static
-const char* RetrievePropertyOp::kVeritas[] = { "false", "true" };
-
-RetrievePropertyOp::RetrievePropertyOp(const std::string& name,
-                                       SignedSettings::Delegate<std::string>* d)
+RetrievePropertyOp::RetrievePropertyOp(
+    const std::string& name,
+    SignedSettings::Delegate<const base::Value&>* d)
     : name_(name),
       d_(d) {
 }
 
 RetrievePropertyOp::~RetrievePropertyOp() {}
 
 void RetrievePropertyOp::Execute() {
   CHECK(chromeos::CrosLibrary::Get()->EnsureLoaded());
   // TODO(dilmah): Fix the race:
   // At the moment when device becomes owned there is lapse of time after
   // device has been owned and before temp_storage settings are finally
   // persisted into signed settings.
   // In this lapse of time Retrieve loses access to those settings.
   if (service_->GetStatus(true) != OwnershipService::OWNERSHIP_TAKEN) {
+    base::Value* temp_value;
     if (g_browser_process &&
         g_browser_process->local_state() &&
         SignedSettingsTempStorage::Retrieve(
-            name_, &value_, g_browser_process->local_state())) {
-      Succeed(value_);
+            name_, &temp_value, g_browser_process->local_state())) {
+      value_.reset(temp_value->DeepCopy());
+      Succeed(*value_);
       return;
     }
   }
 
   if (!service_->has_cached_policy()) {
     TryToFetchPolicyAndCallBack();
     return;
   }
-  std::string value = LookUpInPolicy(name_);
-  if (value.empty())
+  value_.reset(LookUpInPolicy(name_));
+  if (!value_.get())
     Fail(NOT_FOUND);
   else
-    Succeed(value);
+    Succeed(*value_);
 }
 
 void RetrievePropertyOp::Fail(SignedSettings::ReturnCode code) {
+  value_.reset(base::Value::CreateNullValue());
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&RetrievePropertyOp::PerformCallback, this, code,
-                 std::string()));
+      base::Bind(&RetrievePropertyOp::PerformCallback, this,
+                 code, base::ConstRef(*value_)));

[Mattias Nissler (ping if slow), 2011/10/13 14:49:52]

This looks dangerous in that you need to guarantee that RetrievePropertyOp
outlives the references you take here. That makes me lean towards switching back
to passing Values as pointers after all.

[pastarmovj, 2011/10/26 15:44:59]

Done. This made the mem management a little bit clumsier but this is
disappearing a few version later anyway completely.

 }
 
-void RetrievePropertyOp::Succeed(const std::string& value) {
+void RetrievePropertyOp::Succeed(const base::Value& value) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&RetrievePropertyOp::PerformCallback, this, SUCCESS, value));
+      base::Bind(&RetrievePropertyOp::PerformCallback, this,
+                 SUCCESS, base::ConstRef(value)));
 }
 
 // DEPRECATED.
 void RetrievePropertyOp::OnKeyOpComplete(
     const OwnerManager::KeyOpCode return_code,
     const std::vector<uint8>& sig) {
   NOTREACHED();
 }
 
-std::string RetrievePropertyOp::LookUpInPolicy(const std::string& prop) {
+base::Value* RetrievePropertyOp::LookUpInPolicy(const std::string& prop) {
   if (prop == kDeviceOwner) {
     const em::PolicyData& data = service_->cached_policy();
     if (data.has_username() && !data.has_request_token())
-      return data.username();
-    return "";
+      return base::Value::CreateStringValue(data.username());
   }
   VLOG(2) << "Looking up " << prop;
   em::ChromeDeviceSettingsProto pol;
   pol.ParseFromString(service_->cached_policy().policy_value());
   if (prop == kAccountsPrefAllowNewUser) {
     if (pol.has_allow_new_users() &&
         pol.allow_new_users().has_allow_new_users() &&
         pol.allow_new_users().allow_new_users()) {
-      return kVeritas[1];  // New users allowed, user_whitelist() ignored.
+      // New users allowed, user_whitelist() ignored.
+      return base::Value::CreateBooleanValue(true);
     }
     // If we have the allow_new_users bool, and it is true, we honor that above.
     // In all other cases (don't have it, have it and it is set to false, etc),
     // We will honor the user_whitelist() if it is there and populated.
-    // Otherwise, fail open (to do otherwise could render the device unusable).
+    // Otherwise we default to allowing new users.
     if (!pol.has_user_whitelist())
-      return kVeritas[1];  // Default to allowing new users.
-    return kVeritas[pol.user_whitelist().user_whitelist_size() == 0];
+      return base::Value::CreateBooleanValue(true);
+    return base::Value::CreateBooleanValue(
+               pol.user_whitelist().user_whitelist_size() == 0);
 
   } else if (prop == kAccountsPrefAllowGuest) {
     if (!pol.has_guest_mode_enabled() ||
         !pol.guest_mode_enabled().has_guest_mode_enabled()) {
-      return kVeritas[1];  // Default to allowing guests;
+      // Default to allowing guests;
+      return base::Value::CreateBooleanValue(true);
     }
-    return kVeritas[pol.guest_mode_enabled().guest_mode_enabled()];
+    return base::Value::CreateBooleanValue(
+               pol.guest_mode_enabled().guest_mode_enabled());
 
   } else if (prop == kAccountsPrefShowUserNamesOnSignIn) {
     if (!pol.has_show_user_names() ||
         !pol.show_user_names().has_show_user_names()) {
-      return kVeritas[1];  // Default to showing pods on the login screen;
+      // Default to showing pods on the login screen;
+      return base::Value::CreateBooleanValue(true);
     }
-    return kVeritas[pol.show_user_names().show_user_names()];
+    return base::Value::CreateBooleanValue(
+               pol.show_user_names().show_user_names());
 
   } else if (prop == kSignedDataRoamingEnabled) {
     if (!pol.has_data_roaming_enabled() ||
         !pol.data_roaming_enabled().has_data_roaming_enabled()) {
-      return kVeritas[0];  // Default to disabling cellular data roaming;
+      // Default to disabling cellular data roaming;
+      return base::Value::CreateBooleanValue(false);
     }
-    return kVeritas[pol.data_roaming_enabled().data_roaming_enabled()];
+    return base::Value::CreateBooleanValue(
+               pol.data_roaming_enabled().data_roaming_enabled());
 
   } else if (prop == kSettingProxyEverywhere) {
     // TODO(cmasone): NOTIMPLEMENTED() once http://crosbug.com/13052 is fixed.
     std::string serialized;
-    if (!pol.has_device_proxy_settings() ||
-        !pol.device_proxy_settings().SerializeToString(&serialized)) {
-      return "";           // Default to invalid proxy config (will be ignored).
+    if (pol.has_device_proxy_settings() &&
+        pol.device_proxy_settings().SerializeToString(&serialized)) {
+      return base::Value::CreateStringValue(serialized);
     }
-    return serialized;
 
   } else if (prop == kReleaseChannel) {
     if (!pol.has_release_channel() ||
         !pol.release_channel().has_release_channel()) {
-      return "";           // Default to an invalid channel (will be ignored).
+      // Default to an invalid channel (will be ignored).
+      return base::Value::CreateStringValue("");
     }
-    return pol.release_channel().release_channel();
+    return base::Value::CreateStringValue(
+               pol.release_channel().release_channel());
 
   } else if (prop == kStatsReportingPref) {
     if (pol.has_metrics_enabled()) {
-      return kVeritas[pol.metrics_enabled().metrics_enabled()];
+      return base::Value::CreateBooleanValue(
+                 pol.metrics_enabled().metrics_enabled());
     }
+  } else if (prop == kAccountsPrefUsers) {
+    base::ListValue* list = new base::ListValue();
+    const em::UserWhitelistProto& whitelist_proto = pol.user_whitelist();
+    const RepeatedPtrField<string>& whitelist =
+        whitelist_proto.user_whitelist();
+    for (RepeatedPtrField<string>::const_iterator it = whitelist.begin();
+         it != whitelist.end(); ++it) {
+      list->Append(base::Value::CreateStringValue(*it));
+    }
+    return list;
   }
-  return std::string();
+  return NULL;
 }
 
 void RetrievePropertyOp::PerformCallback(SignedSettings::ReturnCode code,
-                                         const std::string& value) {
+                                         const base::Value& value) {
   d_->OnSettingsOpCompleted(code, value);
 }
 
 StorePolicyOp::StorePolicyOp(em::PolicyFetchResponse* policy,
                              SignedSettings::Delegate<bool>* d)
     : policy_(policy),
       d_(d) {
 }
 
 StorePolicyOp::~StorePolicyOp() {}
@@ skipping 157 matching lines @@
   sig.assign(sig_ptr, sig_ptr + policy_.policy_data_signature().length());
   service_->StartVerifyAttempt(policy_.policy_data(), sig, this);
 }
 
 void RetrievePolicyOp::PerformCallback(SignedSettings::ReturnCode code,
                                        const em::PolicyFetchResponse& value) {
   d_->OnSettingsOpCompleted(code, value);
 }
 
 }  // namespace chromeos