Move user cloud policy to BrowserProcess (was 6979011)

chrome/browser/policy/cloud_policy_provider_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/policy/cloud_policy_provider.h"
+
+#include "base/values.h"
+#include "chrome/browser/policy/cloud_policy_cache_base.h"
+#include "chrome/browser/policy/cloud_policy_provider_impl.h"
+#include "chrome/browser/policy/configuration_policy_pref_store.h"
+#include "chrome/browser/policy/mock_configuration_policy_store.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using testing::AnyNumber;
+using testing::_;
+
+namespace policy {
+
+class MockCloudPolicyCache : public CloudPolicyCacheBase {
+ public:
+  MockCloudPolicyCache() {}
+  virtual ~MockCloudPolicyCache() {}
+
+  // CloudPolicyCacheBase Implementation.

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

lower-case: implementation

[gfeher, 2011/06/22 19:18:34]

Done.

+  void Load() {}
+  void SetPolicy(const em::PolicyFetchResponse& policy) {}
+  bool DecodePolicyData(const em::PolicyData& policy_data,
+                        PolicyMap* mandatory,
+                        PolicyMap* recommended) {
+    return true;
+  }
+
+  // Non-const accessors for underlying PolicyMaps.
+  PolicyMap* raw_mandatory_policy() {
+    return &mandatory_policy_;
+  }
+
+  PolicyMap* raw_recommended_policy() {
+    return &recommended_policy_;
+  }
+
+  void SetUnmanaged() {
+    is_unmanaged_ = true;
+  }
+
+  void set_initialized(bool initialized) {
+    initialization_complete_ = initialized;
+  }
+

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

private:

[gfeher, 2011/06/22 19:18:34]

Done.

+  DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyCache);
+};
+
+class CloudPolicyProviderTest : public testing::Test {
+ protected:
+  void CreateCloudPolicyProvider(
+      const ConfigurationPolicyProvider::PolicyDefinitionList* policy_list,
+      CloudPolicyCacheBase::PolicyLevel level) {
+    cloud_policy_provider_.reset(
+        new CloudPolicyProviderImpl(policy_list, level));
+  }
+
+  void CreateCloudPolicyProvider(
+      CloudPolicyCacheBase::PolicyLevel level) {
+    CreateCloudPolicyProvider(
+        policy::ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList(),
+        level);
+  }
+
+  void Provide(ConfigurationPolicyStoreInterface* store) {
+    DCHECK(cloud_policy_provider_.get());
+    cloud_policy_provider_->Provide(store);
+  }
+
+  void AppendCache(CloudPolicyCacheBase* cache) {
+    DCHECK(cloud_policy_provider_.get());
+    cloud_policy_provider_->AppendCache(cache);
+  }
+
+  void PrependCache(CloudPolicyCacheBase* cache) {
+    DCHECK(cloud_policy_provider_.get());
+    cloud_policy_provider_->PrependCache(cache);
+  }
+
+  void CombineTwoPolicyMaps(const PolicyMap& base,
+                            const PolicyMap& overlay,
+                            PolicyMap* out_map) {
+    DCHECK(cloud_policy_provider_.get());
+    cloud_policy_provider_->CombineTwoPolicyMaps(base, overlay, out_map);
+  }
+
+  ConfigurationPolicyProvider::PolicyDefinitionList* simple_list() {
+    return simple_list_.get();
+  }
+
+  ConfigurationPolicyType simple_list_policy_type(int id) {
+    DCHECK(id < simple_list_length_);
+    return simple_list_->begin[id].policy_type;
+  }
+
+  int simple_list_length() {
+    return simple_list_length_;
+  }
+
+  int proxy_policy_count() {
+    return CloudPolicyProviderImpl::proxy_policy_count();
+  }
+
+  bool is_proxy_policy(ConfigurationPolicyType policy) {
+    return CloudPolicyProviderImpl::is_proxy_policy(policy);
+  }
+
+  ConfigurationPolicyType get_proxy_policy(int i) {
+    DCHECK(i < proxy_policy_count());
+    return CloudPolicyProviderImpl::proxy_policies[i];
+  }
+
+  virtual void SetUp() {
+    // Extract all non-proxy policies from the default chrome policy definition
+    // list but set all the types to integer. The names of policies are not
+    // modified, and they are not used later in these tests.
+    const ConfigurationPolicyProvider::PolicyDefinitionList* real_list =
+        ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList();
+    const ConfigurationPolicyProvider::PolicyDefinitionList::Entry* current;
+
+    // Count the number of non-proxy policies.
+    simple_list_length_ = 0;
+    for (current = real_list->begin; current != real_list->end; ++current) {
+      if (!CloudPolicyProviderImpl::is_proxy_policy(current->policy_type)) {
+        ++simple_list_length_;
+      }
+    }
+
+    int counter = 0;
+    simple_list_.reset(new ConfigurationPolicyProvider::PolicyDefinitionList);
+    entries_.reset(
+        new ConfigurationPolicyProvider::PolicyDefinitionList::Entry[
+            simple_list_length_]);
+    for (current = real_list->begin; current != real_list->end; ++current) {
+      if (CloudPolicyProviderImpl::is_proxy_policy(current->policy_type))
+        continue;
+
+      entries_[counter].policy_type = current->policy_type;
+      entries_[counter].value_type = Value::TYPE_INTEGER;
+      entries_[counter].name =  current->name;
+
+      ++counter;
+    }
+
+    simple_list_->begin = entries_.get();
+    simple_list_->end = entries_.get() + simple_list_length_;
+  }
+
+  virtual void TearDown() {
+    simple_list_.reset();
+    entries_.reset();
+  }
+
+  // Verify the test scenario of MultipleCaches

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

period.

[gfeher, 2011/06/22 19:18:34]

Done.

+  void VerifyMultipleCaches(MockCloudPolicyCache caches[],

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

I don't like this verification code. It's hard to understand, so it's hard to
make sure it actually tests the right thing. I think we should rather have
simple-to-understand specific tests. In particular, can't we just fill a few
caches with test data and check the specific result?

[gfeher, 2011/06/22 19:18:34]

Done.

+                            CloudPolicyCacheBase::PolicyLevel level) {
+    CreateCloudPolicyProvider(simple_list(), level);
+    MockConfigurationPolicyStore store;
+    for (int i = 0; i < simple_list_length(); ++i)
+      AppendCache(&caches[i]);
+
+    EXPECT_CALL(store, Apply(_, _)).Times(AnyNumber());
+    Provide(&store);
+
+    const int n = simple_list_length();
+    // The final compilation should have exactly two policies per active cache
+    // set.
+    for (int i = 0; i < n; ++i) {
+      const Value* value = store.Get(simple_list_policy_type(i));
+      if (i == n - 1 && !caches[i].initialization_complete()) {
+        // If the last provider is not initialized, then the last policy
+        // is not provided by anyone.
+        EXPECT_EQ(NULL, value);
+        break;
+      }
+      ASSERT_TRUE(value != NULL);
+      int policy_value;
+      EXPECT_TRUE(value->GetAsInteger(&policy_value));
+      int originating_cache = policy_value / n;
+      int policy_id = policy_value % n;
+      EXPECT_EQ(i, policy_id) << "Policy identifier mismatch";
+      if (caches[i].initialization_complete()) {
+        // Initialized caches will have their corresponding policy set.
+        EXPECT_EQ(i, originating_cache) << "Providing cache mismatch.";
+      } else {
+        // For non-initialized providers the corresponding value will be set by
+        // the next provider. (The last provider is handled elsewhere.)
+        EXPECT_EQ(i + 1, originating_cache) << "Providing cache mismatch.";
+      }
+    }
+  }
+
+ private:
+  scoped_ptr<CloudPolicyProviderImpl> cloud_policy_provider_;
+
+  // A list of policies to be used in tests: it has some simplifications to
+  // make testing more easy: it doesn't have proxy policies and all the
+  // policies have the type of integer.
+  scoped_ptr<ConfigurationPolicyProvider::PolicyDefinitionList> simple_list_;

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

Why is this so complicated? Can't we just declare a suitable static list similar
to how it's done in configuration_policy_pref_store.cc?

[gfeher, 2011/06/22 19:18:34]

We can go even deeper...

+  int simple_list_length_;
+
+  // The following members own the internal parts of simple_list_, therefore
+  // they may only be freed after simple_list_ is freed.
+  scoped_array<ConfigurationPolicyProvider::PolicyDefinitionList::Entry>
+     entries_;
+};
+
+// Proxy setting distributed over multiple caches.
+TEST_F(CloudPolicyProviderTest,
+       ProxySettingDistributedOverMultipleCaches) {
+  const int proxy_related_policies = proxy_policy_count();
+
+  // There are |proxy_related_policies|+1 caches and they are mixed together by
+  // one instance of CloudPolicyProvider. The first cache has some policies but
+  // no proxy-related ones. The following caches have each one proxy-policy set.
+  const int n = proxy_related_policies + 1;
+
+  // Make sure that our assumptions about the existing proxy-related policies
+  // are still correct.
+  DCHECK(proxy_related_policies == 5);
+
+  scoped_array<MockCloudPolicyCache> caches(new MockCloudPolicyCache[n]);

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

Just put 5 instead of n and delete all the cruft above :)

[gfeher, 2011/06/22 19:18:34]

Done.

+
+  const ConfigurationPolicyProvider::PolicyDefinitionList* list =
+      policy::ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList();
+  const ConfigurationPolicyProvider::PolicyDefinitionList::Entry* current;
+
+  // Prepare |cache[0]| to serve some non-proxy policies.
+  caches[0].set_initialized(true);
+  caches[0].raw_mandatory_policy()->Set(kPolicyShowHomeButton,
+                                        Value::CreateBooleanValue(true));
+  caches[0].raw_mandatory_policy()->Set(kPolicyIncognitoEnabled,
+                                        Value::CreateBooleanValue(true));
+  caches[0].raw_mandatory_policy()->Set(kPolicyTranslateEnabled,
+                                        Value::CreateBooleanValue(true));
+
+  // Prepare the other caches to serve one proxy-policy each.
+  caches[1].set_initialized(true);
+  caches[1].raw_mandatory_policy()->Set(kPolicyProxyMode,
+                                        Value::CreateStringValue("cache 1"));
+  caches[2].set_initialized(true);
+  caches[2].raw_mandatory_policy()->Set(kPolicyProxyServerMode,
+                                        Value::CreateIntegerValue(2));
+  caches[3].set_initialized(true);
+  caches[3].raw_mandatory_policy()->Set(kPolicyProxyServer,
+                                        Value::CreateStringValue("cache 3"));
+  caches[4].set_initialized(true);
+  caches[4].raw_mandatory_policy()->Set(kPolicyProxyPacUrl,
+                                        Value::CreateStringValue("cache 4"));
+  caches[5].set_initialized(true);
+  caches[5].raw_mandatory_policy()->Set(kPolicyProxyMode,
+                                        Value::CreateStringValue("cache 5"));
+
+  CreateCloudPolicyProvider(CloudPolicyCacheBase::POLICY_LEVEL_MANDATORY);
+
+  // Prepend from the end.
+  for (int i = 0; i < n; ++i)
+    PrependCache(&caches[n-i-1]);
+
+  MockConfigurationPolicyStore store;
+  EXPECT_CALL(store, Apply(_, _)).Times(AnyNumber());
+  Provide(&store);
+
+  // The final compilation should have some policies set to true. From the proxy
+  // related policies only |kPolicyProxyMode| should be set.
+  for (current = list->begin; current != list->end; ++current) {

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

let's just write down what exact expectation we have instead of looping and
conditionals, which is not exactly simple to understand.

[gfeher, 2011/06/22 19:18:34]

Done.

+    const Value* value = store.Get(current->policy_type);
+    if (current->policy_type == kPolicyProxyMode) {
+      EXPECT_TRUE(value);
+      std::string string_value;
+      bool result = value->GetAsString(&string_value);
+      EXPECT_TRUE(result);
+      EXPECT_EQ(string_value, "cache 1");
+    } else if (is_proxy_policy(current->policy_type)) {
+      EXPECT_FALSE(value);
+    } else if (current->policy_type == kPolicyShowHomeButton ||
+               current->policy_type == kPolicyIncognitoEnabled ||
+               current->policy_type == kPolicyTranslateEnabled) {
+      EXPECT_TRUE(value);
+      bool boolean_value = false;
+      EXPECT_TRUE(value->GetAsBoolean(&boolean_value));
+      EXPECT_TRUE(boolean_value);
+    } else {
+      EXPECT_FALSE(value);
+    }
+  }
+}
+
+// Having many different caches adding them to the end incrementally.
+TEST_F(CloudPolicyProviderTest, MultipleCaches) {
+  const int n = simple_list_length();
+  // We set up as many caches as non-proxy policies (n). We configure them
+  // according to the following pattern:
+  // The i-th cache will serve i policies and be at position i in the list
+  // of CloudPolicyProvider.
+  // The i-th cache will be set to initialized iff i % 2 == 1
+  // Thus in the end there should be exactly two policies applied per
+  // initialized cache.
+  // The j-th policy of the i-th cache will be set to i * n + j, thus
+  // given a policy value of x, x / n gives us the cache that provided it and
+  // x % n should give its own identifier.
+  // Prepare |cache[i]| to serve the first i policies from |list|.

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

Again, I find this too complicated. Why not just create 3 caches, add set a few
policies explicitly, merge and examine the resulting policy map?

[gfeher, 2011/06/22 19:18:34]

Done.

+  scoped_array<MockCloudPolicyCache> caches(new MockCloudPolicyCache[n]);
+  for (int cache_id = 0; cache_id < n; ++cache_id) {
+    for (int policy_id = 0; policy_id <= cache_id; ++policy_id) {
+      // Set a unique value.
+      caches[cache_id].raw_recommended_policy()->Set(
+          simple_list_policy_type(policy_id),
+          Value::CreateIntegerValue(cache_id * n + policy_id));
+      caches[cache_id].raw_mandatory_policy()->Set(
+          simple_list_policy_type(policy_id),
+          Value::CreateIntegerValue(cache_id * n + policy_id));
+
+      // Odd numbered caches are initialized and evens are not.
+      caches[cache_id].set_initialized(cache_id % 2);
+    }
+  }
+
+  // Verify that the provider provides policies according to the expected
+  // pattern.
+  VerifyMultipleCaches(caches.get(),
+                       CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED);
+  VerifyMultipleCaches(caches.get(),
+                       CloudPolicyCacheBase::POLICY_LEVEL_MANDATORY);
+}
+
+// Combining two PolicyMaps.
+TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsSame) {
+  PolicyMap A, B, C;
+  CreateCloudPolicyProvider(CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED);
+  for (int i = 0; i < simple_list_length(); ++i) {
+      A.Set(simple_list_policy_type(i), Value::CreateIntegerValue(i));
+      B.Set(simple_list_policy_type(i), Value::CreateIntegerValue(-1 * i));
+  }
+  CombineTwoPolicyMaps(A, B, &C);
+  EXPECT_TRUE(A.Equals(C));
+}
+
+TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsEmpty) {
+  PolicyMap A, B, C;
+  CreateCloudPolicyProvider(CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED);
+  CombineTwoPolicyMaps(A, B, &C);
+  EXPECT_TRUE(C.empty());
+}
+
+TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsPartial) {

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

What additional coverage does this test give over MultipleCaches above?

[gfeher, 2011/06/22 19:18:34]

If this doesn't fail but MultipleCaches does, then we can know that
CloudPolicyProviderImpl::CombineTwoPolicyMaps works correctly but there is some
problem elsewhere in CloudPolicyProviderImpl/CloudPolicyCacheBase.

+  PolicyMap A, B, C;
+  CreateCloudPolicyProvider(CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED);
+
+  for (int i = 0; i < simple_list_length(); ++i) {
+    B.Set(simple_list_policy_type(i), Value::CreateIntegerValue(-i));
+    if (i % 2 == 0)
+      A.Set(simple_list_policy_type(i), Value::CreateIntegerValue(i));
+  }
+
+  CombineTwoPolicyMaps(A, B, &C);
+
+  for (int i = 0; i < simple_list_length(); ++i) {
+    if (const Value* value = C.Get(simple_list_policy_type(i))) {
+      int int_value;
+      EXPECT_TRUE(value->GetAsInteger(&int_value));
+      if (i % 2 == 1) {
+        EXPECT_EQ(int_value, -1 * i);
+      } else {
+        EXPECT_EQ(int_value, i);
+      }
+    }
+  }
+}
+
+TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsProxies) {
+  const int n = proxy_policy_count();
+  const int a_value = 1;
+  const int b_value = -1;
+  PolicyMap A, B, C;
+  CreateCloudPolicyProvider(CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED);
+
+  A.Set(get_proxy_policy(0), Value::CreateIntegerValue(a_value));

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

Why not just use a static constant here?

[gfeher, 2011/06/22 19:18:34]

Done.

+  for (int i = 1; i < n; ++i)
+    B.Set(get_proxy_policy(i), Value::CreateIntegerValue(b_value));

[Mattias Nissler (ping if slow), 2011/06/21 20:09:56]

and here, and unroll the loop?

[gfeher, 2011/06/22 19:18:34]

Done.

+
+  CombineTwoPolicyMaps(A, B, &C);
+
+  EXPECT_TRUE(A.Equals(C));
+  EXPECT_FALSE(B.Equals(C));
+}
+
+}  // namespace policy