This commit contains the following

1. Applying patches from Tim Platten attched to QPID-2062 and QPID-2063
2. Fixed QPID-2098
3. Added test cases for QPID-2062 and QPID-2063
4. Added more user friendly logging for ACL rule processing and lookup methods. In debug mode the logging should now provide more visibility into how rules are evaluated.


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

6 files changed, 327 insertions, 164 deletions

diff --git a/cpp/src/qpid/acl/AclData.cpp b/cpp/src/qpid/acl/AclData.cpp
index d2a55c0027..81519c3311 100644
--- a/cpp/src/qpid/acl/AclData.cpp
+++ b/cpp/src/qpid/acl/AclData.cpp
@@ -53,42 +53,65 @@ bool AclData::matchProp(const std::string & src, const std::string& src1)
 	}
 }
  
-AclResult AclData::lookup(const std::string& id, const Action& action, const ObjectType& objType, const std::string& name, std::map<Property, std::string>* params)
-{
-     AclResult aclresult = decisionMode;
-	
-	 if (actionList[action] && actionList[action][objType]){
-	      AclData::actObjItr itrRule = actionList[action][objType]->find(id);
-		  if (itrRule == actionList[action][objType]->end())
-		       itrRule = actionList[action][objType]->find("*");
-		  if (itrRule != actionList[action][objType]->end() ) {
-			   
-			   //loop the vector
-    		   for (ruleSetItr i=itrRule->second.begin(); i<itrRule->second.end(); i++) {
-                    
-					// loop the names looking for match
-					bool match =true;
-					for (propertyMapItr pMItr = i->props.begin(); (pMItr != i->props.end()) && match; pMItr++)
-					{
-                        //match name is exists first
-						if (pMItr->first == acl::PROP_NAME){
-						     if (!matchProp(pMItr->second, name)){  
-							     match= false;
-							 }
-						}else if (params){ //match pMItr against params
-							propertyMapItr paramItr = params->find (pMItr->first);
-							if (paramItr == params->end()){
-						    	match = false;
-							}else if (!matchProp(paramItr->second, pMItr->second)){  
-							    	match = false;
-							}
+AclResult AclData::lookup(const std::string& id, const Action& action, const ObjectType& objType,
+		const std::string& name, std::map<Property, std::string>* params) {
+
+	QPID_LOG(debug, "ACL: Lookup for id:" << id << " action:" << AclHelper::getActionStr((Action) action)
+			<< " objectType:" << AclHelper::getObjectTypeStr((ObjectType) objType) << " name:" << name
+			<< " with params " << AclHelper::propertyMapToString(params));
+
+	AclResult aclresult = decisionMode;
+	if (actionList[action] && actionList[action][objType]) {
+		AclData::actObjItr itrRule = actionList[action][objType]->find(id);
+		if (itrRule == actionList[action][objType]->end())
+			itrRule = actionList[action][objType]->find("*");
+		if (itrRule != actionList[action][objType]->end()) {
+
+			QPID_LOG(debug, "ACL: checking the following rules for : " << itrRule->first );
+
+			//loop the vector
+			for (ruleSetItr i = itrRule->second.begin(); i < itrRule->second.end(); i++) {
+				QPID_LOG(debug, "ACL: checking rule " <<  i->toString());
+				// loop the names looking for match
+				bool match = true;
+				for (propertyMapItr pMItr = i->props.begin(); (pMItr != i->props.end()) && match; pMItr++) {
+					//match name is exists first
+					if (pMItr->first == acl::PROP_NAME) {
+						if (matchProp(pMItr->second, name)){
+							QPID_LOG(debug, "ACL: name '" << name << "' matched with name '"
+																        << pMItr->second << "' given in the rule");
+					    }else{
+							match = false;
+							QPID_LOG(debug, "ACL: name '" << name << "' didn't match with name '"
+									        << pMItr->second << "' given in the rule");
+						}
+					} else if (params) { //match pMItr against params
+						propertyMapItr paramItr = params->find(pMItr->first);
+						if (paramItr == params->end()) {
+							match = false;
+							QPID_LOG(debug, "ACL: the given parameter map in lookup doesn't contain the property '"
+									        << AclHelper::getPropertyStr(pMItr->first) << "'");
+						} else if (!matchProp(pMItr->second, paramItr->second)) {
+							QPID_LOG(debug, "ACL: the pair("
+									        << AclHelper::getPropertyStr(paramItr->first) << "," << paramItr->second
+									        << ") given in lookup doesn't match the pair("
+									        << AclHelper::getPropertyStr(pMItr->first) << "," << pMItr->second << ") given in the rule");
+							match = false;
 						}
 					}
-					if (match) return getACLResult(i->logOnly, i->log);
-    		   }
-		  }
-	 }
-     return aclresult;
+				}
+				if (match)
+				{
+					aclresult = getACLResult(i->logOnly, i->log);
+					QPID_LOG(debug,"Successful match, the decision is:" << AclHelper::getAclResultStr(aclresult));
+					return aclresult;
+				}
+			}
+		}
+	}
+
+	QPID_LOG(debug,"No successful match, defaulting to the decision mode " << AclHelper::getAclResultStr(aclresult));
+	return aclresult;
 }
 
 AclResult AclData::lookup(const std::string& id, const Action& action, const ObjectType& objType, const std::string& /*Exchange*/ name, const std::string& RoutingKey)
diff --git a/cpp/src/qpid/acl/AclData.h b/cpp/src/qpid/acl/AclData.h
index 249c3523eb..a63afab12b 100644
--- a/cpp/src/qpid/acl/AclData.h
+++ b/cpp/src/qpid/acl/AclData.h
@@ -22,7 +22,7 @@
 
 #include "qpid/broker/AclModule.h"
 #include <vector>
-
+#include <sstream>
 
 namespace qpid {
 namespace acl {
@@ -45,6 +45,16 @@ public:
 	  
 	  
 	  rule (propertyMap& p):log(false),logOnly(false),props(p) {};
+
+	  std::string toString () const {
+	  	std::ostringstream ruleStr;
+	  	ruleStr << "[log=" << log << ", logOnly=" << logOnly << " props{";
+	  	for (propertyMapItr pMItr = props.begin(); pMItr != props.end(); pMItr++) {
+	  		ruleStr << " " << AclHelper::getPropertyStr((Property) pMItr-> first) << "=" << pMItr->second;
+	  	}
+	  	ruleStr << " }]";
+	  	return ruleStr.str();
+	  }
    };
    typedef  std::vector<rule> ruleSet;
    typedef  ruleSet::const_iterator ruleSetItr;
diff --git a/cpp/src/qpid/acl/AclReader.cpp b/cpp/src/qpid/acl/AclReader.cpp
index 8f5e4f5b57..8f419a6f50 100644
--- a/cpp/src/qpid/acl/AclReader.cpp
+++ b/cpp/src/qpid/acl/AclReader.cpp
@@ -83,115 +83,142 @@ std::string AclReader::aclRule::toString() {
     return oss.str();
 }
 
-void AclReader::loadDecisionData( boost::shared_ptr<AclData> d )
-{
-    d->clear();
-    QPID_LOG(debug, "ACL Load Rules");
-    int cnt = rules.size();
+void AclReader::loadDecisionData(boost::shared_ptr<AclData> d) {
+	d->clear();
+	QPID_LOG(debug, "ACL Load Rules");
+	int cnt = rules.size();
 	bool foundmode = false;
-    for (rlCitr i=rules.end()-1; cnt; i--,cnt--) {
-        QPID_LOG(debug, "ACL Processing " << std::setfill(' ') << std::setw(2) << cnt << " " << (*i)->toString());
-		
-		if (!foundmode && (*i)->actionAll && (*i)->names.size()==1 && (*((*i)->names.begin())).compare("*")==0 ){
-  	       d->decisionMode = (*i)->res;
-           QPID_LOG(debug, "ACL FoundMode " << AclHelper::getAclResultStr(d->decisionMode));
-		   foundmode=true;
-		}else{
-		    AclData::rule rule((*i)->props);
-		    bool addrule= true;
-		
-		    switch ((*i)->res)
-		    {
-		    case qpid::acl::ALLOWLOG:
-		        rule.log = true;
-                if (d->decisionMode == qpid::acl::ALLOW || d->decisionMode == qpid::acl::ALLOWLOG) 
-				    rule.logOnly = true;
+
+	for (rlCitr i = rules.end() - 1; cnt; i--, cnt--) {
+		QPID_LOG(debug, "ACL Processing " << std::setfill(' ') << std::setw(2)
+				<< cnt << " " << (*i)->toString());
+
+		if (!foundmode && (*i)->actionAll && (*i)->names.size() == 1
+				&& (*((*i)->names.begin())).compare("*") == 0) {
+			d->decisionMode = (*i)->res;
+			QPID_LOG(debug, "ACL FoundMode " << AclHelper::getAclResultStr(
+					d->decisionMode));
+			foundmode = true;
+		} else {
+			AclData::rule rule((*i)->props);
+			bool addrule = true;
+
+			switch ((*i)->res) {
+			case qpid::acl::ALLOWLOG:
+				rule.log = true;
+				if (d->decisionMode == qpid::acl::ALLOW || d->decisionMode
+						== qpid::acl::ALLOWLOG)
+					rule.logOnly = true;
+				break;
+			case qpid::acl::ALLOW:
+				if (d->decisionMode == qpid::acl::ALLOW || d->decisionMode
+						== qpid::acl::ALLOWLOG)
+					addrule = false;
+				break;
+			case qpid::acl::DENYLOG:
+				rule.log = true;
+				if (d->decisionMode == qpid::acl::DENY || d->decisionMode
+						== qpid::acl::DENYLOG)
+					rule.logOnly = true;
+				break;
+			case qpid::acl::DENY:
+				if (d->decisionMode == qpid::acl::DENY || d->decisionMode
+						== qpid::acl::DENYLOG)
+					addrule = false;
 				break;
-	    	case qpid::acl::ALLOW:
-                if (d->decisionMode == qpid::acl::ALLOW || d->decisionMode == qpid::acl::ALLOWLOG)
-				    addrule = false; 
-                break;
-		    case qpid::acl::DENYLOG:
-			    rule.log = true;
-                if (d->decisionMode == qpid::acl::DENY || d->decisionMode == qpid::acl::DENYLOG) 
-				    rule.logOnly = true;
-                break;
-		    case qpid::acl::DENY:
-                if (d->decisionMode == qpid::acl::DENY || d->decisionMode == qpid::acl::DENYLOG)
-				    addrule = false; 
-			    break;
-		    default:
-		        throw Exception("Invalid ACL Result loading rules.");
-		    }
-		    
-			
-            // Action -> Object -> map<user -> set<Rule> >
-            if (addrule){
-				for (int acnt= ((*i)->actionAll?0:(*i)->action); 
-        				 acnt< acl::ACTIONSIZE; (*i)->actionAll?acnt++:acnt=acl::ACTIONSIZE )  {
-
-            		if (acnt == acl::ACT_PUBLISH) d->transferAcl = true; // we have transfer ACL
-					
-					QPID_LOG(debug, "ACL Adding action:" << AclHelper::getActionStr((Action)acnt) );
-
-	        		//find the Action, create if not exist
-					if (d->actionList[acnt]==NULL) {
-			    		d->actionList[acnt] = new AclData::aclAction[qpid::acl::OBJECTSIZE];
-                		for (int j=0;j<qpid::acl::OBJECTSIZE; j++)
-                    		d->actionList[acnt][j] = NULL;
-            		}
+			default:
+				throw Exception("Invalid ACL Result loading rules.");
+			}
+
+			// Action -> Object -> map<user -> set<Rule> >
+			if (addrule) {
+				std::ostringstream actionstr;
+				for (int acnt = ((*i)->actionAll ? 0 : (*i)->action); acnt
+						< acl::ACTIONSIZE; (*i)->actionAll ? acnt++ : acnt
+						= acl::ACTIONSIZE) {
+
+					if (acnt == acl::ACT_PUBLISH)
+						d->transferAcl = true; // we have transfer ACL
+
+					actionstr << AclHelper::getActionStr((Action) acnt) << ",";
+
+					//find the Action, create if not exist
+					if (d->actionList[acnt] == NULL) {
+						d->actionList[acnt]
+								= new AclData::aclAction[qpid::acl::OBJECTSIZE];
+						for (int j = 0; j < qpid::acl::OBJECTSIZE; j++)
+							d->actionList[acnt][j] = NULL;
+					}
 
 					// optimize this loop to limit to valid options only!!
-					for (int ocnt= ((*i)->objStatus!=aclRule::VALUE ?0:(*i)->object); 
-        					 ocnt< acl::OBJECTSIZE; 
-							 (*i)->objStatus!=aclRule::VALUE?ocnt++:ocnt=acl::OBJECTSIZE )  {
-
-            				QPID_LOG(debug, "ACL Adding object:" << AclHelper::getObjectTypeStr((ObjectType)ocnt) );
-
-							//find the Object, create if not exist
-							if (d->actionList[acnt][ocnt] == NULL) 
-	            				d->actionList[acnt][ocnt] = new AclData::actionObject;
-
-            				// add users and Rule to object set
-							bool allNames=false;
-							// check to see if names.begin is '*'
-							if ( (*(*i)->names.begin()).compare("*")==0 ) allNames = true;
-
-							for (nsCitr itr = (allNames?names.begin():(*i)->names.begin());
-						        	  itr != (allNames?names.end():(*i)->names.end()); itr++) {
-							   AclData::actObjItr itrRule = d->actionList[acnt][ocnt]->find(*itr);
-							   if (itrRule == d->actionList[acnt][ocnt]->end()) {
-                				   QPID_LOG(debug, "ACL Adding rule & user:" << *itr);
-			    				   AclData::ruleSet rSet;
-			    				   rSet.push_back(rule);
-                				   d->actionList[acnt][ocnt]->insert(make_pair( std::string(*itr) , rSet) );
-            				   }else{
-							   
-							   // TODO add code to check for dead rules
-							   // allow peter create queue name=tmp <-- dead rule!!
-							   // allow peter create queue
-							   
-			   					   itrRule->second.push_back(rule);
-                				   QPID_LOG(debug, "ACL Adding rule to user:" << *itr);
-							   }
-            				}
-
-	        		}
-
-        	   }
-		   }else{
-            	QPID_LOG(debug, "ACL Skipping based on Mode:" << AclHelper::getAclResultStr(d->decisionMode) );
-		   }
-      }		
-		
-    }
+					for (int ocnt = ((*i)->objStatus != aclRule::VALUE ? 0
+							: (*i)->object); ocnt < acl::OBJECTSIZE; (*i)->objStatus
+							!= aclRule::VALUE ? ocnt++ : ocnt = acl::OBJECTSIZE) {
+
+						//find the Object, create if not exist
+						if (d->actionList[acnt][ocnt] == NULL)
+							d->actionList[acnt][ocnt]
+									= new AclData::actionObject;
+
+						// add users and Rule to object set
+						bool allNames = false;
+						// check to see if names.begin is '*'
+						if ((*(*i)->names.begin()).compare("*") == 0)
+							allNames = true;
+
+						for (nsCitr itr = (allNames ? names.begin()
+								: (*i)->names.begin()); itr
+								!= (allNames ? names.end() : (*i)->names.end()); itr++) {
+
+							AclData::actObjItr itrRule =
+									d->actionList[acnt][ocnt]->find(*itr);
+
+							if (itrRule == d->actionList[acnt][ocnt]->end()) {
+								AclData::ruleSet rSet;
+								rSet.push_back(rule);
+								d->actionList[acnt][ocnt]->insert(make_pair(
+										std::string(*itr), rSet));
+							} else {
+
+								// TODO add code to check for dead rules
+								// allow peter create queue name=tmp <-- dead rule!!
+								// allow peter create queue
+
+								itrRule->second.push_back(rule);
+							}
+						}
+
+					}
+				}
+
+				std::ostringstream objstr;
+				for (int ocnt = ((*i)->objStatus != aclRule::VALUE ? 0 : (*i)->object); ocnt < acl::OBJECTSIZE;
+					 (*i)->objStatus != aclRule::VALUE ? ocnt++ : ocnt = acl::OBJECTSIZE) {
+										objstr << AclHelper::getObjectTypeStr((ObjectType) ocnt) << ",";
+				}
+
+				bool allNames = ((*(*i)->names.begin()).compare("*") == 0);
+				std::ostringstream userstr;
+				for (nsCitr itr = (allNames ? names.begin() : (*i)->names.begin());
+				     itr != (allNames ? names.end() : (*i)->names.end()); itr++) {
+											userstr << *itr << ",";
+				}
+
+				QPID_LOG(debug,"ACL: Adding actions {" << actionstr.str().substr(0,actionstr.str().length()-1)
+						       << "} to objects {" << objstr.str().substr(0,objstr.str().length()-1)
+						       << "} with props " << AclHelper::propertyMapToString(&rule.props)
+						       << " for users {" << userstr.str().substr(0,userstr.str().length()-1) << "}" );
+			} else {
+				QPID_LOG(debug, "ACL Skipping based on Mode:"
+						<< AclHelper::getAclResultStr(d->decisionMode));
+			}
+		}
 
+	}
 
 }
 
 
-
-
 void AclReader::aclRule::processName(const std::string& name, const groupMap& groups) {
     if (name.compare("all") == 0) {
         names.insert("*");
diff --git a/cpp/src/qpid/broker/AclModule.h b/cpp/src/qpid/broker/AclModule.h
index a78b2d5b4a..536fa21b2b 100644
--- a/cpp/src/qpid/broker/AclModule.h
+++ b/cpp/src/qpid/broker/AclModule.h
@@ -26,7 +26,7 @@
 #include <map>
 #include <set>
 #include <string>
-
+#include <sstream>
 
 namespace qpid {
 
@@ -179,6 +179,8 @@ class AclHelper {
     typedef std::map<ObjectType, actionMapPtr> objectMap;
     typedef objectMap::const_iterator omCitr;
     typedef boost::shared_ptr<objectMap> objectMapPtr;
+    typedef std::map<Property, std::string> propMap;
+    typedef propMap::const_iterator propMapItr;
 
     // This map contains the legal combinations of object/action/properties found in an ACL file
     static void loadValidationMap(objectMapPtr& map) {
@@ -248,6 +250,19 @@ class AclHelper {
 
         map->insert(objectPair(OBJ_METHOD, a4));
     }
+
+    static std::string propertyMapToString(const std::map<Property, std::string>* params) {
+    	std::ostringstream ss;
+	ss << "{";
+	if (params)
+	{
+		for (propMapItr pMItr = params->begin(); pMItr != params->end(); pMItr++) {
+			ss << " " << getPropertyStr((Property) pMItr-> first) << "=" << pMItr->second;
+		}
+	}
+	ss << " }";
+	return ss.str();
+    }
 };
 
     
diff --git a/cpp/src/qpid/broker/SessionAdapter.cpp b/cpp/src/qpid/broker/SessionAdapter.cpp
index af07605552..a1ad5a0a30 100644
--- a/cpp/src/qpid/broker/SessionAdapter.cpp
+++ b/cpp/src/qpid/broker/SessionAdapter.cpp
@@ -72,7 +72,7 @@ void SessionAdapter::ExchangeHandlerImpl::declare(const string& exchange, const
         params.insert(make_pair(acl::PROP_PASSIVE, std::string(passive ? _TRUE : _FALSE) ));
         params.insert(make_pair(acl::PROP_DURABLE, std::string(durable ? _TRUE : _FALSE)));
         if (!acl->authorise(getConnection().getUserId(),acl::ACT_CREATE,acl::OBJ_EXCHANGE,exchange,&params) )
-            throw NotAllowedException(QPID_MSG("ACL denied exhange declare request from " << getConnection().getUserId()));
+            throw NotAllowedException(QPID_MSG("ACL denied exchange declare request from " << getConnection().getUserId()));
     }
     
     //TODO: implement autoDelete
@@ -134,7 +134,7 @@ void SessionAdapter::ExchangeHandlerImpl::delete_(const string& name, bool /*ifU
     AclModule* acl = getBroker().getAcl();
     if (acl) {
         if (!acl->authorise(getConnection().getUserId(),acl::ACT_DELETE,acl::OBJ_EXCHANGE,name,NULL) )
-            throw NotAllowedException(QPID_MSG("ACL denied exhange delete request from " << getConnection().getUserId()));
+            throw NotAllowedException(QPID_MSG("ACL denied exchange delete request from " << getConnection().getUserId()));
     }
 
     //TODO: implement unused
@@ -154,7 +154,7 @@ ExchangeQueryResult SessionAdapter::ExchangeHandlerImpl::query(const string& nam
     AclModule* acl = getBroker().getAcl();
     if (acl) {
         if (!acl->authorise(getConnection().getUserId(),acl::ACT_ACCESS,acl::OBJ_EXCHANGE,name,NULL) )
-            throw NotAllowedException(QPID_MSG("ACL denied exhange query request from " << getConnection().getUserId()));
+            throw NotAllowedException(QPID_MSG("ACL denied exchange query request from " << getConnection().getUserId()));
     }
 
     try {
@@ -171,8 +171,12 @@ void SessionAdapter::ExchangeHandlerImpl::bind(const string& queueName,
 {
     AclModule* acl = getBroker().getAcl();
     if (acl) {
-        if (!acl->authorise(getConnection().getUserId(),acl::ACT_BIND,acl::OBJ_EXCHANGE,exchangeName,routingKey) )
-            throw NotAllowedException(QPID_MSG("ACL denied exhange bind request from " << getConnection().getUserId()));
+        std::map<acl::Property, std::string> params;
+        params.insert(make_pair(acl::PROP_QUEUENAME, queueName));
+        params.insert(make_pair(acl::PROP_ROUTINGKEY, routingKey));
+
+        if (!acl->authorise(getConnection().getUserId(),acl::ACT_BIND,acl::OBJ_EXCHANGE,exchangeName,&params))
+            throw NotAllowedException(QPID_MSG("ACL denied exchange bind request from " << getConnection().getUserId()));
     }
 
     Queue::shared_ptr queue = getQueue(queueName);
@@ -234,8 +238,8 @@ ExchangeBoundResult SessionAdapter::ExchangeHandlerImpl::bound(const std::string
         std::map<acl::Property, std::string> params;
         params.insert(make_pair(acl::PROP_QUEUENAME, queueName));
         params.insert(make_pair(acl::PROP_ROUTINGKEY, key));
-        if (!acl->authorise(getConnection().getUserId(),acl::ACT_CREATE,acl::OBJ_EXCHANGE,exchangeName,&params) )
-            throw NotAllowedException(QPID_MSG("ACL denied exhange bound request from " << getConnection().getUserId()));
+        if (!acl->authorise(getConnection().getUserId(),acl::ACT_ACCESS,acl::OBJ_EXCHANGE,exchangeName,&params) )
+            throw NotAllowedException(QPID_MSG("ACL denied exchange bound request from " << getConnection().getUserId()));
     }
     
     Exchange::shared_ptr exchange;
diff --git a/cpp/src/tests/acl.py b/cpp/src/tests/acl.py
index b62288a769..f6cd1b2669 100755
--- a/cpp/src/tests/acl.py
+++ b/cpp/src/tests/acl.py
@@ -208,9 +208,9 @@ class ACLTests(TestBase010):
    # ACL queue tests
    #=====================================
            
-    def test_queue_acl(self):
+    def test_queue_acl_deny(self):
         """
-        Test various modes for queue acl
+        Test cases for queue acl in allow mode
         """
         aclf = ACLFile()
         aclf.write('acl deny bob@QPID create queue name=q1 durable=true passive=true\n')
@@ -240,6 +240,12 @@ class ACLTests(TestBase010):
             session = self.get_session('bob','bob')
         
         try:
+            session.queue_declare(queue="q2", durable='true')            
+        except qpid.session.SessionException, e:
+            if (530 == e.args[0].error_code):
+                self.fail("ACL should allow queue create request for q2 with any parameter other than exclusive");
+
+        try:
             session.queue_declare(queue="q3", exclusive='true')
             session.queue_declare(queue="q4", durable='true')
         except qpid.session.SessionException, e:
@@ -283,9 +289,12 @@ class ACLTests(TestBase010):
    # ACL exchange tests
    #=====================================
    
-    def test_exchange_acl(self):
+    def test_exchange_acl_deny(self):
+        session = self.get_session('bob','bob')        
+        session.queue_declare(queue="baz")
+
         """
-        Test various modes for exchange acl
+        Test cases for exchange acl in allow mode
         """
         aclf = ACLFile()
         aclf.write('acl deny bob@QPID create exchange name=testEx durable=true passive=true\n')
@@ -293,38 +302,47 @@ class ACLTests(TestBase010):
         aclf.write('acl deny bob@QPID access exchange name=myEx\n')
         aclf.write('acl deny bob@QPID bind exchange name=myEx queuename=q1 routingkey=rk1\n')
         aclf.write('acl deny bob@QPID unbind exchange name=myEx queuename=q1 routingkey=rk1\n')
-        aclf.write('acl deny bob@QPID delete exchange name=myEx\n')                
+        aclf.write('acl deny bob@QPID delete exchange name=myEx\n')
         aclf.write('acl allow all all')
         aclf.close()        
         
         self.reload_acl()       
         
         session = self.get_session('bob','bob')
-        
+        session.queue_declare(queue='q1')
+        session.queue_declare(queue='q2')
+        session.exchange_declare(exchange='myEx', type='direct')
+
         try:
-            session.exchange_declare(exchange='testEx', durable='true', passive='true')
+            session.exchange_declare(exchange='testEx', durable=True, passive=True)
             self.fail("ACL should deny exchange create request with name=testEx durable=true passive=true");
         except qpid.session.SessionException, e:
             self.assertEqual(530,e.args[0].error_code)
             session = self.get_session('bob','bob')
        
         try:
+            session.exchange_declare(exchange='testEx', type='direct', durable=True, passive=False)
+        except qpid.session.SessionException, e:
+            print e
+            if (530 == e.args[0].error_code):
+                self.fail("ACL should allow exchange create request for testEx with any parameter other than durable=true and passive=true");
+                        
+        try:
             session.exchange_declare(exchange='ex1', type='direct')
             self.fail("ACL should deny exchange create request with name=ex1 type=direct");
-        except qpid.session.SessionException, e:
+        except qpid.session.SessionException, e:    
             self.assertEqual(530,e.args[0].error_code) 
             session = self.get_session('bob','bob')
         
         try:
             session.exchange_declare(exchange='myXml', type='direct')
-            session.queue_declare(queue='q1')
         except qpid.session.SessionException, e:
             if (530 == e.args[0].error_code):
                 self.fail("ACL should allow exchange create request for myXml with any parameter");
 
         try:
             session.exchange_query(name='myEx')
-            self.fail("ACL should deny queue query request for q3");
+            self.fail("ACL should deny exchange query request for myEx");
         except qpid.session.SessionException, e:
             self.assertEqual(530,e.args[0].error_code)
             session = self.get_session('bob','bob')
@@ -337,10 +355,18 @@ class ACLTests(TestBase010):
             session = self.get_session('bob','bob')
 
         try:
-            session.exchange_bind(exchange='myXml', queue='q1', binding_key='x')
+            session.exchange_bind(exchange='myEx', queue='q1', binding_key='x')
+        except qpid.session.SessionException, e:
+            print e
+            if (530 == e.args[0].error_code):
+                self.fail("ACL should allow exchange bind request for exchange='myEx', queue='q1', binding_key='x'");
+
+        try:
+            session.exchange_bind(exchange='myEx', queue='q2', binding_key='rk1')
         except qpid.session.SessionException, e:
             if (530 == e.args[0].error_code):
-                self.fail("ACL should allow exchange bind request for exchange='myXml', queue='q1', binding_key='x'");
+                self.fail("ACL should allow exchange bind request for exchange='myEx', queue='q2', binding_key='rk1'");
+
         try:
             session.exchange_unbind(exchange='myEx', queue='q1', binding_key='rk1')
             self.fail("ACL should deny exchange unbind request with exchange='myEx' queuename='q1' bindingkey='rk1'");
@@ -349,10 +375,16 @@ class ACLTests(TestBase010):
             session = self.get_session('bob','bob')
 
         try:
-            session.exchange_unbind(exchange='myXml', queue='q1', binding_key='x')
+            session.exchange_unbind(exchange='myEx', queue='q1', binding_key='x')
+        except qpid.session.SessionException, e:
+            if (530 == e.args[0].error_code):
+                self.fail("ACL should allow exchange unbind request for exchange='myEx', queue='q1', binding_key='x'");
+
+        try:
+            session.exchange_unbind(exchange='myEx', queue='q2', binding_key='rk1')
         except qpid.session.SessionException, e:
             if (530 == e.args[0].error_code):
-                self.fail("ACL should allow exchange unbind request for exchange='myXml', queue='q1', binding_key='x'");
+                self.fail("ACL should allow exchange unbind request for exchange='myEx', queue='q2', binding_key='rk1'");
                    
         try:
             session.exchange_delete(exchange='myEx')
@@ -366,8 +398,66 @@ class ACLTests(TestBase010):
         except qpid.session.SessionException, e:
             if (530 == e.args[0].error_code):
                 self.fail("ACL should allow exchange delete request for myXml");
-                        
+        
+
+    def test_exchange_acl_allow(self):
+        session = self.get_session('bob','bob')
+        session.queue_declare(queue='bar')
+
+        """
+        Test cases for exchange acl in deny mode
+        """
+        aclf = ACLFile()
+        aclf.write('acl allow bob@QPID bind exchange name=amq.topic queuename=bar routingkey=foo.*\n') 
+        aclf.write('acl allow bob@QPID unbind exchange name=amq.topic queuename=bar routingkey=foo.*\n')
+        aclf.write('acl allow guest@QPID all all\n') 
+        aclf.write('acl deny all all')
+        aclf.close()        
+        
+        self.reload_acl()       
+        
+        session = self.get_session('bob','bob')
            
+        try:
+            session.exchange_bind(exchange='amq.topic', queue='bar', binding_key='foo.bar')
+        except qpid.session.SessionException, e:
+            if (530 == e.args[0].error_code):
+                self.fail("ACL should allow exchange bind request for exchange='amq.topic', queue='bar', binding_key='foor.bar'");
+
+        try:
+            session.exchange_bind(exchange='amq.topic', queue='baz', binding_key='foo.bar')
+            self.fail("ACL should deny exchange bind request for exchange='amq.topic', queue='baz', binding_key='foo.bar'");
+        except qpid.session.SessionException, e:
+            self.assertEqual(530,e.args[0].error_code)
+            session = self.get_session('bob','bob')
+
+        try:
+            session.exchange_bind(exchange='amq.topic', queue='bar', binding_key='fooz.bar')
+            self.fail("ACL should deny exchange bind request for exchange='amq.topic', queue='bar', binding_key='fooz.bar'");
+        except qpid.session.SessionException, e:
+            self.assertEqual(530,e.args[0].error_code)
+            session = self.get_session('bob','bob')
+
+
+        try:
+            session.exchange_unbind(exchange='amq.topic', queue='bar', binding_key='foo.bar')
+        except qpid.session.SessionException, e:
+            if (530 == e.args[0].error_code):
+                self.fail("ACL should allow exchange unbind request for exchange='amq.topic', queue='bar', binding_key='foor.bar'");
+        try:
+            session.exchange_unbind(exchange='amq.topic', queue='baz', binding_key='foo.bar')
+            self.fail("ACL should deny exchange unbind request for exchange='amq.topic', queue='baz', binding_key='foo.bar'");
+        except qpid.session.SessionException, e:
+            self.assertEqual(530,e.args[0].error_code)
+            session = self.get_session('bob','bob')
+
+        try:
+            session.exchange_unbind(exchange='amq.topic', queue='bar', binding_key='fooz.bar')
+            self.fail("ACL should deny exchange unbind request for exchange='amq.topic', queue='bar', binding_key='fooz.bar'");
+        except qpid.session.SessionException, e:
+            self.assertEqual(530,e.args[0].error_code)
+            session = self.get_session('bob','bob')
+
    #=====================================
    # ACL consume tests
    #=====================================
@@ -434,12 +524,6 @@ class ACLTests(TestBase010):
         self.reload_acl()       
         
         session = self.get_session('bob','bob')
-        
-        try:
-            session.exchange_declare(exchange='myEx', type='topic')
-        except qpid.session.SessionException, e:
-            if (530 == e.args[0].error_code):
-                self.fail("ACL should allow exchange create request for myEx with any parameter");
             
         props = session.delivery_properties(routing_key="rk1")