QPID-4735: ACL file size/count upper limit checks incorrect

Merge IntMin and IntMax functions into a single function to contain duplicated code.
Improve log messages so that reading a log file is less painful.
Turn on ACL debug logging for main broker in ACL self test.



git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1466652 13f79535-47bb-0310-9956-ffa450edef68

4 files changed, 129 insertions, 75 deletions

diff --git a/qpid/cpp/src/qpid/acl/AclData.cpp b/qpid/cpp/src/qpid/acl/AclData.cpp
index 847b67cb58..68fc137612 100644
--- a/qpid/cpp/src/qpid/acl/AclData.cpp
+++ b/qpid/cpp/src/qpid/acl/AclData.cpp
@@ -254,10 +254,11 @@ namespace acl {
                                     case acl::SPECPROP_MAXFILECOUNTUPPERLIMIT:
                                     case acl::SPECPROP_MAXFILESIZEUPPERLIMIT:
                                         limitChecked &=
-                                            compareIntMax(
+                                            compareInt(
                                                 rulePropMapItr->first,
                                                 boost::lexical_cast<std::string>(rulePropMapItr->second),
-                                                boost::lexical_cast<std::string>(lookupParamItr->second));
+                                                boost::lexical_cast<std::string>(lookupParamItr->second),
+                                                true);
                                         break;
 
                                     case acl::SPECPROP_MAXQUEUECOUNTLOWERLIMIT:
@@ -265,10 +266,11 @@ namespace acl {
                                     case acl::SPECPROP_MAXFILECOUNTLOWERLIMIT:
                                     case acl::SPECPROP_MAXFILESIZELOWERLIMIT:
                                         limitChecked &=
-                                            compareIntMin(
+                                            compareInt(
                                                 rulePropMapItr->first,
                                                 boost::lexical_cast<std::string>(rulePropMapItr->second),
-                                                boost::lexical_cast<std::string>(lookupParamItr->second));
+                                                boost::lexical_cast<std::string>(lookupParamItr->second),
+                                                false);
                                         break;
 
                                     default:
@@ -635,95 +637,56 @@ namespace acl {
 
 
     //
-    // Limit check a MAX int limit
+    // Limit check an int limit
     //
-    bool AclData::compareIntMax(const qpid::acl::SpecProperty theProperty,
-                                const std::string             theAclValue,
-                                const std::string             theLookupValue)
+    bool AclData::compareInt(const qpid::acl::SpecProperty theProperty,
+                             const std::string             theAclValue,
+                             const std::string             theLookupValue,
+                             bool                          theMaxFlag)
     {
-        uint64_t aclMax   (0);
-        uint64_t paramMax (0);
+        uint64_t aclRuleValue (0);
+        uint64_t lookupValue  (0);
 
-        try
-        {
-            aclMax = boost::lexical_cast<uint64_t>(theAclValue);
-        }
-        catch(const boost::bad_lexical_cast&)
-        {
-            assert (false);
-            return false;
-        }
+        QPID_LOG(debug, "ACL: "
+            << (theMaxFlag ? "Upper" : "Lower") << "-limit comparison for property "
+            << AclHelper::getPropertyStr(theProperty)
+            << ". Success if lookup(" << theLookupValue
+            << ") "
+            << (theMaxFlag ? "<=" : ">=") << " rule(" << theAclValue << ")");
 
         try
         {
-            paramMax = boost::lexical_cast<uint64_t>(theLookupValue);
+            aclRuleValue = boost::lexical_cast<uint64_t>(theAclValue);
         }
         catch(const boost::bad_lexical_cast&)
         {
-            QPID_LOG(error,"ACL: Error evaluating rule. "
-                << "Illegal value given in lookup for property '"
-                << AclHelper::getPropertyStr(theProperty)
-                << "' : " << theLookupValue);
-            return false;
-        }
-
-        QPID_LOG(debug, "ACL: Numeric greater-than comparison for property "
-            << AclHelper::getPropertyStr(theProperty)
-            << " (value given in lookup = " << theLookupValue
-            << ", value give in rule = " << theAclValue << " )");
-
-        if (( aclMax ) && ( paramMax == 0 || paramMax > aclMax))
-        {
-            QPID_LOG(debug, "ACL: Max limit exceeded for property '"
-                << AclHelper::getPropertyStr(theProperty) << "'");
+            assert (false);
             return false;
         }
 
-        return true;
-    }
-
-
-    //
-    // limit check a MIN int limit
-    //
-    bool AclData::compareIntMin(const qpid::acl::SpecProperty theProperty,
-                                const std::string             theAclValue,
-                                const std::string             theLookupValue)
-    {
-        uint64_t aclMin   (0);
-        uint64_t paramMin (0);
-
-        try
-        {
-            aclMin = boost::lexical_cast<uint64_t>(theAclValue);
-        }
-        catch(const boost::bad_lexical_cast&)
+        if (aclRuleValue == 0)
         {
-            assert (false);
-            return false;
+            QPID_LOG(debug, "ACL: Comparison is always true when ACL rule value is zero");
+            return true;
         }
 
         try
         {
-            paramMin = boost::lexical_cast<uint64_t>(theLookupValue);
+            lookupValue = boost::lexical_cast<uint64_t>(theLookupValue);
         }
         catch(const boost::bad_lexical_cast&)
         {
-            QPID_LOG(error,"ACL: Error evaluating rule. "
-                << "Illegal value given in lookup for property '"
+            QPID_LOG(error,"ACL: Illegal value given in lookup for property '"
                 << AclHelper::getPropertyStr(theProperty)
                 << "' : " << theLookupValue);
             return false;
         }
 
-        QPID_LOG(debug, "ACL: Numeric less-than comparison for property "
-            << AclHelper::getPropertyStr(theProperty)
-            << " (value given in lookup = " << theLookupValue
-            << ", value give in rule = " << theAclValue << " )");
-
-        if (( aclMin ) && ( paramMin == 0 || paramMin < aclMin))
+        bool result =
+        (theMaxFlag ? lookupValue > aclRuleValue : lookupValue < aclRuleValue);
+        if ( result )
         {
-            QPID_LOG(debug, "ACL: Min limit exceeded for property '"
+            QPID_LOG(debug, "ACL: Limit exceeded for property '"
                 << AclHelper::getPropertyStr(theProperty) << "'");
             return false;
         }
diff --git a/qpid/cpp/src/qpid/acl/AclData.h b/qpid/cpp/src/qpid/acl/AclData.h
index cd41e6d315..afc9ce7c2a 100644
--- a/qpid/cpp/src/qpid/acl/AclData.h
+++ b/qpid/cpp/src/qpid/acl/AclData.h
@@ -204,13 +204,11 @@ public:
     virtual ~AclData();
 
 private:
-    bool compareIntMax(const qpid::acl::SpecProperty theProperty,
-                       const std::string             theAclValue,
-                       const std::string             theLookupValue);
 
-    bool compareIntMin(const qpid::acl::SpecProperty theProperty,
-                       const std::string             theAclValue,
-                       const std::string             theLookupValue);
+    bool compareInt(const qpid::acl::SpecProperty theProperty,
+                    const std::string             theAclValue,
+                    const std::string             theLookupValue,
+                    bool                          theMaxFlag);
 
     // Per-user connection quota
     bool connQuotaRulesExist;
diff --git a/qpid/cpp/src/tests/acl.py b/qpid/cpp/src/tests/acl.py
index a8861db170..41f1e7a8cd 100755
--- a/qpid/cpp/src/tests/acl.py
+++ b/qpid/cpp/src/tests/acl.py
@@ -976,6 +976,7 @@ class ACLTests(TestBase010):
         aclf.write('acl deny bob@QPID purge queue name=qf3\n')
         aclf.write('acl deny bob@QPID delete queue name=qf4\n')
         aclf.write('acl deny bob@QPID create queue name=qf5 filemaxsizeupperlimit=1000 filemaxcountupperlimit=100\n')
+        aclf.write('acl deny bob@QPID create queue name=ABCDE queuemaxsizelowerlimit=900000 queuemaxsizeupperlimit=1024000 queuemaxcountlowerlimit=900 queuemaxcountupperlimit=2000 filemaxsizelowerlimit=0 filemaxsizeupperlimit=32 filemaxcountlowerlimit=0 filemaxcountupperlimit=4 policytype=ring durable=false autodelete=true\n')
         aclf.write('acl allow all all')
         aclf.close()
 
@@ -985,6 +986,16 @@ class ACLTests(TestBase010):
 
         session = self.get_session('bob','bob')
 
+        self.Lookup("bob@QPID", "create", "queue", "ABCDE", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"0",
+                                                             "maxfilecount":"0" }, "deny")
+
         try:
             queue_options = {}
             queue_options["qpid.file_count"] = 200
@@ -1020,6 +1031,9 @@ class ACLTests(TestBase010):
         aclf.write('acl allow bob@QPID delete queue name=qfd4\n')
         aclf.write('acl allow bob@QPID create queue name=qfd5 filemaxsizeupperlimit=1000 filemaxcountupperlimit=100\n')
         aclf.write('acl allow bob@QPID create queue name=qfd6 filemaxsizelowerlimit=50 filemaxsizeupperlimit=100 filemaxcountlowerlimit=50 filemaxcountupperlimit=100\n')
+        aclf.write('acl allow bob@QPID create queue name=ABCDE queuemaxsizelowerlimit=900000 queuemaxsizeupperlimit=1024000 queuemaxcountlowerlimit=900 queuemaxcountupperlimit=2000 filemaxsizelowerlimit=0 filemaxsizeupperlimit=32 filemaxcountlowerlimit=0 filemaxcountupperlimit=4 policytype=ring durable=false autodelete=true\n')
+        aclf.write('acl allow bob@QPID create queue name=FGHIJ queuemaxsizelowerlimit=900000 queuemaxsizeupperlimit=1024000 queuemaxcountlowerlimit=900 queuemaxcountupperlimit=2000 filemaxsizelowerlimit=2 filemaxsizeupperlimit=32 filemaxcountlowerlimit=0 filemaxcountupperlimit=4 policytype=ring durable=false autodelete=true\n')
+        aclf.write('acl allow bob@QPID create queue name=KLMNO queuemaxsizelowerlimit=900000 queuemaxsizeupperlimit=1024000 queuemaxcountlowerlimit=900 queuemaxcountupperlimit=2000 filemaxsizelowerlimit=0 filemaxsizeupperlimit=0 filemaxcountlowerlimit=0 filemaxcountupperlimit=4 policytype=ring durable=false autodelete=true\n')
         aclf.write('acl allow anonymous all all\n')
         aclf.write('acl deny all all')
         aclf.close()
@@ -1030,6 +1044,86 @@ class ACLTests(TestBase010):
 
         session = self.get_session('bob','bob')
 
+        self.Lookup("bob@QPID", "create", "queue", "ABCDE", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"0",
+                                                             "maxfilecount":"0" }, "allow")
+
+        self.Lookup("bob@QPID", "create", "queue", "FGHIJ", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"1",
+                                                             "maxfilecount":"0" }, "deny")
+
+        self.Lookup("bob@QPID", "create", "queue", "FGHIJ", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"2",
+                                                             "maxfilecount":"0" }, "allow")
+
+        self.Lookup("bob@QPID", "create", "queue", "FGHIJ", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"32",
+                                                             "maxfilecount":"0" }, "allow")
+
+        self.Lookup("bob@QPID", "create", "queue", "FGHIJ", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"33",
+                                                             "maxfilecount":"0" }, "deny")
+
+        self.Lookup("bob@QPID", "create", "queue", "KLMNO", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"0",
+                                                             "maxfilecount":"0" }, "allow")
+
+        self.Lookup("bob@QPID", "create", "queue", "KLMNO", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"17",
+                                                             "maxfilecount":"0" }, "allow")
+
+        self.Lookup("bob@QPID", "create", "queue", "KLMNO", {"durable":"false",
+                                                             "autodelete":"true",
+                                                             "exclusive":"false",
+                                                             "alternate":"",
+                                                             "policytype":"ring",
+                                                             "maxqueuesize":"1024000",
+                                                             "maxqueuecount":"1000",
+                                                             "maxfilesize":"33",
+                                                             "maxfilecount":"0" }, "allow")
+
         try:
             session.queue_declare(queue="qfd1", durable=True)
         except qpid.session.SessionException, e:
@@ -1778,7 +1872,6 @@ class ACLTests(TestBase010):
 
         for u in g_admins:
             self.Lookup(u, "create", "queue", "anything", {"durable":"true"}, "allow-log")
-
         uInTest = g_auditors + g_admins
         uOutTest = self.AllBut(g_all, uInTest)
 
diff --git a/qpid/cpp/src/tests/run_acl_tests b/qpid/cpp/src/tests/run_acl_tests
index ebe4cf8bdb..d259f89255 100755
--- a/qpid/cpp/src/tests/run_acl_tests
+++ b/qpid/cpp/src/tests/run_acl_tests
@@ -29,7 +29,7 @@ DATA_DIRQ=`pwd`/data_dirq
 trap stop_brokers INT TERM QUIT
 
 start_brokers() {
-    ../qpidd --daemon --port 0 --no-module-dir --data-dir $DATA_DIR --load-module $ACL_LIB --acl-file policy.acl --auth no                               --log-to-file local.log > qpidd.port
+    ../qpidd --daemon --port 0 --no-module-dir --data-dir $DATA_DIR --load-module $ACL_LIB --acl-file policy.acl --auth no --log-enable trace+:acl  --log-to-file local.log > qpidd.port
     LOCAL_PORT=`cat qpidd.port`
     ../qpidd --daemon --port 0 --no-module-dir --data-dir $DATA_DIRI --load-module $ACL_LIB --acl-file policy.acl --auth no --connection-limit-per-ip 2   --log-to-file locali.log > qpiddi.port
     LOCAL_PORTI=`cat qpiddi.port`