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"
 #include "base/memory/ref_counted.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/cros/login_library.h"
 #include "chrome/browser/chromeos/cros_settings_names.h"
 #include "chrome/browser/chromeos/login/authenticator.h"
 #include "chrome/browser/chromeos/login/ownership_service.h"
 #include "chrome/browser/chromeos/login/signed_settings_temp_storage.h"
@@ skipping 133 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[];

[Mattias Nissler (ping if slow), 2011/10/07 11:02:57]

This should be unused, so remove it.

[pastarmovj, 2011/10/13 11:25:06]

Done.

 
-  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 200 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,
       NewRunnableMethod(this, &StorePropertyOp::PerformCallback, code, false));
 }
 
@@ skipping 33 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);

[Mattias Nissler (ping if slow), 2011/10/07 11:02:57]

else NOTREACHED() (here and below?)

Actually, it might be quite helpful to write a small helpers for the common
boolean and string cases.

[pastarmovj, 2011/10/13 11:25:06]

Given the fact that we can only abstract the extraction step  it will bake less
sense to extract helpers. But NOTREACHED has been done.

   } 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 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 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 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 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 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 if (prop == kAccountsPrefUsers) {

[Chris Masone, 2011/10/06 16:13:06]

so, this essentially means that WhitelistOp is obsolete, right?  Can we get a
comment to that effect on the WhitelistOp class declaration?

[pastarmovj, 2011/10/13 11:25:06]

I don't think this is much needed. The code still uses the Whitelist ops as of
this CL and the next CLs actually remove the WhitelistOp altogether.

+    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,
-      NewRunnableMethod(this,
-                        &RetrievePropertyOp::PerformCallback,
-                        code, std::string()));
+      base::Bind(&RetrievePropertyOp::PerformCallback, this,
+                 code, base::ConstRef(*value_)));
 }
 
-void RetrievePropertyOp::Succeed(const std::string& value) {
+void RetrievePropertyOp::Succeed(const base::Value& value) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(this,
-                        &RetrievePropertyOp::PerformCallback, 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(

[Mattias Nissler (ping if slow), 2011/10/07 11:02:57]

don't need outer parentheses.

[pastarmovj, 2011/10/13 11:25:06]

Done.

+                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 161 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