QPID-1301 Fixes for issues with temporary queue permissions, including promotion of temporary element out from individual queue elements.

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

3 files changed, 298 insertions, 223 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java
index 6fe4696d20..6d35d7ae48 100755
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java
@@ -47,7 +47,6 @@ public class PrincipalPermissions
     private static final Object CREATE_QUEUE_QUEUES_KEY = new Object();
     private static final Object CREATE_QUEUE_EXCHANGES_KEY = new Object();
 
-    private static final Object CREATE_QUEUE_EXCHANGES_TEMPORARY_KEY = new Object();
     private static final Object CREATE_QUEUE_EXCHANGES_ROUTINGKEYS_KEY = new Object();
 
     private static final int PUBLISH_EXCHANGES_KEY = 0;
@@ -181,165 +180,168 @@ public class PrincipalPermissions
 		rights.put(name, className);
 	}
 
-	private void grantCreateQueue(Permission permission, Object... parameters) {
-		Map createRights = (Map) _permissions.get(permission);
-
-		if (createRights == null)
-		{
-		    createRights = new ConcurrentHashMap();
-		    _permissions.put(permission, createRights);
-
-		}
-
-		//The existence of the empty map mean permission to all.
-		if (parameters.length == 0)
-		{
-		    return;
-		}
-
-		Boolean temporary = (Boolean) parameters[0];
-
-		AMQShortString queueName = parameters.length > 1 ? (AMQShortString) parameters[1] : null;
-		AMQShortString exchangeName = parameters.length > 2 ? (AMQShortString) parameters[2] : null;
-		//Set the routingkey to the specified value or the queueName if present
-		AMQShortString routingKey = (parameters.length > 3 && null != parameters[3]) ? (AMQShortString) parameters[3] : queueName;
-
-		// Get the queues map
-		Map create_queues = (Map) createRights.get(CREATE_QUEUES_KEY);
-
-		if (create_queues == null)
-		{
-		    create_queues = new ConcurrentHashMap();
-		    createRights.put(CREATE_QUEUES_KEY, create_queues);
-		}
-
-		//Allow all temp queues to be created
-		create_queues.put(CREATE_QUEUE_TEMPORARY_KEY, temporary);
-
-		//Create empty list of queues
-		Map create_queues_queues = (Map) create_queues.get(CREATE_QUEUE_QUEUES_KEY);
-
-		if (create_queues_queues == null)
-		{
-		    create_queues_queues = new ConcurrentHashMap();
-		    create_queues.put(CREATE_QUEUE_QUEUES_KEY, create_queues_queues);
-		}
-
-		// We are granting CREATE rights to all temporary queues only
-		if (parameters.length == 1)
-		{
-		    return;
-		}
+	private void grantCreateQueue(Permission permission, Object... parameters)
+    {
+        Map createRights = (Map) _permissions.get(permission);
 
-		// if we have a queueName then we need to store any associated exchange / rk bindings
-		if (queueName != null)
-		{
-		    Map queue = (Map) create_queues_queues.get(queueName);
-		    if (queue == null)
-		    {
-		        queue = new ConcurrentHashMap();
-		        create_queues_queues.put(queueName, queue);
-		    }
+        if (createRights == null)
+        {
+            createRights = new ConcurrentHashMap();
+            _permissions.put(permission, createRights);
+        }
 
-		    if (exchangeName != null)
-		    {
-		        queue.put(exchangeName, routingKey);
-		    }
+        //The existence of the empty map mean permission to all.
+        if (parameters.length == 0)
+        {
+            return;
+        }
 
-		    //If no exchange is specified then the presence of the queueName in the map says any exchange is ok
-		}
+        // Get the queues map
+        Map create_queues = (Map) createRights.get(CREATE_QUEUES_KEY);
 
-		// Store the exchange that we are being granted rights to. This will be used as part of binding
+        //Initialiase the queue permissions if not already done
+        if (create_queues == null)
+        {
+            create_queues = new ConcurrentHashMap();
+            //initialise temp queue permission to false and overwrite below if true
+            create_queues.put(CREATE_QUEUE_TEMPORARY_KEY, false);
+            createRights.put(CREATE_QUEUES_KEY, create_queues);
+        }
 
-		//Lookup the list of exchanges
-		Map create_queues_exchanges = (Map) create_queues.get(CREATE_QUEUE_EXCHANGES_KEY);
+         //Create empty list of queues
+        Map create_queues_queues = (Map) create_queues.get(CREATE_QUEUE_QUEUES_KEY);
 
-		if (create_queues_exchanges == null)
-		{
-		    create_queues_exchanges = new ConcurrentHashMap();
-		    create_queues.put(CREATE_QUEUE_EXCHANGES_KEY, create_queues_exchanges);
-		}
+        if (create_queues_queues == null)
+        {
+            create_queues_queues = new ConcurrentHashMap();
+            create_queues.put(CREATE_QUEUE_QUEUES_KEY, create_queues_queues);
+        }
 
-		//if we have an exchange
-		if (exchangeName != null)
-		{
-		    //Retrieve the list of permitted exchanges.
-		    Map exchanges = (Map) create_queues_exchanges.get(exchangeName);
+        // If we are initialising and granting CREATE rights to all temporary queues, then that's all we do
+        Boolean temporary = false;
+        if (parameters.length == 1)
+        {
+            temporary = (Boolean) parameters[0];
+            create_queues.put(CREATE_QUEUE_TEMPORARY_KEY, temporary);
+            return;
+        }
 
-		    if (exchanges == null)
-		    {
-		        exchanges = new ConcurrentHashMap();
-		        create_queues_exchanges.put(exchangeName, exchanges);
-		    }
+        //From here we can be permissioning a variety of things, with varying parameters
+        AMQShortString queueName = parameters.length > 1 ? (AMQShortString) parameters[1] : null;
+        AMQShortString exchangeName = parameters.length > 2 ? (AMQShortString) parameters[2] : null;
+        //Set the routingkey to the specified value or the queueName if present
+        AMQShortString routingKey = (parameters.length > 3 && null != parameters[3]) ? (AMQShortString) parameters[3] : queueName;
+        // if we have a queueName then we need to store any associated exchange / rk bindings
+        if (queueName != null)
+        {
+            Map queue = (Map) create_queues_queues.get(queueName);
+            if (queue == null)
+            {
+                queue = new ConcurrentHashMap();
+                create_queues_queues.put(queueName, queue);
+            }
+
+            if (exchangeName != null)
+            {
+                queue.put(exchangeName, routingKey);
+            }
+
+            //If no exchange is specified then the presence of the queueName in the map says any exchange is ok
+        }
 
-		    //Store the temporary setting CREATE_QUEUE_EXCHANGES_ROUTINGKEYS_KEY
-		    exchanges.put(CREATE_QUEUE_EXCHANGES_TEMPORARY_KEY, temporary);
+        // Store the exchange that we are being granted rights to. This will be used as part of binding
 
-		    //Store the binding details of queue/rk for this exchange.
-		    if (queueName != null)
-		    {
-		        //Retrieve the list of permitted routingKeys.
-		        Map rKeys = (Map) exchanges.get(exchangeName);
+        //Lookup the list of exchanges
+        Map create_queues_exchanges = (Map) create_queues.get(CREATE_QUEUE_EXCHANGES_KEY);
 
-		        if (rKeys == null)
-		        {
-		            rKeys = new ConcurrentHashMap();
-		            exchanges.put(CREATE_QUEUE_EXCHANGES_ROUTINGKEYS_KEY, rKeys);
-		        }
+        if (create_queues_exchanges == null)
+        {
+            create_queues_exchanges = new ConcurrentHashMap();
+            create_queues.put(CREATE_QUEUE_EXCHANGES_KEY, create_queues_exchanges);
+        }
 
-		        rKeys.put(queueName, routingKey);
-		    }
-		}
-	}
+        //if we have an exchange
+        if (exchangeName != null)
+        {
+            //Retrieve the list of permitted exchanges.
+            Map exchanges = (Map) create_queues_exchanges.get(exchangeName);
+
+            if (exchanges == null)
+            {
+                exchanges = new ConcurrentHashMap();
+                create_queues_exchanges.put(exchangeName, exchanges);
+            }
+
+            //Store the binding details of queue/rk for this exchange.
+            if (queueName != null)
+            {
+                //Retrieve the list of permitted routingKeys.
+                Map rKeys = (Map) exchanges.get(exchangeName);
+
+                if (rKeys == null)
+                {
+                    rKeys = new ConcurrentHashMap();
+                    exchanges.put(CREATE_QUEUE_EXCHANGES_ROUTINGKEYS_KEY, rKeys);
+                }
 
-	private void grantConsume(Permission permission, Object... parameters) {
-		Map consumeRights = (Map) _permissions.get(permission);
+                rKeys.put(queueName, routingKey);
+            }
+        }
+    }
 
-		if (consumeRights == null)
-		{
-		    consumeRights = new ConcurrentHashMap();
-		    _permissions.put(permission, consumeRights);
-		}
+    /**
+     * Grant consume permissions
+     */
+    private void grantConsume(Permission permission, Object... parameters)
+    {
+        Map consumeRights = (Map) _permissions.get(permission);
 
-		//if we have parametsre
-		if (parameters.length > 0)
-		{
-		    AMQShortString queueName = (AMQShortString) parameters[0];
-		    Boolean temporary = (Boolean) parameters[1];
-		    Boolean ownQueueOnly = (Boolean) parameters[2];
+        if (consumeRights == null)
+        {
+           consumeRights = new ConcurrentHashMap();
+           _permissions.put(permission, consumeRights);
 
-		    if (temporary)
-		    {
-		        consumeRights.put(CONSUME_TEMPORARY_KEY, true);
-		    }
-		    else
-		    {
-		        consumeRights.put(CONSUME_TEMPORARY_KEY, false);
-		    }
+           //initialise own and temporary rights to false to be overwritten below if set
+           consumeRights.put(CONSUME_TEMPORARY_KEY, false);
+           consumeRights.put(CONSUME_OWN_QUEUES_ONLY_KEY, false);
+        }
 
-		    if (ownQueueOnly)
-		    {
-		        consumeRights.put(CONSUME_OWN_QUEUES_ONLY_KEY, true);
-		    }
-		    else
-		    {
-		        consumeRights.put(CONSUME_OWN_QUEUES_ONLY_KEY, false);
-		    }
 
+        //if we only have one param then we're permissioning temporary queues and topics
+        if (parameters.length == 1)
+        {
+           Boolean temporary = (Boolean) parameters[0];
 
-		    LinkedList queues = (LinkedList) consumeRights.get(CONSUME_QUEUES_KEY);
-		    if (queues == null)
-		    {
-		        queues = new LinkedList();
-		        consumeRights.put(CONSUME_QUEUES_KEY, queues);
-		    }
+           if (temporary)
+           {
+               consumeRights.put(CONSUME_TEMPORARY_KEY, true);
+           }
+        }
 
-		    if (queueName != null)
-		    {
-		        queues.add(queueName);
-		    }
-		}
-	}
+        //if we have 2 parameters - should be a contract for this, but for now we'll handle it as is
+        if (parameters.length == 2)
+        {
+           AMQShortString queueName = (AMQShortString) parameters[0];
+           Boolean ownQueueOnly = (Boolean) parameters[1];
+
+           if (ownQueueOnly)
+           {
+               consumeRights.put(CONSUME_OWN_QUEUES_ONLY_KEY, true);
+           }
+
+           LinkedList queues = (LinkedList) consumeRights.get(CONSUME_QUEUES_KEY);
+           if (queues == null)
+           {
+               queues = new LinkedList();
+               consumeRights.put(CONSUME_QUEUES_KEY, queues);
+           }
+
+           if (queueName != null)
+           {
+               queues.add(queueName);
+           }
+        }
+    }
 
     /**
      * 
@@ -399,62 +401,63 @@ public class PrincipalPermissions
 	        //user has been granted full access to the vhost
 	        return AuthzResult.ALLOWED;
 	    }
-	    
-		if (parameters.length == 1 && parameters[0] instanceof AMQQueue)
-		{
-		    AMQQueue queue = ((AMQQueue) parameters[0]);
-		    Map queuePermissions = (Map) _permissions.get(permission);
-		    
-		    if (queuePermissions == null)
-		    {
-		    	//if the outer map is null, the user has no CONSUME rights at all
-		    	return AuthzResult.DENIED;
-		    }
 
-		    List queues = (List) queuePermissions.get(CONSUME_QUEUES_KEY);
+        if (parameters.length == 1 && parameters[0] instanceof AMQQueue)
+        {
+            AMQQueue queue = ((AMQQueue) parameters[0]);
+            Map queuePermissions = (Map) _permissions.get(permission);
 
-		    Boolean temporayQueues = (Boolean) queuePermissions.get(CONSUME_TEMPORARY_KEY);
-		    Boolean ownQueuesOnly = (Boolean) queuePermissions.get(CONSUME_OWN_QUEUES_ONLY_KEY);
+            if (queuePermissions == null)
+            {
+                //we have a problem - we've never granted this type of permission .....
+                return AuthzResult.DENIED;
+            }
 
-		    // If user is allowed to publish to temporary queues and this is a temp queue then allow it.
-		    if (temporayQueues)
-		    {
-		        if (queue.isAutoDelete())
-		        // This will allow consumption from any temporary queue including ones not owned by this user.
-		        // Of course the exclusivity will not be broken.
-		        {
-		            // if not limited to ownQueuesOnly then ok else check queue Owner.
-		            return (!ownQueuesOnly || queue.getOwner().equals(_user)) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
-		        }
-		        else
-		        {
-		            return AuthzResult.DENIED;
-		        }
-		    }
+            List queues = (List) queuePermissions.get(CONSUME_QUEUES_KEY);
 
-		    // if queues are white listed then ensure it is ok
-		    if (queues != null)
-		    {
-		        // if no queues are listed then ALL are ok othereise it must be specified.
-		        if (ownQueuesOnly)
-		        {
-		            if (queue.getOwner().equals(_user))
-		            {
-		                return (queues.size() == 0 || queues.contains(queue.getName())) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
-		            }
-		            else
-		            {
-		                return AuthzResult.DENIED;
-		            }
-		        }
+            Boolean temporaryQueues = (Boolean) queuePermissions.get(CONSUME_TEMPORARY_KEY);
+            Boolean ownQueuesOnly = (Boolean) queuePermissions.get(CONSUME_OWN_QUEUES_ONLY_KEY);
 
-		        // If we are
-		        return (queues.size() == 0 || queues.contains(queue.getName())) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
-		    }
-		}
 
-		// Can't authenticate without the right parameters
-		return AuthzResult.DENIED;
+            // If user is allowed to consume from temporary queues and this is a temp queue then allow it.
+            if (temporaryQueues && queue.isAutoDelete())
+            {
+                // This will allow consumption from any temporary queue including ones not owned by this user.
+                // Of course the exclusivity will not be broken.
+                {
+                    // if not limited to ownQueuesOnly then ok else check queue Owner.
+                    return (!ownQueuesOnly || queue.getOwner().equals(_user)) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
+                }
+            }
+            //if this is a temporary queue and the user does not have permissions for temporary queues then deny
+            else if (!temporaryQueues && queue.isAutoDelete())
+            {
+                return AuthzResult.DENIED;
+            }
+
+            // if queues are white listed then ensure it is ok
+            if (queues != null)
+            {
+                // if no queues are listed then ALL are ok othereise it must be specified.
+                if (ownQueuesOnly)
+                {
+                    if (queue.getOwner().equals(_user))
+                    {
+                        return (queues.size() == 0 || queues.contains(queue.getName())) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
+                    }
+                    else
+                    {
+                        return AuthzResult.DENIED;
+                    }
+                }
+
+                // If we are
+                return (queues.size() == 0 || queues.contains(queue.getName())) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
+            }
+        }
+
+        // Can't authenticate without the right parameters
+        return AuthzResult.DENIED;
 	}
 
 	private AuthzResult authorisePublish(Permission permission, Object... parameters)
@@ -662,31 +665,7 @@ public class PrincipalPermissions
 		}
 		else
 		{
-		    //There a is no white list for queues
-
-		    // So can allow all queues to be bound
-		    //  but we should first check and see if we have a temp queue and validate that we are allowed
-		    //  to bind temp queues.
-
-		    //Check to see if we have a temporary queue
-		    if (bind_queueName.isAutoDelete())
-		    {
-		        // Check and see if we have an exchange white list.
-		        Map bind_exchanges = (Map) bind_create_queues.get(CREATE_QUEUE_EXCHANGES_KEY);
-
-		        // If the exchange exists then we must check to see if temporary queues are allowed here
-		        if (bind_exchanges != null)
-		        {
-		            // Check to see if the requested exchange is allowed.
-		            Map exchangeDetails = (Map) bind_exchanges.get(exchange.getName());
-
-		            return ((Boolean) exchangeDetails.get(CREATE_QUEUE_EXCHANGES_TEMPORARY_KEY)) ? AuthzResult.ALLOWED : AuthzResult.DENIED;
-		        }
-
-		        //no white list so all allowed, drop through to return true below.
-		    }
-
-		    // not a temporary queue and no white list so all allowed.
+		    //no white list so all allowed.
 		    return AuthzResult.ALLOWED;
 		}
 	}
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java
index d0514224f1..e299ce99c4 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java
@@ -187,8 +187,26 @@ public class SimpleXML implements ACLPlugin
 
     private void processConsume(Configuration config)
     {
+        boolean temporary = false;
+        Configuration tempConfig = null;
         Configuration consumeConfig = config.subset("access_control_list.consume");
 
+        tempConfig = consumeConfig.subset("queues.temporary(0)");
+        if (tempConfig != null)
+        {
+            temporary = true;
+        }
+
+        //Permission all users who have rights to temp queues and topics
+        if (tempConfig != null && !tempConfig.isEmpty())
+        {
+            String[] tempUsers = tempConfig.getStringArray("users.user");
+            for (String user : tempUsers)
+            {
+                grant(Permission.CONSUME, user, temporary);
+            }
+        }
+
         // Process queue limited users
         int queueCount = 0;
         Configuration queueConfig = consumeConfig.subset("queues.queue(" + queueCount + ")");
@@ -198,14 +216,14 @@ public class SimpleXML implements ACLPlugin
             // Get queue Name
             AMQShortString queueName = new AMQShortString(queueConfig.getString("name"));
             // if there is no name then there may be a temporary element
-            boolean temporary = queueConfig.containsKey("temporary");
+
             boolean ownQueues = queueConfig.containsKey("own_queues");
 
             // Process permissions for this queue
             String[] users = queueConfig.getStringArray("users.user");
             for (String user : users)
             {
-                grant(Permission.CONSUME, user, queueName, temporary, ownQueues);
+                grant(Permission.CONSUME, user, queueName, ownQueues);
             }
 
             // See if we have another config
@@ -218,14 +236,33 @@ public class SimpleXML implements ACLPlugin
 
         for (String user : users)
         {
+            //NOTE: this call does not appear to do anything inside the grant section for consume
             grant(Permission.CONSUME, user);
         }
     }
 
     private void processCreate(Configuration config)
     {
+        boolean temporary = false;
+        Configuration tempConfig = null;
+
         Configuration createConfig = config.subset("access_control_list.create");
 
+        tempConfig = createConfig.subset("queues.temporary(0)");
+        if (tempConfig != null)
+        {
+            temporary = true;
+        }
+
+        //Permission all users who have rights to temp queues and topics
+        if (tempConfig != null && !tempConfig.isEmpty())
+        {
+            String[] tempUsers = tempConfig.getStringArray("users.user");
+            for (String user : tempUsers)
+            {
+                grant(Permission.CREATEQUEUE, user, temporary);
+            }
+        }
         // Process create permissions for queue creation
         int queueCount = 0;
         Configuration queueConfig = createConfig.subset("queues.queue(" + queueCount + ")");
@@ -235,9 +272,6 @@ public class SimpleXML implements ACLPlugin
             // Get queue Name
             AMQShortString queueName = new AMQShortString(queueConfig.getString("name"));
 
-            // if there is no name then there may be a temporary element
-            boolean temporary = queueConfig.containsKey("temporary");
-
             int exchangeCount = 0;
             Configuration exchangeConfig = queueConfig.subset("exchanges.exchange(" + exchangeCount + ")");
 
@@ -246,12 +280,15 @@ public class SimpleXML implements ACLPlugin
 
                 AMQShortString exchange = new AMQShortString(exchangeConfig.getString("name"));
                 AMQShortString routingKey = new AMQShortString(exchangeConfig.getString("routing_key"));
-
+               
                 // Process permissions for this queue
                 String[] users = exchangeConfig.getStringArray("users.user");
                 for (String user : users)
                 {
+                    //This is broken as the user name is not stored
                     grant(Permission.CREATEEXCHANGE, user, exchange);
+
+                    //This call could be cleaned up as temporary is now being set earlier (above) 
                     grant(Permission.CREATEQUEUE, user, temporary, (queueName.equals("") ? null : queueName), (exchange
                             .equals("") ? null : exchange), (routingKey.equals("") ? null : routingKey));
                 }
@@ -287,6 +324,7 @@ public class SimpleXML implements ACLPlugin
             String[] users = exchangeConfig.getStringArray("users.user");
             for (String user : users)
             {
+                //And this is broken too 
                 grant(Permission.CREATEEXCHANGE, user, exchange, clazz);
             }
 
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/PrincipalPermissionsTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/PrincipalPermissionsTest.java
index adfe9f52e4..81735aa455 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/PrincipalPermissionsTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/PrincipalPermissionsTest.java
@@ -46,6 +46,7 @@ public class PrincipalPermissionsTest extends TestCase
     
     // Common things that are passed to frame constructors
     private AMQShortString _queueName = new AMQShortString(this.getClass().getName()+"queue");
+    private AMQShortString _tempQueueName = new AMQShortString(this.getClass().getName()+"tempqueue");
     private AMQShortString _exchangeName = new AMQShortString("amq.direct");
     private AMQShortString _routingKey = new AMQShortString(this.getClass().getName()+"route");
     private int _ticket = 1;
@@ -61,7 +62,9 @@ public class PrincipalPermissionsTest extends TestCase
     private VirtualHost _virtualHost;
     private AMQShortString _owner = new AMQShortString(this.getClass().getName()+"owner");
     private AMQQueue _queue;
+    private AMQQueue _temporaryQueue;
     private Boolean _temporary = false;
+    private Boolean _ownQueue = false;
         
     @Override
     public void setUp()
@@ -76,7 +79,8 @@ public class PrincipalPermissionsTest extends TestCase
             _virtualHost = new VirtualHost(new VirtualHostConfiguration("test", env));
             _exchange = DirectExchange.TYPE.newInstance(_virtualHost, _exchangeName, _durable, _ticket, _autoDelete);
             _queue = AMQQueueFactory.createAMQQueueImpl(_queueName, false, _owner , false, _virtualHost, _arguments);
-        } 
+            _temporaryQueue = AMQQueueFactory.createAMQQueueImpl(_tempQueueName, false, _owner , true, _virtualHost, _arguments);
+        }
         catch (Exception e)
         {
             fail(e.getMessage());
@@ -146,7 +150,7 @@ public class PrincipalPermissionsTest extends TestCase
     public void testConsume()
     {
         Object[] authArgs = new Object[]{_queue};
-        Object[] grantArgs = new Object[]{_queueName, _temporary, _temporary};
+        Object[] grantArgs = new Object[]{_queueName, _ownQueue};
         
         /* FIXME: This throws a null pointer exception QPID-1599
          * assertFalse(_perms.authorise(Permission.CONSUME, authArgs));
@@ -198,4 +202,58 @@ public class PrincipalPermissionsTest extends TestCase
         assertEquals("Queue creation was not allowed", AuthzResult.ALLOWED, _perms.authorise(Permission.CREATEQUEUE, authArgsCreateQueue));
         assertEquals("Binding creation was not allowed", AuthzResult.ALLOWED, _perms.authorise(Permission.BIND, authArgsBind));
     }
+
+    /**
+    * If the consume permission for temporary queues is for an unnamed queue then is should
+    * be global for any temporary queue but not for any non-temporary queue
+    */
+    public void testTemporaryUnnamedQueueConsume()
+    {
+       Object[] authNonTempQArgs = new Object[]{_queue};
+       Object[] authTempQArgs = new Object[]{_temporaryQueue};
+       Object[] grantArgs = new Object[]{true};
+
+       _perms.grant(Permission.CONSUME, grantArgs);
+
+       //Next line shows up bug - non temp queue should be denied
+       assertEquals(AuthzResult.DENIED, _perms.authorise(Permission.CONSUME, authNonTempQArgs));
+       assertEquals(AuthzResult.ALLOWED, _perms.authorise(Permission.CONSUME, authTempQArgs));
+    }
+
+    /**
+    * Test that temporary queue permissions before queue perms in the ACL config work correctly
+    */
+    public void testTemporaryQueueFirstConsume()
+    {
+       Object[] authNonTempQArgs = new Object[]{_queue};
+       Object[] authTempQArgs = new Object[]{_temporaryQueue};
+       Object[] grantArgs = new Object[]{true};
+       Object[] grantNonTempQArgs = new Object[]{_queueName, _ownQueue};
+
+       //should not matter if the temporary permission is processed first or last
+       _perms.grant(Permission.CONSUME, grantNonTempQArgs);
+       _perms.grant(Permission.CONSUME, grantArgs);
+
+       assertEquals(AuthzResult.ALLOWED, _perms.authorise(Permission.CONSUME, authNonTempQArgs));
+       assertEquals(AuthzResult.ALLOWED, _perms.authorise(Permission.CONSUME, authTempQArgs));
+    }
+
+    /**
+    * Test that temporary queue permissions after queue perms in the ACL config work correctly
+    */
+    public void testTemporaryQueueLastConsume()
+    {
+       Object[] authNonTempQArgs = new Object[]{_queue};
+       Object[] authTempQArgs = new Object[]{_temporaryQueue};
+       Object[] grantArgs = new Object[]{true};
+       Object[] grantNonTempQArgs = new Object[]{_queueName, _ownQueue};
+
+       //should not matter if the temporary permission is processed first or last
+       _perms.grant(Permission.CONSUME, grantArgs);
+       _perms.grant(Permission.CONSUME, grantNonTempQArgs);
+
+       assertEquals(AuthzResult.ALLOWED, _perms.authorise(Permission.CONSUME, authNonTempQArgs));
+       assertEquals(AuthzResult.ALLOWED, _perms.authorise(Permission.CONSUME, authTempQArgs));
+    }
+
 }