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

chrome/browser/chromeos/login/signed_settings_unittest.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 "base/file_util.h"
 #include "base/logging.h"
 #include "base/scoped_temp_dir.h"
 #include "base/stringprintf.h"
@@ skipping 59 matching lines @@
   virtual void compare_expected(T to_compare) = 0;
 };
 
 template <class T>
 class NormalDelegate : public DummyDelegate<T> {
  public:
   explicit NormalDelegate(T to_expect) : DummyDelegate<T>(to_expect) {}
   virtual ~NormalDelegate() {}
  protected:
   virtual void compare_expected(T to_compare) {
-    EXPECT_EQ(this->expected_, to_compare);  // without this-> this won't build.
+    // without this-> this won't build.
+    EXPECT_EQ(this->expected_, to_compare);
   }
 };
 
+// Speicalize the template for base::Value obects because these compare

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

typos!

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

Done.

+// differently.
+template <>
+class NormalDelegate<const base::Value&>
+    : public DummyDelegate<const base::Value&> {
+ public:
+  explicit NormalDelegate(const base::Value& to_expect)
+      : DummyDelegate<const base::Value&>(to_expect) {}
+  virtual ~NormalDelegate() {}
+ protected:
+  virtual void compare_expected(const base::Value& to_compare) {
+    // without this-> this won't build.
+    EXPECT_TRUE(this->expected_.Equals(&to_compare));
+  }
+};
+
 class ProtoDelegate : public DummyDelegate<const em::PolicyFetchResponse&> {
  public:
   explicit ProtoDelegate(const em::PolicyFetchResponse& e)
       : DummyDelegate<const em::PolicyFetchResponse&>(e) {
   }
   virtual ~ProtoDelegate() {}
  protected:
   virtual void compare_expected(const em::PolicyFetchResponse& to_compare) {
     std::string ex_string, comp_string;
     EXPECT_TRUE(expected_.SerializeToString(&ex_string));
     EXPECT_TRUE(to_compare.SerializeToString(&comp_string));
     EXPECT_EQ(ex_string, comp_string);
   }
 };
 
 }  // anonymous namespace
 
 class SignedSettingsTest : public testing::Test {
  public:
   SignedSettingsTest()
       : fake_email_("fakey@example.com"),
         fake_domain_("*@example.com"),
         fake_prop_(kAccountsPrefAllowGuest),
+        fake_signature_("false"),
         fake_value_("false"),
         message_loop_(MessageLoop::TYPE_UI),
         ui_thread_(BrowserThread::UI, &message_loop_),
         file_thread_(BrowserThread::FILE),
         mock_(new MockKeyUtils),
         injector_(mock_) /* injector_ takes ownership of mock_ */ {
   }
 
   virtual ~SignedSettingsTest() {}
 
@@ skipping 149 matching lines @@
     em::PolicyFetchResponse fake_policy;
     if (!data.empty())
       fake_policy.set_policy_data(data);
     if (!sig.empty())
       fake_policy.set_policy_data_signature(sig);
     EXPECT_TRUE(fake_policy.SerializeToString(out_serialized));
     return fake_policy;
   }
 
   void DoRetrieveProperty(const std::string& name,
-                          const std::string& value,
+                          const base::Value& value,
                           em::PolicyData* fake_pol) {
-    NormalDelegate<std::string> d(value);
+    NormalDelegate<const base::Value&> d(value);
     d.expect_success();
     scoped_refptr<SignedSettings> s(
         SignedSettings::CreateRetrievePropertyOp(name, &d));
     mock_service(s.get(), &m_);
     EXPECT_CALL(m_, GetStatus(_))
         .WillOnce(Return(OwnershipService::OWNERSHIP_TAKEN));
     EXPECT_CALL(m_, has_cached_policy())
         .WillOnce(Return(true));
 
     EXPECT_CALL(m_, cached_policy())
         .WillOnce(ReturnRef(*fake_pol));
 
     s->Execute();
     message_loop_.RunAllPending();
   }
 
   const std::string fake_email_;
   const std::string fake_domain_;
   const std::string fake_prop_;
-  const std::string fake_value_;
+  const std::string fake_signature_;
+  const base::StringValue fake_value_;
   MockOwnershipService m_;
 
   ScopedTempDir tmpdir_;
   FilePath tmpfile_;
 
   MessageLoop message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
 
   std::vector<uint8> fake_public_key_;
@@ skipping 169 matching lines @@
 }
 
 TEST_F(SignedSettingsTest, StorePropertyNoKey) {
   FailingStorePropertyOp(OwnerManager::KEY_UNAVAILABLE);
 }
 
 TEST_F(SignedSettingsTest, StorePropertyFailed) {
   FailingStorePropertyOp(OwnerManager::OPERATION_FAILED);
 }
 
 TEST_F(SignedSettingsTest, RetrieveProperty) {

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

Could you put in a new test to validate the whitelist getting/setting you added?

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

I'd rather not do in this CL. A latter step of this CLs remove all those tests
that use the RetrievePropertyOp because we remove it too. However I will move
those tests in a new CrosSettings test suite with the CL that removes the
RetrievePropertyOp from the picture.

   em::PolicyData fake_pol = BuildPolicyData(std::vector<std::string>());
-  DoRetrieveProperty(fake_prop_, fake_value_, &fake_pol);
+  base::FundamentalValue fake_value(false);
+  DoRetrieveProperty(fake_prop_, fake_value, &fake_pol);
 }
 
 TEST_F(SignedSettingsTest, RetrieveOwnerProperty) {
   em::PolicyData fake_pol = BuildPolicyData(std::vector<std::string>());
   fake_pol.set_username(fake_email_);
-  DoRetrieveProperty(kDeviceOwner, fake_email_, &fake_pol);
+  base::StringValue fake_value(fake_email_);
+  DoRetrieveProperty(kDeviceOwner, fake_value, &fake_pol);
 }
 
 TEST_F(SignedSettingsTest, ExplicitlyAllowNewUsers) {
   em::PolicyData fake_pol = BuildPolicyData(std::vector<std::string>());
   SetAllowNewUsers(true, &fake_pol);
-  DoRetrieveProperty(kAccountsPrefAllowNewUser, "true", &fake_pol);
+  base::FundamentalValue fake_value(true);
+  DoRetrieveProperty(kAccountsPrefAllowNewUser, fake_value, &fake_pol);
 }
 
 TEST_F(SignedSettingsTest, ExplicitlyDisallowNewUsers) {
   std::vector<std::string> whitelist(1, fake_email_ + "m");
   em::PolicyData fake_pol = BuildPolicyData(whitelist);
   SetAllowNewUsers(false, &fake_pol);
-  DoRetrieveProperty(kAccountsPrefAllowNewUser, "false", &fake_pol);
+  base::FundamentalValue fake_value(false);
+  DoRetrieveProperty(kAccountsPrefAllowNewUser, fake_value, &fake_pol);
 }
 
 TEST_F(SignedSettingsTest, ImplicitlyDisallowNewUsers) {
   std::vector<std::string> whitelist(1, fake_email_ + "m");
   em::PolicyData fake_pol = BuildPolicyData(whitelist);
-  DoRetrieveProperty(kAccountsPrefAllowNewUser, "false", &fake_pol);
+  base::FundamentalValue fake_value(false);
+  DoRetrieveProperty(kAccountsPrefAllowNewUser, fake_value, &fake_pol);
 }
 
 TEST_F(SignedSettingsTest, AccidentallyDisallowNewUsers) {
   em::PolicyData fake_pol = BuildPolicyData(std::vector<std::string>());
   SetAllowNewUsers(false, &fake_pol);
-  DoRetrieveProperty(kAccountsPrefAllowNewUser, "true", &fake_pol);
+  base::FundamentalValue fake_value(true);
+  DoRetrieveProperty(kAccountsPrefAllowNewUser, fake_value, &fake_pol);
 }
 
 TEST_F(SignedSettingsTest, RetrievePropertyNotFound) {
-  NormalDelegate<std::string> d(fake_value_);
+  NormalDelegate<const base::Value&> d(fake_value_);
   d.expect_failure(SignedSettings::NOT_FOUND);
   scoped_refptr<SignedSettings> s(
       SignedSettings::CreateRetrievePropertyOp("unknown_prop", &d));
   mock_service(s.get(), &m_);
   EXPECT_CALL(m_, GetStatus(_))
       .WillOnce(Return(OwnershipService::OWNERSHIP_TAKEN));
   EXPECT_CALL(m_, has_cached_policy())
       .WillOnce(Return(true));
 
   em::PolicyData fake_pol = BuildPolicyData(std::vector<std::string>());
   EXPECT_CALL(m_, cached_policy())
       .WillOnce(ReturnRef(fake_pol));
 
   s->Execute();
   message_loop_.RunAllPending();
 }
 
 ACTION_P(Retrieve, s) { (*arg0)((void*)arg1, s.c_str(), s.length()); }
 ACTION_P(FinishKeyOp, s) { arg2->OnKeyOpComplete(OwnerManager::SUCCESS, s); }
 
 TEST_F(SignedSettingsTest, RetrievePolicyToRetrieveProperty) {
-  NormalDelegate<std::string> d(fake_value_);
+  base::FundamentalValue fake_value(false);
+  NormalDelegate<const base::Value&> d(fake_value);
   d.expect_success();
   scoped_refptr<SignedSettings> s(
       SignedSettings::CreateRetrievePropertyOp(fake_prop_, &d));
 
   em::PolicyData fake_pol = BuildPolicyData(std::vector<std::string>());
   std::string data = fake_pol.SerializeAsString();
   std::string signed_serialized;
   em::PolicyFetchResponse signed_policy = BuildProto(data,
-                                                     fake_value_,
+                                                     fake_signature_,
                                                      &signed_serialized);
   MockLoginLibrary* lib = MockLoginLib();
   EXPECT_CALL(*lib, RequestRetrievePolicy(_, _))
       .WillOnce(Retrieve(signed_serialized))
       .RetiresOnSaturation();
 
   mock_service(s.get(), &m_);
 
   EXPECT_CALL(m_, GetStatus(_))
       .WillOnce(Return(OwnershipService::OWNERSHIP_TAKEN))
       .WillOnce(Return(OwnershipService::OWNERSHIP_TAKEN));
   EXPECT_CALL(m_, has_cached_policy())
       .WillOnce(Return(false))
       .WillOnce(Return(true));
   em::PolicyData out_pol;
   EXPECT_CALL(m_, set_cached_policy(A<const em::PolicyData&>()))
       .WillOnce(SaveArg<0>(&out_pol));
   EXPECT_CALL(m_, cached_policy())
       .WillOnce(ReturnRef(out_pol));
 
-  std::vector<uint8> fake_sig(fake_value_.c_str(),
-                              fake_value_.c_str() + fake_value_.length());
+  std::string string_fake_value;
+  fake_value_.GetAsString(&string_fake_value);
+  std::vector<uint8> fake_sig(
+      string_fake_value.c_str(),
+      string_fake_value.c_str() + string_fake_value.length());
   EXPECT_CALL(m_, StartVerifyAttempt(data, fake_sig, _))
       .WillOnce(FinishKeyOp(fake_sig))
       .RetiresOnSaturation();
 
   s->Execute();
   message_loop_.RunAllPending();
   UnMockLoginLib();
 }
 
 TEST_F(SignedSettingsTest, SignAndStorePolicy) {
@@ skipping 16 matching lines @@
   EXPECT_CALL(m_, set_cached_policy(A<const em::PolicyData&>()))
       .WillOnce(SaveArg<0>(&out_pol));
 
   // Ask for signature over unsigned policy.
   s->Execute();
   message_loop_.RunAllPending();
 
   // Fake out a successful signing.
   std::string signed_serialized;
   em::PolicyFetchResponse signed_policy = BuildProto(data_serialized,
-                                                     fake_value_,
+                                                     fake_signature_,
                                                      &signed_serialized);
-  std::vector<uint8> fake_sig(fake_value_.c_str(),
-                              fake_value_.c_str() + fake_value_.length());
+  std::string string_fake_value;
+  fake_value_.GetAsString(&string_fake_value);
+  std::vector<uint8> fake_sig(
+      string_fake_value.c_str(),
+      string_fake_value.c_str() + string_fake_value.length());

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

seems like a helper for this would be nice now that it's longer and occurring
already 4 times.

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

Done.

 
   MockLoginLibrary* lib = MockLoginLib();
   EXPECT_CALL(*lib, RequestStorePolicy(StrEq(signed_serialized), _, s.get()))
       .WillOnce(InvokeArgument<1>(static_cast<void*>(s.get()), true))
       .RetiresOnSaturation();
   s->OnKeyOpComplete(OwnerManager::SUCCESS, fake_sig);
   message_loop_.RunAllPending();
   UnMockLoginLib();
 }
 
 TEST_F(SignedSettingsTest, StoreSignedPolicy) {
   NormalDelegate<bool> d(true);
   d.expect_success();
 
   em::PolicyData in_pol = BuildPolicyData(std::vector<std::string>());
   std::string serialized = in_pol.SerializeAsString();
   std::string signed_serialized;
   em::PolicyFetchResponse signed_policy = BuildProto(serialized,
-                                                     fake_value_,
+                                                     fake_signature_,
                                                      &signed_serialized);
   scoped_refptr<SignedSettings> s(
       SignedSettings::CreateStorePolicyOp(&signed_policy, &d));
   MockLoginLibrary* lib = MockLoginLib();
   EXPECT_CALL(*lib, RequestStorePolicy(StrEq(signed_serialized), _, s.get()))
       .WillOnce(InvokeArgument<1>(static_cast<void*>(s.get()), true))
       .RetiresOnSaturation();
 
   mock_service(s.get(), &m_);
   em::PolicyData out_pol;
@@ skipping 26 matching lines @@
 
   s->Execute();
   message_loop_.RunAllPending();
 }
 
 TEST_F(SignedSettingsTest, RetrievePolicy) {
   em::PolicyData in_pol = BuildPolicyData(std::vector<std::string>());
   std::string serialized = in_pol.SerializeAsString();
   std::string signed_serialized;
   em::PolicyFetchResponse signed_policy = BuildProto(serialized,
-                                                     fake_value_,
+                                                     fake_signature_,
                                                      &signed_serialized);
   ProtoDelegate d(signed_policy);
   d.expect_success();
   scoped_refptr<SignedSettings> s(SignedSettings::CreateRetrievePolicyOp(&d));
 
   MockLoginLibrary* lib = MockLoginLib();
   EXPECT_CALL(*lib, RequestRetrievePolicy(_, s.get()))
       .WillOnce(InvokeArgument<0>(static_cast<void*>(s.get()),
                                   signed_serialized.c_str(),
                                   signed_serialized.length()))
       .RetiresOnSaturation();
 
   mock_service(s.get(), &m_);
-  std::vector<uint8> fake_sig(fake_value_.c_str(),
-                              fake_value_.c_str() + fake_value_.length());
+  std::string string_fake_value;
+  fake_value_.GetAsString(&string_fake_value);
+  std::vector<uint8> fake_sig(
+      string_fake_value.c_str(),
+      string_fake_value.c_str() + string_fake_value.length());
   EXPECT_CALL(m_, StartVerifyAttempt(serialized, fake_sig, _))
       .Times(1);
   em::PolicyData out_pol;
   EXPECT_CALL(m_, set_cached_policy(A<const em::PolicyData&>()))
       .WillOnce(SaveArg<0>(&out_pol));
 
   s->Execute();
   message_loop_.RunAllPending();
   UnMockLoginLib();
 
@@ skipping 53 matching lines @@
       .RetiresOnSaturation();
 
   s->Execute();
   message_loop_.RunAllPending();
   UnMockLoginLib();
 }
 
 TEST_F(SignedSettingsTest, RetrieveMalsignedPolicy) {
   std::string signed_serialized;
   em::PolicyFetchResponse signed_policy = BuildProto(fake_prop_,
-                                                     fake_value_,
+                                                     fake_signature_,
                                                      &signed_serialized);
   ProtoDelegate d(signed_policy);
   d.expect_failure(SignedSettings::BAD_SIGNATURE);
   scoped_refptr<SignedSettings> s(SignedSettings::CreateRetrievePolicyOp(&d));
 
   MockLoginLibrary* lib = MockLoginLib();
   EXPECT_CALL(*lib, RequestRetrievePolicy(_, s.get()))
       .WillOnce(InvokeArgument<0>(static_cast<void*>(s.get()),
                                   signed_serialized.c_str(),
                                   signed_serialized.length()))
       .RetiresOnSaturation();
 
   mock_service(s.get(), &m_);
-  std::vector<uint8> fake_sig(fake_value_.c_str(),
-                              fake_value_.c_str() + fake_value_.length());
+  std::string string_fake_value;
+  fake_value_.GetAsString(&string_fake_value);
+  std::vector<uint8> fake_sig(
+      string_fake_value.c_str(),
+      string_fake_value.c_str() + string_fake_value.length());
   EXPECT_CALL(m_, StartVerifyAttempt(fake_prop_, fake_sig, _))
       .Times(1);
 
   s->Execute();
   message_loop_.RunAllPending();
   UnMockLoginLib();
 
   s->OnKeyOpComplete(OwnerManager::OPERATION_FAILED, std::vector<uint8>());
   message_loop_.RunAllPending();
 }
 
 }  // namespace chromeos