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

diff --git a/src/common/types/PolicyBucket.cpp b/src/common/types/PolicyBucket.cpp
index 4b9b508..f1a45a1 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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<std::ostringstream>();
-    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<std::stringstream>();
     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<std::stringstream>();
     StorageSerializer serializer(outStream);

diff --git a/test/storage/serializer/dump_load.cpp b/test/storage/serializer/dump_load.cpp
index 01fff9a..f3eff9b 100644 (file)
--- 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<std::stringstream>();
 

diff --git a/test/storage/storage/check.cpp b/test/storage/storage/check.cpp
index 54c16b4..4935b65 100644 (file)
--- 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)
     }));