QPID-107 : Changes based on code review.

git-svn-id: https://svn.apache.org/repos/asf/incubator/qpid/branches/M2.1@635602 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 30 insertions, 125 deletions

diff --git a/java/broker/src/main/java/org/apache/qpid/server/handler/ExchangeDeclareHandler.java b/java/broker/src/main/java/org/apache/qpid/server/handler/ExchangeDeclareHandler.java
index bc4476e969..9a98bc9659 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/handler/ExchangeDeclareHandler.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/handler/ExchangeDeclareHandler.java
@@ -59,8 +59,11 @@ public class ExchangeDeclareHandler implements StateAwareMethodListener<Exchange
         ExchangeRegistry exchangeRegistry = virtualHost.getExchangeRegistry();
         ExchangeFactory exchangeFactory = virtualHost.getExchangeFactory();
 
-        //Perform ACL
-        virtualHost.getAccessManager().authorise(session, Permission.CREATE, body);
+        if (!body.getPassive())
+        {
+            //Perform ACL if request is not passive
+            virtualHost.getAccessManager().authorise(session, Permission.CREATE, body);
+        }
 
         if (_logger.isDebugEnabled())
         {
diff --git a/java/broker/src/main/java/org/apache/qpid/server/handler/QueueDeclareHandler.java b/java/broker/src/main/java/org/apache/qpid/server/handler/QueueDeclareHandler.java
index 89a56b9a3c..7df864f189 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/handler/QueueDeclareHandler.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/handler/QueueDeclareHandler.java
@@ -55,7 +55,7 @@ public class QueueDeclareHandler implements StateAwareMethodListener<QueueDeclar
         return _instance;
     }
 
-    @Configured(path = "queue.auto_register", defaultValue = "false")
+    @Configured(path = "queue.auto_register", defaultValue = "true")
     public boolean autoRegister;
 
     private final AtomicInteger _counter = new AtomicInteger();
@@ -74,9 +74,12 @@ public class QueueDeclareHandler implements StateAwareMethodListener<QueueDeclar
         QueueRegistry queueRegistry = virtualHost.getQueueRegistry();
         MessageStore store = virtualHost.getMessageStore();
 
-        // Perform ACL on queue Creation
-        virtualHost.getAccessManager().authorise(session, Permission.CREATE, body);
 
+        if (!body.getPassive())
+        {
+            //Perform ACL if request is not passive
+            virtualHost.getAccessManager().authorise(session, Permission.CREATE, body);
+        }
 
 
         final AMQShortString queueName;
@@ -120,10 +123,6 @@ public class QueueDeclareHandler implements StateAwareMethodListener<QueueDeclar
                     {
                         Exchange defaultExchange = exchangeRegistry.getDefaultExchange();
 
-                        // Perform ACL to control bindings
-                        virtualHost.getAccessManager().authorise(session, Permission.BIND, body,
-                                                                 defaultExchange, queue, queueName);
-
                         queue.bind(queueName, null, defaultExchange);
                         _logger.info("Queue " + queueName + " bound to default exchange(" + defaultExchange.getName() + ")");
                     }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/protocol/AMQMinaProtocolSession.java b/java/broker/src/main/java/org/apache/qpid/server/protocol/AMQMinaProtocolSession.java
index ab503289ba..4c65a4682a 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/protocol/AMQMinaProtocolSession.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/protocol/AMQMinaProtocolSession.java
@@ -348,7 +348,7 @@ public class AMQMinaProtocolSession implements AMQProtocolSession, Managable
                     _logger.info("Closing connection due to: " + e.getMessage());
                 }
 
-                markChannelawaitingCloseOk(channelId);
+                markChannelAwaitingCloseOk(channelId);
                 closeSession();
                 _stateManager.changeState(AMQState.CONNECTION_CLOSING);
                 writeFrame(e.getCloseFrame(channelId));
@@ -537,7 +537,7 @@ public class AMQMinaProtocolSession implements AMQProtocolSession, Managable
             try
             {
                 channel.close(this);
-                markChannelawaitingCloseOk(channelId);
+                markChannelAwaitingCloseOk(channelId);
             }
             finally
             {
@@ -552,7 +552,7 @@ public class AMQMinaProtocolSession implements AMQProtocolSession, Managable
         _closingChannelsList.remove(new Integer(channelId));
     }
 
-    private void markChannelawaitingCloseOk(int channelId)
+    private void markChannelAwaitingCloseOk(int channelId)
     {
         _closingChannelsList.add(channelId);
     }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java b/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java
index 22f1cf25a8..23073e0613 100755
--- a/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/access/PrincipalPermissions.java
@@ -33,19 +33,19 @@ import java.util.concurrent.ConcurrentHashMap;
 public class PrincipalPermissions
 {
 
-    private static final int CONSUME_QUEUES_KEY = 0;
-    private static final int CONSUME_TEMPORARY_KEY = 1;
-    private static final int CONSUME_OWN_QUEUES_ONLY_KEY = 2;
+    private static final Object CONSUME_QUEUES_KEY = new Object();
+    private static final Object CONSUME_TEMPORARY_KEY = new Object();
+    private static final Object CONSUME_OWN_QUEUES_ONLY_KEY = new Object();
 
-    private static final int CREATE_QUEUES_KEY = 0;
-    private static final int CREATE_EXCHANGES_KEY = 1;
+    private static final Object CREATE_QUEUES_KEY = new Object();
+    private static final Object CREATE_EXCHANGES_KEY = new Object();
 
-    private static final int CREATE_QUEUE_TEMPORARY_KEY = 2;
-    private static final int CREATE_QUEUE_QUEUES_KEY = 1;
-    private static final int CREATE_QUEUE_EXCHANGES_KEY = 0;
+    private static final Object CREATE_QUEUE_TEMPORARY_KEY = new Object();
+    private static final Object CREATE_QUEUE_QUEUES_KEY = new Object();
+    private static final Object CREATE_QUEUE_EXCHANGES_KEY = new Object();
 
-    private static final int CREATE_QUEUE_EXCHANGES_TEMPORARY_KEY = 0;
-    private static final int CREATE_QUEUE_EXCHANGES_ROUTINGKEYS_KEY = 1;
+    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;
 
@@ -316,16 +316,8 @@ public class PrincipalPermissions
                 // The existence of this user specific PP can be validated in the map SimpleXML maintains.
             case BIND: // Parameters : QueueBindMethod , Exchange , AMQQueue, AMQShortString routingKey
 
-//                QueueDeclareBody body = (QueueDeclareBody) parameters[0];
-
                 Exchange exchange = (Exchange) parameters[1];
 
-                if (exchange.getName().equals("<<default>>"))
-                {
-                    // Binding to <<default>> can not be programmed via ACLs due to '<','>' unable to be used in the XML
-                    System.err.println("Binding on exchange <<default>> not alowed via ACLs");
-                }
-
                 AMQQueue bind_queueName = (AMQQueue) parameters[2];
                 AMQShortString routingKey = (AMQShortString) parameters[3];
 
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java b/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java
index c09cdc33f5..251f4e6330 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/access/plugins/SimpleXML.java
@@ -46,9 +46,8 @@ import java.util.concurrent.ConcurrentHashMap;
  */
 public class SimpleXML implements ACLPlugin
 {
-    private static final Logger _logger = ACLManager.getLogger();
-
     private Map<String, PrincipalPermissions> _users;
+    private final AccessResult GRANTED = new AccessResult(this, AccessResult.AccessStatus.GRANTED);
 
     public SimpleXML()
     {
@@ -57,8 +56,6 @@ public class SimpleXML implements ACLPlugin
 
     public void setConfiguaration(Configuration config)
     {
-        _logger.info("SimpleXML Configuration");
-
         processConfig(config);
     }
 
@@ -87,7 +84,6 @@ public class SimpleXML implements ACLPlugin
         for (String user : users)
         {
             grant(Permission.PUBLISH, user);
-            _logger.info("PUBLISH:GRANTED:USER:" + user + " for all destinations");
         }
 
         // Process exchange limited users
@@ -113,7 +109,6 @@ public class SimpleXML implements ACLPlugin
                 for (String user : users)
                 {
                     grant(Permission.PUBLISH, user, exchangeName, routingKeyValue);
-                    _logger.info("PUBLISH:GRANTED:USER:" + user + " on Exchange '" + exchangeName + "' for key '" + routingKeyValue + "'");
                 }
 
                 //Apply permissions to Groups
@@ -129,7 +124,6 @@ public class SimpleXML implements ACLPlugin
             for (String user : users)
             {
                 grant(Permission.PUBLISH, user, exchangeName);
-                _logger.info("PUBLISH:GRANTED:USER:" + user + " on Exchange:" + exchangeName);
             }
 
             //Apply permissions to Groups
@@ -172,21 +166,6 @@ public class SimpleXML implements ACLPlugin
             for (String user : users)
             {
                 grant(Permission.CONSUME, user, queueName, temporary, ownQueues);
-                if (temporary)
-                {
-                    if (ownQueues)
-                    {
-                        _logger.info("CONSUME:GRANTED:USER:" + user + " on temporary queues owned by user.");
-                    }
-                    else
-                    {
-                        _logger.info("CONSUME:GRANTED:USER:" + user + " on all temporary queues.");
-                    }
-                }
-                else
-                {
-                    _logger.info("CONSUME:GRANTED:USER:" + user + " on queue '" + queueName + "'");
-                }
             }
 
             //See if we have another config
@@ -200,7 +179,6 @@ public class SimpleXML implements ACLPlugin
         for (String user : users)
         {
             grant(Permission.CONSUME, user);
-            _logger.info("CONSUME:GRANTED:USER:" + user + " from all queues.");
         }
     }
 
@@ -237,12 +215,6 @@ public class SimpleXML implements ACLPlugin
                           (queueName.equals("") ? null : queueName),
                           (exchange.equals("") ? null : exchange),
                           (routingKey.equals("") ? null : routingKey));
-                    
-                    _logger.info("CREATE :GRANTED:USER:" + user + " for "
-                            + (queueName.equals("") ? "" : "queue '" + queueName + "' ")
-                            + (exchange.equals("") ? "" : "exchange '" + exchange + "' ")
-                            + (routingKey.equals("") ? "" : " rk '" + routingKey + "' ")
-                            + (temporary ? " temporary:" + temporary : ""));
                 }
 
                 //See if we have another config
@@ -256,14 +228,6 @@ public class SimpleXML implements ACLPlugin
             for (String user : users)
             {
                 grant(Permission.CREATE, user, temporary, queueName);
-                if (temporary)
-                {
-                    _logger.info("CREATE :GRANTED:USER:" + user + " from temporary queues on any exchange.");
-                }
-                else
-                {
-                    _logger.info("CREATE :GRANTED:USER:" + user + " from queue '" + queueName + "' on any exchange.");
-                }
             }
 
             //See if we have another config
@@ -285,7 +249,6 @@ public class SimpleXML implements ACLPlugin
             for (String user : users)
             {
                 grant(Permission.CREATE, user, exchange, clazz);
-                _logger.info("CREATE:GRANTED:USER:" + user + " for exchange '" + exchange + ":class:'" + clazz);
             }
 
             //See if we have another config
@@ -299,7 +262,6 @@ public class SimpleXML implements ACLPlugin
         for (String user : users)
         {
             grant(Permission.CREATE, user);
-            _logger.info("CREATE:GRANTED:USER:" + user + " from all queues & exchanges.");
         }
 
 
@@ -326,15 +288,12 @@ public class SimpleXML implements ACLPlugin
         //Get the Users Permissions
         PrincipalPermissions permissions = _users.get(username);
 
-        _logger.warn("Processing :" + permission + " for:" + username + ":" + permissions+":"+parameters.length);
-
         if (permissions != null)
         {
             switch (permission)
             {
                 case ACCESS:
-                    _logger.warn("GRANTED:"+permission);
-                    return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
+                    return GRANTED;
                 case BIND:  // Body QueueDeclareBody - Parameters : Exchange, Queue, QueueName
                     // Body QueueBindBody - Paramters : Exchange, Queue, QueueName
                     if (parameters.length == 3)
@@ -342,23 +301,20 @@ public class SimpleXML implements ACLPlugin
                         // Parameters : Exchange, Queue, RoutingKey
                         if (permissions.authorise(Permission.BIND, body, parameters[0], parameters[1], parameters[2]))
                         {
-                            _logger.warn("GRANTED:"+permission);
-                            return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
+                            return GRANTED;
                         }
                     }
                     break;
                 case CONSUME: // Parameters : none
                     if (parameters.length == 1 && permissions.authorise(Permission.CONSUME, parameters[0]))
                     {
-                        _logger.warn("GRANTED:"+permission);
-                        return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
+                        return GRANTED;
                     }
                     break;
                 case CREATE: // Body : QueueDeclareBody | ExchangeDeclareBody - Parameters : none
                     if (permissions.authorise(Permission.CREATE, body))
                     {
-                        _logger.warn("GRANTED:"+permission);
-                        return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
+                        return GRANTED;
                     }
                     break;
                 case PUBLISH: // Body : BasicPublishBody  Parameters : exchange
@@ -367,8 +323,7 @@ public class SimpleXML implements ACLPlugin
                         if (permissions.authorise(Permission.PUBLISH, ((Exchange) parameters[0]).getName(),
                                                   ((BasicPublishBody) body).getRoutingKey()))
                         {
-                            _logger.warn("GRANTED:"+permission);
-                            return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
+                            return GRANTED;
                         }
                     }
                     break;
@@ -381,51 +336,7 @@ public class SimpleXML implements ACLPlugin
             }
         }
 
-        _logger.warn("Access Denied for :" + permission + " for:" + username + ":" + permissions);
         //todo potential refactor this ConnectionException Out of here
         throw body.getConnectionException(AMQConstant.ACCESS_REFUSED, error);
     }
-
-//todo use or lose
-//        if (accessObject instanceof VirtualHost)
-//        {
-//            VirtualHostAccess[] hosts = lookupVirtualHost(user.getName());
-//
-//            if (hosts != null)
-//            {
-//                for (VirtualHostAccess host : hosts)
-//                {
-//                    if (accessObject.getAccessableName().equals(host.getVirtualHost()))
-//                    {
-//                        if (host.getAccessRights().allows(rights))
-//                        {
-//                            return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
-//                        }
-//                        else
-//                        {
-//                            return new AccessResult(this, AccessResult.AccessStatus.REFUSED);
-//                        }
-//                    }
-//                }
-//            }
-//        }
-//        else if (accessObject instanceof AMQQueue)
-//        {
-//            String[] queues = lookupQueue(username, ((AMQQueue) accessObject).getVirtualHost());
-//
-//            if (queues != null)
-//            {
-//                for (String queue : queues)
-//                {
-//                    if (accessObject.getAccessableName().equals(queue))
-//                    {
-//                        return new AccessResult(this, AccessResult.AccessStatus.GRANTED);
-//                    }
-//                }
-//            }
-//        }
-
-//        return new AccessResult(this, AccessResult.AccessStatus.REFUSED);
-//    }
-
 }