From 94c676f6a67966566072093dadf217ddd20849a1 Mon Sep 17 00:00:00 2001 From: Pawel Wieczorek Date: Mon, 6 Oct 2014 12:45:25 +0200 Subject: [PATCH] Remove PolicyBucket() constructor In some cases using parameterless constructor of PolicyBucket can result in uninitialized PolicyBucket id. Complete removal of this constructor guarantees inablity to create bucket with no id. Change-Id: Id67d7f257697078ef0d4518161ade473a983cf6b --- src/common/types/PolicyBucket.cpp | 2 +- src/common/types/PolicyBucket.h | 15 ++++++++++----- src/storage/InMemoryStorageBackend.cpp | 2 +- src/storage/StorageDeserializer.cpp | 3 ++- test/common/types/policybucket.cpp | 16 +++++++++------- test/storage/inmemorystoragebackend/buckets.cpp | 4 ++-- .../inmemeorystoragebackendfixture.h | 6 +++--- test/storage/performance/bucket.cpp | 2 +- test/storage/serializer/dump.cpp | 12 ++++++------ test/storage/serializer/dump_load.cpp | 6 +++--- test/storage/storage/check.cpp | 14 +++++++------- 11 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/common/types/PolicyBucket.cpp b/src/common/types/PolicyBucket.cpp index 4b9b508..f1a45a1 100644 --- a/src/common/types/PolicyBucket.cpp +++ b/src/common/types/PolicyBucket.cpp @@ -30,7 +30,7 @@ namespace Cynara { PolicyBucket PolicyBucket::filtered(const PolicyKey &key) const { - PolicyBucket result; + PolicyBucket result(m_id + "_filtered"); const auto &policies = m_policyCollection; const auto variants = PolicyKeyHelpers::keyVariants(key); diff --git a/src/common/types/PolicyBucket.h b/src/common/types/PolicyBucket.h index 122ece0..a2faf9c 100644 --- a/src/common/types/PolicyBucket.h +++ b/src/common/types/PolicyBucket.h @@ -49,12 +49,17 @@ public: typedef const_policy_iterator const_iterator; // TODO: Review usefulness of ctors - PolicyBucket() : m_defaultPolicy(PredefinedPolicyType::DENY) {} - PolicyBucket(const PolicyBucketId &id, const PolicyResult &defaultPolicy) - : m_defaultPolicy(defaultPolicy), m_id(id) {} - PolicyBucket(const PolicyCollection &policies) + //delete default constructor in order to prevent creation of buckets with no id + PolicyBucket() = delete; + PolicyBucket(const PolicyBucketId &id, + const PolicyResult &defaultPolicy = PredefinedPolicyType::DENY) + : m_defaultPolicy(defaultPolicy), + m_id(id) {} + PolicyBucket(const PolicyBucketId &id, + const PolicyCollection &policies) : m_policyCollection(makePolicyMap(policies)), - m_defaultPolicy(PredefinedPolicyType::DENY) {} + m_defaultPolicy(PredefinedPolicyType::DENY), + m_id(id) {} PolicyBucket(const PolicyBucketId &id, const PolicyResult &defaultPolicy, const PolicyCollection &policies) diff --git a/src/storage/InMemoryStorageBackend.cpp b/src/storage/InMemoryStorageBackend.cpp index ae9c624..af18926 100644 --- a/src/storage/InMemoryStorageBackend.cpp +++ b/src/storage/InMemoryStorageBackend.cpp @@ -72,7 +72,7 @@ void InMemoryStorageBackend::load(void) { if (!hasBucket(defaultPolicyBucketId)) { LOGN("Creating defaultBucket."); - this->buckets().insert({ defaultPolicyBucketId, PolicyBucket() }); + this->buckets().insert({ defaultPolicyBucketId, PolicyBucket(defaultPolicyBucketId) }); } } diff --git a/src/storage/StorageDeserializer.cpp b/src/storage/StorageDeserializer.cpp index 22d3231..166a406 100644 --- a/src/storage/StorageDeserializer.cpp +++ b/src/storage/StorageDeserializer.cpp @@ -56,7 +56,8 @@ void StorageDeserializer::initBuckets(Buckets &buckets) { auto policyType = parsePolicyType(line, beginToken); auto metadata = parseMetadata(line, beginToken); - buckets[bucketId] = PolicyBucket(bucketId, PolicyResult(policyType, metadata)); + //it's safe to simply insert; buckets were cleared earlier, all ids should be unique + buckets.insert({ bucketId, PolicyBucket(bucketId, PolicyResult(policyType, metadata)) }); } } diff --git a/test/common/types/policybucket.cpp b/test/common/types/policybucket.cpp index 0468782..983a040 100644 --- a/test/common/types/policybucket.cpp +++ b/test/common/types/policybucket.cpp @@ -62,7 +62,7 @@ protected: TEST_F(PolicyBucketFixture, filtered) { using ::testing::UnorderedElementsAre; - PolicyBucket bucket(pkPolicies); + PolicyBucket bucket("filtered", pkPolicies); bucket.setDefaultPolicy(PredefinedPolicyType::DENY); auto filtered = bucket.filtered(pk1); @@ -76,7 +76,7 @@ TEST_F(PolicyBucketFixture, filtered) { TEST_F(PolicyBucketFixture, filtered_other) { using ::testing::IsEmpty; - PolicyBucket bucket(pkPolicies); + PolicyBucket bucket("filtered_other", pkPolicies); bucket.setDefaultPolicy(PredefinedPolicyType::DENY); auto filtered = bucket.filtered(otherPk); @@ -93,7 +93,7 @@ TEST_F(PolicyBucketFixture, filtered_wildcard_1) { // Leave policies with given client, given user and any privilege auto policiesToStay = Helpers::pickFromCollection(wildcardPolicies, { 0, 1, 3 }); - PolicyBucket bucket(wildcardPolicies); + PolicyBucket bucket("filtered_wildcard_1", wildcardPolicies); auto filtered = bucket.filtered(PolicyKey("c1", "u1", "p2")); ASSERT_THAT(filtered, UnorderedElementsAreArray(policiesToStay)); } @@ -104,7 +104,7 @@ TEST_F(PolicyBucketFixture, filtered_wildcard_2) { // Leave policies with given client, given user and any privilege auto policiesToStay = Helpers::pickFromCollection(wildcardPolicies, { 2, 3 }); - PolicyBucket bucket(wildcardPolicies); + PolicyBucket bucket("filtered_wildcard_2", wildcardPolicies); auto filtered = bucket.filtered(PolicyKey("cccc", "u1", "p1")); ASSERT_THAT(filtered, UnorderedElementsAreArray(policiesToStay)); @@ -116,7 +116,7 @@ TEST_F(PolicyBucketFixture, filtered_wildcard_3) { // Leave policies with given client, given user and any privilege auto policiesToStay = Helpers::pickFromCollection(wildcardPolicies, { 0, 3 }); - PolicyBucket bucket(wildcardPolicies); + PolicyBucket bucket("filtered_wildcard_3", wildcardPolicies); auto filtered = bucket.filtered(PolicyKey("c1", "u1", "pppp")); ASSERT_THAT(filtered, UnorderedElementsAreArray(policiesToStay)); } @@ -127,7 +127,7 @@ TEST_F(PolicyBucketFixture, filtered_wildcard_4) { // Leave policies with given client, given user and any privilege auto policiesToStay = Helpers::pickFromCollection(wildcardPolicies, { 3 }); - PolicyBucket bucket(wildcardPolicies); + PolicyBucket bucket("filtered_wildcard_4", wildcardPolicies); auto filtered = bucket.filtered(PolicyKey("cccc", "uuuu", "pppp")); ASSERT_THAT(filtered, UnorderedElementsAreArray(policiesToStay)); } @@ -135,7 +135,9 @@ TEST_F(PolicyBucketFixture, filtered_wildcard_4) { TEST_F(PolicyBucketFixture, filtered_wildcard_none) { using ::testing::IsEmpty; - PolicyBucket bucket({ wildcardPolicies.begin(), wildcardPolicies.begin() + 3 }); + PolicyBucket bucket("filtered_wildcard_none", + PolicyCollection({ wildcardPolicies.begin(), + wildcardPolicies.begin() + 3 })); auto filtered = bucket.filtered(PolicyKey("cccc", "uuuu", "pppp")); ASSERT_THAT(filtered, IsEmpty()); } diff --git a/test/storage/inmemorystoragebackend/buckets.cpp b/test/storage/inmemorystoragebackend/buckets.cpp index b7e2bdd..f53b51c 100644 --- a/test/storage/inmemorystoragebackend/buckets.cpp +++ b/test/storage/inmemorystoragebackend/buckets.cpp @@ -86,7 +86,7 @@ TEST_F(InMemeoryStorageBackendFixture, deleteBucket) { .WillRepeatedly(ReturnRef(m_buckets)); PolicyBucketId bucketId = "delete-bucket"; - m_buckets.insert({ bucketId, PolicyBucket() }); + m_buckets.insert({ bucketId, PolicyBucket(bucketId) }); backend.deleteBucket(bucketId); @@ -102,7 +102,7 @@ TEST_F(InMemeoryStorageBackendFixture, hasBucket) { .WillRepeatedly(ReturnRef(m_buckets)); PolicyBucketId bucketId = "bucket"; - m_buckets.insert({ bucketId, PolicyBucket() }); + m_buckets.insert({ bucketId, PolicyBucket(bucketId) }); ASSERT_TRUE(backend.hasBucket(bucketId)); ASSERT_FALSE(backend.hasBucket("non-existent")); diff --git a/test/storage/inmemorystoragebackend/inmemeorystoragebackendfixture.h b/test/storage/inmemorystoragebackend/inmemeorystoragebackendfixture.h index 2c0624b..3f08dea 100644 --- a/test/storage/inmemorystoragebackend/inmemeorystoragebackendfixture.h +++ b/test/storage/inmemorystoragebackend/inmemeorystoragebackendfixture.h @@ -36,20 +36,20 @@ class InMemeoryStorageBackendFixture : public ::testing::Test { protected: Cynara::Buckets::mapped_type & createBucket(const Cynara::PolicyBucketId &bucketId) { - auto bucket = Cynara::PolicyBucket(); + auto bucket = Cynara::PolicyBucket(bucketId); return m_buckets.insert({ bucketId, bucket }).first->second; } Cynara::Buckets::mapped_type & createBucket(const Cynara::PolicyBucketId &bucketId, const Cynara::PolicyCollection &policies) { - auto bucket = Cynara::PolicyBucket(policies); + auto bucket = Cynara::PolicyBucket(bucketId, policies); return m_buckets.insert({ bucketId, bucket }).first->second; } void addToBucket(Cynara::PolicyBucketId bucketId, const Cynara::PolicyCollection &policies) { // TODO: Consider altering PolicyMap directly for (const auto &policy : policies) { - m_buckets[bucketId].insertPolicy(policy); + m_buckets.at(bucketId).insertPolicy(policy); } } diff --git a/test/storage/performance/bucket.cpp b/test/storage/performance/bucket.cpp index 1e63966..e55da7f 100644 --- a/test/storage/performance/bucket.cpp +++ b/test/storage/performance/bucket.cpp @@ -83,7 +83,7 @@ private: TEST(Performance, bucket_filtered_100000) { using std::chrono::microseconds; - PolicyBucket bucket; + PolicyBucket bucket("test"); PolicyKeyGenerator generator(100, 10); diff --git a/test/storage/serializer/dump.cpp b/test/storage/serializer/dump.cpp index 53034ce..2eafcf7 100644 --- a/test/storage/serializer/dump.cpp +++ b/test/storage/serializer/dump.cpp @@ -49,7 +49,7 @@ static std::string expectedPolicyType(const PolicyType &type) { TEST(serializer_dump, dump_empty_bucket) { auto oss = std::make_shared(); - PolicyBucket bucket; + PolicyBucket bucket("empty"); StorageSerializer serializer(oss); serializer.dump(bucket); @@ -65,8 +65,8 @@ TEST(serializer_dump, dump_bucket) { PolicyKey pk1 = Helpers::generatePolicyKey("1"); PolicyKey pk2 = Helpers::generatePolicyKey("2"); - PolicyBucket bucket = {{ Policy::simpleWithKey(pk1, ALLOW), - Policy::simpleWithKey(pk2, DENY) }}; + PolicyBucket bucket("dump_bucket", PolicyCollection({ Policy::simpleWithKey(pk1, ALLOW), + Policy::simpleWithKey(pk2, DENY) })); auto outStream = std::make_shared(); StorageSerializer serializer(outStream); @@ -94,9 +94,9 @@ TEST(serializer_dump, dump_bucket_bucket) { PolicyKey pk3 = Helpers::generatePolicyKey("3"); PolicyBucketId bucketId = Helpers::generateBucketId(); - PolicyBucket bucket = {{ Policy::bucketWithKey(pk1, bucketId), - Policy::simpleWithKey(pk2, DENY), - Policy::bucketWithKey(pk3, bucketId) }}; + PolicyBucket bucket = {"dump_bucket_bucket", { Policy::bucketWithKey(pk1, bucketId), + Policy::simpleWithKey(pk2, DENY), + Policy::bucketWithKey(pk3, bucketId) }}; auto outStream = std::make_shared(); StorageSerializer serializer(outStream); diff --git a/test/storage/serializer/dump_load.cpp b/test/storage/serializer/dump_load.cpp index 01fff9a..f3eff9b 100644 --- a/test/storage/serializer/dump_load.cpp +++ b/test/storage/serializer/dump_load.cpp @@ -48,9 +48,9 @@ TEST(dump_load, bucket) { PolicyKey pk2 = Helpers::generatePolicyKey("2"); PolicyKey pk3 = Helpers::generatePolicyKey("3"); PolicyBucketId bucketId = Helpers::generateBucketId(); - PolicyBucket bucket = {{ Policy::bucketWithKey(pk1, bucketId), - Policy::simpleWithKey(pk2, PredefinedPolicyType::DENY), - Policy::bucketWithKey(pk3, bucketId) }}; + PolicyBucket bucket = {"bucket", { Policy::bucketWithKey(pk1, bucketId), + Policy::simpleWithKey(pk2, PredefinedPolicyType::DENY), + Policy::bucketWithKey(pk3, bucketId) }}; auto ioStream = std::make_shared(); diff --git a/test/storage/storage/check.cpp b/test/storage/storage/check.cpp index 54c16b4..4935b65 100644 --- a/test/storage/storage/check.cpp +++ b/test/storage/storage/check.cpp @@ -43,7 +43,7 @@ using namespace Cynara; TEST(storage, checkEmpty) { using ::testing::ReturnPointee; - PolicyBucket emptyBucket; + PolicyBucket emptyBucket("empty"); FakeStorageBackend backend; Cynara::Storage storage(backend); @@ -60,7 +60,7 @@ TEST(storage, checkEmpty) { TEST(storage, checkSimple) { using ::testing::ReturnPointee; - PolicyBucket bucket; + PolicyBucket bucket(defaultPolicyBucketId); FakeStorageBackend backend; Cynara::Storage storage(backend); @@ -91,12 +91,12 @@ TEST(storage, checkBucket) { Cynara::Storage storage(backend); PolicyKey pk = Helpers::generatePolicyKey(); - PolicyBucket defaultBucket(PolicyCollection({ + PolicyBucket defaultBucket(defaultPolicyBucketId, PolicyCollection({ Policy::simpleWithKey(pk, PredefinedPolicyType::ALLOW), Policy::bucketWithKey(pk, additionalBucketId) })); - PolicyBucket additionalBucket; + PolicyBucket additionalBucket("additional"); EXPECT_CALL(backend, searchBucket(defaultPolicyBucketId, pk)) .WillRepeatedly(ReturnPointee(&defaultBucket)); @@ -127,7 +127,7 @@ TEST(storage, checkBucketWildcard) { const PolicyKey defaultBucketKey = PolicyKey("c", "*", "p"); const PolicyKey checkKey = PolicyKey("c", "u1", "p"); - PolicyBucket defaultBucket(PolicyCollection({ + PolicyBucket defaultBucket(defaultPolicyBucketId, PolicyCollection({ Policy::bucketWithKey(defaultBucketKey, additionalBucketId) })); @@ -139,7 +139,7 @@ TEST(storage, checkBucketWildcard) { // Check, if next bucket is filtered with original key EXPECT_CALL(backend, searchBucket(additionalBucketId, checkKey)) - .WillRepeatedly(Return(PolicyBucket())); // additional bucket would yield no records + .WillRepeatedly(Return(PolicyBucket("id"))); // additional bucket would yield no records // Should return additional bucket's default policy ASSERT_EQ(PredefinedPolicyType::DENY, storage.checkPolicy(checkKey)); @@ -152,7 +152,7 @@ TEST(storage, checkBucketWildcardOtherDefault) { const PolicyKey defaultBucketKey = PolicyKey("c", "*", "p"); const PolicyKey checkKey = PolicyKey("c", "u1", "p"); - PolicyBucket defaultBucket(PolicyCollection({ + PolicyBucket defaultBucket(defaultPolicyBucketId, PolicyCollection({ Policy::bucketWithKey(defaultBucketKey, additionalBucketId) })); -- 2.7.4