Preliminary patch being tested:

$ svn diff .
Index: src/qpid/broker/Broker.cpp
===================================================================
--- src/qpid/broker/Broker.cpp	(revision 1569293)
+++ src/qpid/broker/Broker.cpp	(working copy)
@@ -1306,13 +1306,15 @@
         params.insert(make_pair(acl::PROP_AUTODELETE, settings.autodelete ? _TRUE : _FALSE));
         params.insert(make_pair(acl::PROP_POLICYTYPE, settings.getLimitPolicy()));
         params.insert(make_pair(acl::PROP_PAGING, settings.paging ? _TRUE : _FALSE));
-        params.insert(make_pair(acl::PROP_MAXPAGES, boost::lexical_cast<string>(settings.maxPages)));
-        params.insert(make_pair(acl::PROP_MAXPAGEFACTOR, boost::lexical_cast<string>(settings.pageFactor)));
-        params.insert(make_pair(acl::PROP_MAXQUEUECOUNT, boost::lexical_cast<string>(settings.maxDepth.getCount())));
-        params.insert(make_pair(acl::PROP_MAXQUEUESIZE, boost::lexical_cast<string>(settings.maxDepth.getSize())));
-        params.insert(make_pair(acl::PROP_MAXFILECOUNT, boost::lexical_cast<string>(settings.maxFileCount)));
-        params.insert(make_pair(acl::PROP_MAXFILESIZE, boost::lexical_cast<string>(settings.maxFileSize)));
-
+        params.insert(make_pair(acl::PROP_MAXPAGES, boost::lexical_cast<string>(settings.maxPages ? settings.maxPages : DEFAULT_MAX_PAGES)));
+        params.insert(make_pair(acl::PROP_MAXPAGEFACTOR, boost::lexical_cast<string>(settings.pageFactor ? settings.pageFactor : DEFAULT_PAGE_FACTOR)));
+        if (settings.maxDepth.hasCount())
+            params.insert(make_pair(acl::PROP_MAXQUEUECOUNT, boost::lexical_cast<string>(settings.maxDepth.getCount())));
+        params.insert(make_pair(acl::PROP_MAXQUEUESIZE, boost::lexical_cast<string>(settings.maxDepth.hasSize() ? settings.maxDepth.getSize() : config.queueLimit)));
+        if (settings.durable) {
+            params.insert(make_pair(acl::PROP_MAXFILECOUNT, boost::lexical_cast<string>(settings.maxFileCount ? settings.maxFileCount : 8)));
+            params.insert(make_pair(acl::PROP_MAXFILESIZE, boost::lexical_cast<string>(settings.maxFileSize ? settings.maxFileSize : 24)));
+        }
         if (!acl->authorise(userId,acl::ACT_CREATE,acl::OBJ_QUEUE,name,&params) )
             throw framing::UnauthorizedAccessException(QPID_MSG("ACL denied queue create request from " << userId));
 
Index: src/qpid/broker/QueueFactory.cpp
===================================================================
--- src/qpid/broker/QueueFactory.cpp	(revision 1569293)
+++ src/qpid/broker/QueueFactory.cpp	(working copy)
@@ -80,8 +80,8 @@
             QPID_LOG(warning, "Cannot create paged queue; no paging directory enabled");
         } else {
             queue->messages = std::auto_ptr<Messages>(new PagedQueue(name, broker->getPagingDir().getPath(),
-                                                                     settings.maxPages ? settings.maxPages : 4,
-                                                                     settings.pageFactor ? settings.pageFactor : 1,
+                                                                     settings.maxPages ? settings.maxPages : DEFAULT_MAX_PAGES,
+                                                                     settings.pageFactor ? settings.pageFactor : DEFAULT_PAGE_FACTOR,
                                                                      broker->getProtocolRegistry(), broker->getExpiryPolicy()));
         }
     } else if (settings.lvqKey.empty()) {//LVQ already handled above
Index: src/qpid/broker/QueueFactory.h
===================================================================
--- src/qpid/broker/QueueFactory.h	(revision 1569293)
+++ src/qpid/broker/QueueFactory.h	(working copy)
@@ -25,6 +25,9 @@
 #include "qpid/types/Variant.h"
 #include <boost/shared_ptr.hpp>
 
+#define DEFAULT_MAX_PAGES 4
+#define DEFAULT_PAGE_FACTOR 1
+
 namespace qpid {
 namespace management {
 class Manageable;
$

Patch in Comment 1 is changing the broker logic during the creation of queues. The current (failing) method sends a zero to ACL for testing when the user does not specify a value. The patched method sends the default value (that will be used when the user specifies none) to ACL for testing.

+1 Ship It!

(In reply to Chuck Rolke from comment #2)
> Patch in Comment 1 is changing the broker logic during the creation of
> queues. The current (failing) method sends a zero to ACL for testing when
> the user does not specify a value. The patched method sends the default
> value (that will be used when the user specifies none) to ACL for testing.
> 
> +1 Ship It!

Thanks for review, I originally planned to ask for in in upstream, but why not here (as I suppose you will be the only one interested atm). Still I have two doubts here, as even my patch is ideal:

1) (minor issue) "if (settings.durable) {" test: 
this would "break" ACL rule like:

acl allow all create queue filemaxcountupperlimit=16

where obviously "durable=true" misses there. So the ACL line above prevents creating transient queues at all (it allows queues with maxFileSize<=16 but that is taken into account only for durable queues). On the other side, the above rule can be seen as invalid, as filemaxcountupperlimit pre-assumes durable=true that misses there.

Also, I don't like copy&pasting constants (like "static const u_int32_t defJrnlFileSizePgs = 24;" -> "settings.maxFileSize ? settings.maxFileSize : 24" but dont see an easy way of getting the constant value from legacystore (on the other hand, I suppose no change in the values so I am fine with it)


2) (bigger issue) "if (settings.maxDepth.hasCount())" test:
Assume ACL:

acl allow all create queue queuemaxcountlowerlimit=10

that should pass default queue creation with no limits to queue depth imposed. But ACL does not match:

2014-02-19 09:38:15 [Security] debug ACL: checking rule [rule 1 ruleMode = allow props{ queuemaxcountlowerlimit=10 }]
2014-02-19 09:38:15 [Security] debug ACL: lookup parameter map doesn't contain the rule property 'queuemaxcountlowerlimit'
2014-02-19 09:38:15 [Security] debug ACL: No successful match, defaulting to the decision mode deny

(This is very related to 0 standing for unlimited value)


3) (modification): add equivalent test to durability: 

+        if (settings.paging) {
+            params.insert(make_pair(acl::PROP_MAXPAGES, boost::lexical_cast<string>(settings.maxPages ? settings.maxPages : DEFAULT_MAX_PAGES)));
+            params.insert(make_pair(acl::PROP_MAXPAGEFACTOR, boost::lexical_cast<string>(settings.pageFactor ? settings.pageFactor : DEFAULT_PAGE_FACTOR)));
+        }


I am about to post it to upsetram(*) and file bug "ACL should treat zero as unlimitted value".

(*) once automated tests finish, as in upstream they fail even without mine patch

These rules are complicated and figuring out a rule's implication is difficult: My code does X -> the client library sends Y on the wire to the broker -> the broker sends Z to ACL for lookup.

Ok, don't ship it yet.

---
In issue 1) the rule
  acl allow all create queue filemaxcountupperlimit=16

could be fixed with
  acl allow all create queue filemaxcountupperlimit=16 durable=true

That doesn't seem like such a problem as long as some examples and guidance are provided.
---
In issue 1) yes, copying and pasting constants is bad. 

I suggest that we may be able to avoid using the constants at all by introducing the concept of the ACL limit rule value"default". Then you can specify a rule that says what to do when the user didn't specify the value. Back in the broker the code would not send the default value to ACL (as proposed in your patch) but send the zero if the value was not specified (or if the user actually specified zero). Essentially this means don't change broker lookup code but add new code to ACL lookups.

Applied to issue 2) you could have ACL

  acl allow all create queue queuemaxcountlowerlimit=default
  acl allow all create queue queuemaxcountlowerlimit=10

The rule using 'default' would match if the item (queuemaxcountlowerlimit) is absent in the lookup or present in the lookup but with a value of zero. Together these rules specify that if no queuemaxcountlowerlimit was specified by the user then allow; failing that if the user specifies a queuemaxcountlower limit then the value must be >= 10.

The appeal to the 'default' rule value is that you don't have to discover or make up the default values in broker code. It gives ACL a way to have explicit rules governing the default case.

---
Issue 3) is also solved by the 'default' rule addition and by the requirement to have "paging=true" in the rule before the rule applies.

(In reply to Chuck Rolke from comment #4)
> My code does X -> the client library sends Y on the wire to the broker -> the
> broker sends Z to ACL for lookup.
The corret flow is rather "my code does X that is sent over wire -> broker interprets it as Y -> ACL understands it as Z (0=0 but not unlimited as now)", no?

> I suggest that we may be able to avoid using the constants at all by
> introducing the concept of the ACL limit rule value"default". Then you can
> specify a rule that says what to do when the user didn't specify the value.
> Back in the broker the code would not send the default value to ACL (as
> proposed in your patch) but send the zero if the value was not specified (or
> if the user actually specified zero). Essentially this means don't change
> broker lookup code but add new code to ACL lookups.
So, if a user does not specify a property, then the broker will not set the ACL property and ACL will assume it is "default" value? Assume ACL:

acl allow all queue create queuemaxsizeupperlimit=104857600
(the number is the value of --default-queue-limit broker parameter)

Will that ACL rule be met in these situations?
a) user does not specify qpid.max_size queue declare option (such that broker will set 104857600 there)
b) user sets qpid.max_size=104857600

Is it ok if a) is not matched while b) does? Would not it be confusing?

Note the only default constant values (that would have to be specified on 2nd place in source code) are only related to legacy store. I will think about the way passing the legacy store constants there..


> 
> Applied to issue 2) you could have ACL
> 
>   acl allow all create queue queuemaxcountlowerlimit=default
>   acl allow all create queue queuemaxcountlowerlimit=10

Then explicitly set zero value (that is interpreted by broker as infinity) would be interpreted by ACL as "default", what is wrong. Esp. for qpid.max_size with default value 100MB.

Moreover even approach with "default" ACL value does not solve the problem how to distinguish cases when user did not specify a property (and default 0 is set in QueueSettings) and when user set it to zero explicitly. In the first case, real default value should be used, in the second, zero/infinity value should be used. But that is QueueSettings / QueueDepth class limitation.

FYI I have a solution for the "zero means infinite / no limit" issue. It is already resolved for max_size / max_count (see struct Optional in src/qpid/broker/QueueDepth.h).

(In reply to Pavel Moravec from comment #6)
> FYI I have a solution for the "zero means infinite / no limit" issue. It is
> already resolved for max_size / max_count (see struct Optional in
> src/qpid/broker/QueueDepth.h).

https://reviews.apache.org/r/18423/

Committed to upstream as r1572059.

Retested on qpid-cpp-

The ACL rules are tested against the directly specified parameters by the user or the default broker values.

Verified on qpid-cpp-0.30-7 on Rhel6-i686 and x86_64 and Rhel7-x86_64.

--> VERIFIED

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHEA-2015-0805.html