From 6aa10cc1aeb0ffbc6b02bf662b93eab879c517d7 Mon Sep 17 00:00:00 2001 From: Martin Ritchie Date: Mon, 10 Mar 2008 17:16:09 +0000 Subject: 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 --- .../server/handler/ExchangeDeclareHandler.java | 7 +- .../qpid/server/handler/QueueDeclareHandler.java | 13 ++- .../server/protocol/AMQMinaProtocolSession.java | 6 +- .../security/access/PrincipalPermissions.java | 28 ++---- .../server/security/access/plugins/SimpleXML.java | 101 ++------------------- .../java/org/apache/qpid/client/AMQSession.java | 4 +- .../qpid/client/protocol/AMQProtocolHandler.java | 3 +- .../org/apache/qpid/framing/AMQShortString.java | 4 +- .../apache/qpid/framing/AMQShortStringTest.java | 26 +++--- java/systests/etc/acl.config.xml | 4 - .../qpid/server/security/acl/SimpleACLTest.java | 28 +++--- 11 files changed, 63 insertions(+), 161 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>")) - { - // Binding to <> can not be programmed via ACLs due to '<','>' unable to be used in the XML - System.err.println("Binding on exchange <> 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 _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); -// } - } diff --git a/java/client/src/main/java/org/apache/qpid/client/AMQSession.java b/java/client/src/main/java/org/apache/qpid/client/AMQSession.java index 2ecc0515a5..0a90846b94 100644 --- a/java/client/src/main/java/org/apache/qpid/client/AMQSession.java +++ b/java/client/src/main/java/org/apache/qpid/client/AMQSession.java @@ -2377,7 +2377,7 @@ public class AMQSession extends Closeable implements Session, QueueSession, Topi * @todo Verify the destiation is valid or throw an exception. * @todo Be aware of possible changes to parameter order as versions change. */ - public AMQShortString declareQueue(final AMQDestination amqd, final AMQProtocolHandler protocolHandler) + private AMQShortString declareQueue(final AMQDestination amqd, final AMQProtocolHandler protocolHandler) throws AMQException { /*return new FailoverRetrySupport(*/ @@ -2445,7 +2445,7 @@ public class AMQSession extends Closeable implements Session, QueueSession, Topi return ++_nextProducerId; } - public AMQProtocolHandler getProtocolHandler() + private AMQProtocolHandler getProtocolHandler() { return _connection.getProtocolHandler(); } diff --git a/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java b/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java index 0faacaabc1..89982a1af0 100644 --- a/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java +++ b/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java @@ -209,8 +209,7 @@ public class AMQProtocolHandler extends IoHandlerAdapter } catch (RuntimeException e) { - _logger.warn(e.getMessage()); - e.printStackTrace(); + _logger.error(e.getMessage(), e); } if (Boolean.getBoolean("protectio")) diff --git a/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java b/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java index 505c819bb2..665cbf7a84 100644 --- a/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java +++ b/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java @@ -325,7 +325,7 @@ public final class AMQShortString implements CharSequence, Comparable2.0 - - false - - ${conf}/virtualhosts.xml diff --git a/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java b/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java index a643230bc2..62c54d86eb 100644 --- a/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java +++ b/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java @@ -45,12 +45,17 @@ public class SimpleACLTest extends TestCase implements ConnectionListener final String QpidExampleHome = System.getProperty("QPID_EXAMPLE_HOME"); final File defaultaclConfigFile = new File(QpidExampleHome, "etc/acl.config.xml"); - if (!defaultaclConfigFile.exists() || System.getProperty("QPID_HOME") == null) + if (!defaultaclConfigFile.exists()) { System.err.println("Configuration file not found:" + defaultaclConfigFile); fail("Configuration file not found:" + defaultaclConfigFile); } + if (System.getProperty("QPID_HOME") == null) + { + fail("QPID_HOME not set"); + } + ConfigurationFileApplicationRegistry config = new ConfigurationFileApplicationRegistry(defaultaclConfigFile); ApplicationRegistry.initialise(config, 1); @@ -175,9 +180,9 @@ public class SimpleACLTest extends TestCase implements ConnectionListener conn.start(); - //Create Temporary Queue - ((AMQSession) sesh).declareQueue((AMQDestination) sesh.createTemporaryQueue(), - ((AMQSession) sesh).getProtocolHandler()); + //Create Temporary Queue - can't use the createTempQueue as QueueName is null. + ((AMQSession) sesh).createQueue(new AMQShortString("doesnt_matter_as_autodelete_means_tmp"), + true, false, false); conn.close(); } @@ -198,8 +203,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener conn.start(); //Create a Named Queue - ((AMQSession) sesh).declareQueue((AMQDestination) sesh.createQueue("IllegalQueue"), - ((AMQSession) sesh).getProtocolHandler()); + ((AMQSession) sesh).createQueue(new AMQShortString("IllegalQueue"), false, false, false); fail("Test failed as Queue creation succeded."); } @@ -391,8 +395,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener conn.start(); //Create Temporary Queue - ((AMQSession) sesh).declareQueue((AMQDestination) sesh.createQueue("example.RequestQueue"), - ((AMQSession) sesh).getProtocolHandler()); + ((AMQSession) sesh).createQueue(new AMQShortString("example.RequestQueue"), false, false, false); conn.close(); } @@ -402,7 +405,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener } } - public void testServerCreateNamedQueueInValid() throws JMSException, URLSyntaxException, AMQException + public void testServerCreateNamedQueueInvalid() throws JMSException, URLSyntaxException, AMQException { try { @@ -413,8 +416,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener conn.start(); //Create a Named Queue - ((AMQSession) sesh).declareQueue((AMQDestination) sesh.createQueue("IllegalQueue"), - ((AMQSession) sesh).getProtocolHandler()); + ((AMQSession) sesh).createQueue(new AMQShortString("IllegalQueue"), false, false, false); fail("Test failed as creation succeded."); } @@ -434,8 +436,8 @@ public class SimpleACLTest extends TestCase implements ConnectionListener conn.start(); - ((AMQSession) sesh).declareQueue((AMQDestination) sesh.createTemporaryQueue(), - ((AMQSession) sesh).getProtocolHandler()); + ((AMQSession) sesh).createQueue(new AMQShortString("again_ensure_auto_delete_queue_for_temporary"), + true, false, false); fail("Test failed as creation succeded."); } -- cgit v1.2.1