Skip to content

Commit

Permalink
Add type checking for policy value retrieval from PolicyMap and Entry.
Browse files Browse the repository at this point in the history 
The new accessor methods take into account the expected value type. If
there is a mismatch between the stored and retrieved value types,
nullptr is returned.

Unsafe accessor methods are also added which do not perform type
checking. Although their implementation is identical to the legacy
methods, having "unsafe" in the name makes users aware that 1. there is
a safer version and 2. they should be used carefully.

The legacy accessor methods will be removed once all existing use cases
have been migrated to use the new type-checking or unsafe methods.

Bug: 1294817, b/221918496
Change-Id: Ib0b740e49c3142079b0ca13208cad813a7932f0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3500732
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Commit-Queue: Igor Ruvinov <igorruvinov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#979200}
  • Loading branch information
Igor Ruvinov authored and Chromium LUCI CQ committed Mar 9, 2022
1 parent 249577d commit 9063512
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 36 deletions.
40 changes: 40 additions & 0 deletions components/policy/core/common/policy_map.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,24 @@ PolicyMap::Entry PolicyMap::Entry::DeepCopy() const {
return copy;
}

const base::Value* PolicyMap::Entry::value(base::Value::Type value_type) const {
const base::Value* value = value_unsafe();
return value && value->type() == value_type ? value : nullptr;
}

base::Value* PolicyMap::Entry::value(base::Value::Type value_type) {
base::Value* value = value_unsafe();
return value && value->type() == value_type ? value : nullptr;
}

const base::Value* PolicyMap::Entry::value_unsafe() const {
return base::OptionalOrNullptr(value_);
}

base::Value* PolicyMap::Entry::value_unsafe() {
return base::OptionalOrNullptr(value_);
}

base::Value* PolicyMap::Entry::value() {
return base::OptionalOrNullptr(value_);
}
Expand Down Expand Up @@ -309,6 +327,28 @@ PolicyMap::Entry* PolicyMap::GetMutable(const std::string& policy) {
: nullptr;
}

const base::Value* PolicyMap::GetValue(const std::string& policy,
base::Value::Type value_type) const {
auto* entry = Get(policy);
return entry ? entry->value(value_type) : nullptr;
}

base::Value* PolicyMap::GetMutableValue(const std::string& policy,
base::Value::Type value_type) {
auto* entry = GetMutable(policy);
return entry ? entry->value(value_type) : nullptr;
}

const base::Value* PolicyMap::GetValueUnsafe(const std::string& policy) const {
auto* entry = Get(policy);
return entry ? entry->value_unsafe() : nullptr;
}

base::Value* PolicyMap::GetMutableValueUnsafe(const std::string& policy) {
auto* entry = GetMutable(policy);
return entry ? entry->value_unsafe() : nullptr;
}

const base::Value* PolicyMap::GetValue(const std::string& policy) const {
auto entry = map_.find(policy);
return entry != map_.end() && !entry->second.ignored() ? entry->second.value()
Expand Down
33 changes: 30 additions & 3 deletions components/policy/core/common/policy_map.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,18 @@ class POLICY_EXPORT PolicyMap {
// Returns a copy of |this|.
Entry DeepCopy() const;

// Retrieves the stored value if its type matches the desired type,
// otherwise returns |nullptr|.
const base::Value* value(base::Value::Type value_type) const;
base::Value* value(base::Value::Type value_type);

// Retrieves the stored value without performing type checking. Use the
// type-checking versions above where possible.
const base::Value* value_unsafe() const;
base::Value* value_unsafe();

// DEPRECATED - do not use in new code. Use either the type-checking or
// unsafe versions above.
base::Value* value();
const base::Value* value() const;

Expand Down Expand Up @@ -180,9 +192,24 @@ class POLICY_EXPORT PolicyMap {
const Entry* Get(const std::string& policy) const;
Entry* GetMutable(const std::string& policy);

// Returns a weak reference to the value currently stored for key
// |policy|, or NULL if not found. Ownership is retained by the PolicyMap.
// This is equivalent to Get(policy)->value, when it doesn't return NULL.
// Returns a weak reference to the value currently stored for key |policy| if
// the value type matches the requested type, otherwise returns |nullptr| if
// not found or there is a type mismatch. Ownership is retained by the
// |PolicyMap|.
const base::Value* GetValue(const std::string& policy,
base::Value::Type value_type) const;
base::Value* GetMutableValue(const std::string& policy,
base::Value::Type value_type);

// Returns a weak reference to the value currently stored for key |policy|
// without performing type checking, otherwise returns |nullptr| if not found.
// Ownership is retained by the |PolicyMap|. Use the type-checking versions
// above where possible.
const base::Value* GetValueUnsafe(const std::string& policy) const;
base::Value* GetMutableValueUnsafe(const std::string& policy);

// DEPRECATED - do not use in new code. Use either the type-checking or unsafe
// versions above.
const base::Value* GetValue(const std::string& policy) const;
base::Value* GetMutableValue(const std::string& policy);

Expand Down
106 changes: 73 additions & 33 deletions components/policy/core/common/policy_map_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,34 @@ class PolicyMapTest : public PolicyMapTestBase, public testing::Test {};
TEST_F(PolicyMapTest, SetAndGet) {
PolicyMap map;
SetPolicy(&map, kTestPolicyName1, base::Value("aaa"));
base::Value expected("aaa");
EXPECT_TRUE(expected == *map.GetValue(kTestPolicyName1));
const base::Value kExpectedStringA("aaa");
EXPECT_EQ(kExpectedStringA, *map.GetValueUnsafe(kTestPolicyName1));
EXPECT_EQ(kExpectedStringA,
*map.GetValue(kTestPolicyName1, base::Value::Type::STRING));
EXPECT_EQ(nullptr,
map.GetValue(kTestPolicyName1, base::Value::Type::BOOLEAN));

SetPolicy(&map, kTestPolicyName1, base::Value("bbb"));
base::Value expected_b("bbb");
EXPECT_TRUE(expected_b == *map.GetValue(kTestPolicyName1));
const base::Value kExpectedStringB("bbb");
EXPECT_EQ(kExpectedStringB, *map.GetValueUnsafe(kTestPolicyName1));
EXPECT_EQ(kExpectedStringB,
*map.GetValue(kTestPolicyName1, base::Value::Type::STRING));
EXPECT_EQ(nullptr,
map.GetValue(kTestPolicyName1, base::Value::Type::BOOLEAN));

SetPolicy(&map, kTestPolicyName1, base::Value(true));
const base::Value kExpectedBool(true);
EXPECT_EQ(kExpectedBool, *map.GetValueUnsafe(kTestPolicyName1));
EXPECT_EQ(nullptr, map.GetValue(kTestPolicyName1, base::Value::Type::STRING));
EXPECT_EQ(kExpectedBool,
*map.GetValue(kTestPolicyName1, base::Value::Type::BOOLEAN));

SetPolicy(&map, kTestPolicyName1, CreateExternalDataFetcher("dummy"));
map.AddMessage(kTestPolicyName1, PolicyMap::MessageType::kError,
IDS_POLICY_STORE_STATUS_VALIDATION_ERROR, {kTestError});
EXPECT_FALSE(map.GetValue(kTestPolicyName1));
EXPECT_EQ(nullptr, map.GetValueUnsafe(kTestPolicyName1));
const PolicyMap::Entry* entry = map.Get(kTestPolicyName1);
ASSERT_TRUE(entry != nullptr);
ASSERT_NE(nullptr, entry);
EXPECT_EQ(POLICY_LEVEL_MANDATORY, entry->level);
EXPECT_EQ(POLICY_SCOPE_USER, entry->scope);
EXPECT_EQ(POLICY_SOURCE_CLOUD, entry->source);
Expand All @@ -104,9 +121,9 @@ TEST_F(PolicyMapTest, SetAndGet) {
CreateExternalDataFetcher("dummy").get()));
map.Set(kTestPolicyName1, POLICY_LEVEL_RECOMMENDED, POLICY_SCOPE_MACHINE,
POLICY_SOURCE_ENTERPRISE_DEFAULT, absl::nullopt, nullptr);
EXPECT_FALSE(map.GetValue(kTestPolicyName1));
EXPECT_EQ(nullptr, map.GetValueUnsafe(kTestPolicyName1));
entry = map.Get(kTestPolicyName1);
ASSERT_TRUE(entry != nullptr);
ASSERT_NE(nullptr, entry);
EXPECT_EQ(POLICY_LEVEL_RECOMMENDED, entry->level);
EXPECT_EQ(POLICY_SCOPE_MACHINE, entry->scope);
EXPECT_EQ(POLICY_SOURCE_ENTERPRISE_DEFAULT, entry->source);
Expand Down Expand Up @@ -334,16 +351,19 @@ TEST_F(PolicyMapTest, Swap) {
SetPolicy(&b, kTestPolicyName3, base::Value(true));

a.Swap(&b);
base::Value expected("bbb");
EXPECT_TRUE(expected == *a.GetValue(kTestPolicyName1));
base::Value expected_bool(true);
EXPECT_TRUE(expected_bool == *a.GetValue(kTestPolicyName3));
EXPECT_FALSE(a.GetValue(kTestPolicyName2));
EXPECT_FALSE(a.Get(kTestPolicyName2));
base::Value expected_a("aaa");
EXPECT_TRUE(expected_a == *b.GetValue(kTestPolicyName1));
EXPECT_FALSE(b.GetValue(kTestPolicyName3));
EXPECT_FALSE(a.GetValue(kTestPolicyName2));
const base::Value kExpectedStringB("bbb");
EXPECT_EQ(kExpectedStringB,
*a.GetValue(kTestPolicyName1, base::Value::Type::STRING));
const base::Value kExpectedBool(true);
EXPECT_EQ(kExpectedBool,
*a.GetValue(kTestPolicyName3, base::Value::Type::BOOLEAN));
EXPECT_EQ(nullptr, a.GetValueUnsafe(kTestPolicyName2));
EXPECT_EQ(nullptr, a.Get(kTestPolicyName2));
const base::Value kExpectedStringA("aaa");
EXPECT_EQ(kExpectedStringA,
*b.GetValue(kTestPolicyName1, base::Value::Type::STRING));
EXPECT_EQ(nullptr, b.GetValueUnsafe(kTestPolicyName3));
EXPECT_EQ(nullptr, a.GetValueUnsafe(kTestPolicyName2));
const PolicyMap::Entry* entry = b.Get(kTestPolicyName2);
ASSERT_TRUE(entry);
EXPECT_TRUE(
Expand Down Expand Up @@ -1110,23 +1130,35 @@ TEST_F(PolicyMapTest, BlockedEntry) {
policies.Set("c", entry_c_blocked.DeepCopy());

const size_t expected_size = 3;
EXPECT_TRUE(policies.size() == expected_size);
EXPECT_EQ(policies.size(), expected_size);

EXPECT_TRUE(policies.Get("a")->Equals(entry_a));
EXPECT_TRUE(policies.Get("b")->Equals(entry_b));
EXPECT_TRUE(policies.Get("c") == nullptr);
EXPECT_EQ(policies.Get("c"), nullptr);

EXPECT_TRUE(policies.GetMutable("a")->Equals(entry_a));
EXPECT_TRUE(policies.GetMutable("b")->Equals(entry_b));
EXPECT_TRUE(policies.GetMutable("c") == nullptr);
EXPECT_EQ(policies.GetMutable("c"), nullptr);

EXPECT_EQ(*policies.GetValue("a", base::Value::Type::STRING),
*entry_a.value(base::Value::Type::STRING));
EXPECT_EQ(*policies.GetValue("b", base::Value::Type::STRING),
*entry_b.value(base::Value::Type::STRING));
EXPECT_EQ(policies.GetValue("c", base::Value::Type::STRING), nullptr);

EXPECT_TRUE(*policies.GetValue("a") == *entry_a.value());
EXPECT_TRUE(*policies.GetValue("b") == *entry_b.value());
EXPECT_TRUE(policies.GetValue("c") == nullptr);
EXPECT_EQ(*policies.GetValueUnsafe("a"), *entry_a.value_unsafe());
EXPECT_EQ(*policies.GetValueUnsafe("b"), *entry_b.value_unsafe());
EXPECT_EQ(policies.GetValueUnsafe("c"), nullptr);

EXPECT_TRUE(*policies.GetMutableValue("a") == *entry_a.value());
EXPECT_TRUE(*policies.GetMutableValue("b") == *entry_b.value());
EXPECT_TRUE(policies.GetMutableValue("c") == nullptr);
EXPECT_EQ(*policies.GetMutableValue("a", base::Value::Type::STRING),
*entry_a.value(base::Value::Type::STRING));
EXPECT_EQ(*policies.GetMutableValue("b", base::Value::Type::STRING),
*entry_b.value(base::Value::Type::STRING));
EXPECT_EQ(policies.GetMutableValue("c", base::Value::Type::STRING), nullptr);

EXPECT_EQ(*policies.GetMutableValueUnsafe("a"), *entry_a.value_unsafe());
EXPECT_EQ(*policies.GetMutableValueUnsafe("b"), *entry_b.value_unsafe());
EXPECT_EQ(policies.GetMutableValueUnsafe("c"), nullptr);

EXPECT_TRUE(policies.GetUntrusted("a")->Equals(entry_a));
EXPECT_TRUE(policies.GetUntrusted("b")->Equals(entry_b));
Expand Down Expand Up @@ -1162,16 +1194,24 @@ TEST_F(PolicyMapTest, InvalidEntry) {
EXPECT_EQ(policies.size(), expected_size);

EXPECT_TRUE(policies.Get("a")->Equals(entry_a));
EXPECT_TRUE(policies.Get("b") == nullptr);
EXPECT_EQ(policies.Get("b"), nullptr);

EXPECT_TRUE(policies.GetMutable("a")->Equals(entry_a));
EXPECT_TRUE(policies.GetMutable("b") == nullptr);
EXPECT_EQ(policies.GetMutable("b"), nullptr);

EXPECT_EQ(*policies.GetValue("a", base::Value::Type::STRING),
*entry_a.value(base::Value::Type::STRING));
EXPECT_EQ(policies.GetValue("b", base::Value::Type::STRING), nullptr);

EXPECT_EQ(*policies.GetValueUnsafe("a"), *entry_a.value_unsafe());
EXPECT_EQ(policies.GetValueUnsafe("b"), nullptr);

EXPECT_TRUE(*policies.GetValue("a") == *entry_a.value());
EXPECT_TRUE(policies.GetValue("b") == nullptr);
EXPECT_EQ(*policies.GetMutableValue("a", base::Value::Type::STRING),
*entry_a.value(base::Value::Type::STRING));
EXPECT_EQ(policies.GetMutableValue("b", base::Value::Type::STRING), nullptr);

EXPECT_TRUE(*policies.GetMutableValue("a") == *entry_a.value());
EXPECT_TRUE(policies.GetMutableValue("b") == nullptr);
EXPECT_EQ(*policies.GetMutableValueUnsafe("a"), *entry_a.value_unsafe());
EXPECT_EQ(policies.GetMutableValueUnsafe("b"), nullptr);

EXPECT_TRUE(policies.GetUntrusted("a")->Equals(entry_a));
EXPECT_TRUE(policies.GetUntrusted("b")->Equals(entry_b_invalid));
Expand Down

0 comments on commit 9063512

Please sign in to comment.