QPID-3641: ACLV2 Simplifications and Improvements for Java Broker

Improvements and simplifications to ACL V2 for the Java Broker:
1) Removed 'EXECUTE' operation (we now just have ACCESS and UPDATE like C++ broker)
2) Enable users with management rights for a procedure to complete that procedure without matching AMQP rights (configurable)
3) Fix up system tests (make clearer, remove need for lots of support files)
4) Fix disparity in DENY_LOG and DENY-LOG values between brokers.
5) Get rid of transitive/expand permission rules

Work from Robbie Gemmell and myself.

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

3 files changed, 29 insertions, 84 deletions

diff --git a/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/RuleSet.java b/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/RuleSet.java
index 78355a7501..94f05ed265 100644
--- a/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/RuleSet.java
+++ b/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/RuleSet.java
@@ -36,7 +36,7 @@ import javax.security.auth.Subject;
 
 import org.apache.commons.lang.BooleanUtils;
 import org.apache.commons.lang.StringUtils;
-import org.apache.qpid.exchange.ExchangeDefaults;
+import org.apache.log4j.Logger;
 import org.apache.qpid.server.logging.actors.CurrentActor;
 import org.apache.qpid.server.security.Result;
 import org.apache.qpid.server.security.access.ObjectProperties;
@@ -53,20 +53,15 @@ import org.apache.qpid.server.security.access.logging.AccessControlMessages;
  */
 public class RuleSet
 {
+    public static final Logger _logger = Logger.getLogger(RuleSet.class);
+
     private static final String AT = "@";
     private static final String SLASH = "/";
 
     public static final String DEFAULT_ALLOW = "defaultallow";
     public static final String DEFAULT_DENY = "defaultdeny";
-    public static final String TRANSITIVE = "transitive";
-    public static final String EXPAND = "expand";
-    public static final String AUTONUMBER = "autonumber";
-    public static final String CONTROLLED = "controlled";
-    public static final String VALIDATE = "validate";
 
-    public static final List<String> CONFIG_PROPERTIES = Arrays.asList(
-            DEFAULT_ALLOW, DEFAULT_DENY, TRANSITIVE, EXPAND, AUTONUMBER, CONTROLLED
-        );
+    public static final List<String> CONFIG_PROPERTIES = Arrays.asList(DEFAULT_ALLOW, DEFAULT_DENY);
 
     private static final Integer _increment = 10;
 
@@ -80,7 +75,6 @@ public class RuleSet
     {
         // set some default configuration properties
         configure(DEFAULT_DENY, Boolean.TRUE);
-        configure(TRANSITIVE, Boolean.TRUE);
     }
 
     /**
@@ -139,10 +133,20 @@ public class RuleSet
 
             // Save the rules we selected
             objects.put(objectType, filtered);
+            if(_logger.isDebugEnabled())
+            {
+                _logger.debug("Cached " + objectType + " RulesList: " + filtered);
+            }
         }
 
         // Return the cached rules
-        return objects.get(objectType);
+        List<Rule> rules = objects.get(objectType);
+        if(_logger.isDebugEnabled())
+        {
+            _logger.debug("Returning RuleList: " + rules);
+        }
+
+        return rules;
     }
 
 
@@ -175,20 +179,6 @@ public class RuleSet
         return false;
     }
 
-    private Permission noLog(Permission permission)
-    {
-        switch (permission)
-        {
-            case ALLOW:
-            case ALLOW_LOG:
-                return Permission.ALLOW;
-            case DENY:
-            case DENY_LOG:
-            default:
-                return Permission.DENY;
-        }
-    }
-
     // TODO make this work when group membership is not known at file parse time
     public void addRule(Integer number, String identity, Permission permission, Action action)
     {
@@ -196,63 +186,13 @@ public class RuleSet
 
         if (!action.isAllowed())
         {
-            throw new IllegalArgumentException("Action is not allowd: " + action);
+            throw new IllegalArgumentException("Action is not allowed: " + action);
         }
         if (ruleExists(identity, action))
         {
             return;
         }
 
-        // expand actions - possibly multiply number by
-        if (isSet(EXPAND))
-        {
-            if (action.getOperation() == Operation.CREATE && action.getObjectType() == ObjectType.TOPIC)
-            {
-                addRule(null, identity, noLog(permission), new Action(Operation.BIND, ObjectType.EXCHANGE,
-                        new ObjectProperties("amq.topic", action.getProperties().get(ObjectProperties.Property.NAME))));
-                ObjectProperties topicProperties = new ObjectProperties();
-                topicProperties.put(ObjectProperties.Property.DURABLE, true);
-                addRule(null, identity, permission, new Action(Operation.CREATE, ObjectType.QUEUE, topicProperties));
-                return;
-            }
-            if (action.getOperation() == Operation.DELETE && action.getObjectType() == ObjectType.TOPIC)
-            {
-                addRule(null, identity, noLog(permission), new Action(Operation.UNBIND, ObjectType.EXCHANGE,
-                        new ObjectProperties("amq.topic", action.getProperties().get(ObjectProperties.Property.NAME))));
-                ObjectProperties topicProperties = new ObjectProperties();
-                topicProperties.put(ObjectProperties.Property.DURABLE, true);
-                addRule(null, identity, permission, new Action(Operation.DELETE, ObjectType.QUEUE, topicProperties));
-                return;
-            }
-        }
-
-        // transitive action dependencies
-        if (isSet(TRANSITIVE))
-        {
-            if (action.getOperation() == Operation.CREATE && action.getObjectType() == ObjectType.QUEUE)
-            {
-                ObjectProperties exchProperties = new ObjectProperties(action.getProperties());
-                exchProperties.setName(ExchangeDefaults.DEFAULT_EXCHANGE_NAME);
-                exchProperties.put(ObjectProperties.Property.ROUTING_KEY, action.getProperties().get(ObjectProperties.Property.NAME));
-                addRule(null, identity, noLog(permission), new Action(Operation.BIND, ObjectType.EXCHANGE, exchProperties));
-                if (action.getProperties().isSet(ObjectProperties.Property.AUTO_DELETE))
-                {
-                    addRule(null, identity, noLog(permission), new Action(Operation.DELETE, ObjectType.QUEUE, action.getProperties()));
-                }
-            }
-            else if (action.getOperation() == Operation.DELETE && action.getObjectType() == ObjectType.QUEUE)
-            {
-                ObjectProperties exchProperties = new ObjectProperties(action.getProperties());
-                exchProperties.setName(ExchangeDefaults.DEFAULT_EXCHANGE_NAME);
-                exchProperties.put(ObjectProperties.Property.ROUTING_KEY, action.getProperties().get(ObjectProperties.Property.NAME));
-                addRule(null, identity, noLog(permission), new Action(Operation.UNBIND, ObjectType.EXCHANGE, exchProperties));
-            }
-            else if (action.getOperation() != Operation.ACCESS && action.getObjectType() != ObjectType.VIRTUALHOST)
-            {
-                addRule(null, identity, noLog(permission), new Action(Operation.ACCESS, ObjectType.VIRTUALHOST));
-            }
-        }
-
         // set rule number if needed
         Rule rule = new Rule(number, identity, action, permission);
         if (rule.getNumber() == null)
@@ -392,24 +332,29 @@ public class RuleSet
         // Create the action to check
         Action action = new Action(operation, objectType, properties);
 
+        if(_logger.isDebugEnabled())
+        {
+            _logger.debug("Checking action: " + action);
+        }
+
         // get the list of rules relevant for this request
         List<Rule> rules = getRules(subject, operation, objectType);
         if (rules == null)
         {
-            if (isSet(CONTROLLED))
+            if(_logger.isDebugEnabled())
             {
-                // Abstain if there are no rules for this operation
-                return Result.ABSTAIN;
-            }
-            else
-            {
-                return getDefault();
+                _logger.debug("No rules found, returning default result");
             }
+            return getDefault();
         }
 
         // Iterate through a filtered set of rules dealing with this identity and operation
         for (Rule current : rules)
         {
+            if(_logger.isDebugEnabled())
+            {
+                _logger.debug("Checking against rule: " + current);
+            }
             // Check if action matches
             if (action.matches(current.getAction()))
             {
diff --git a/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AccessControl.java b/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AccessControl.java
index a7b3059262..a97b66a287 100644
--- a/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AccessControl.java
+++ b/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AccessControl.java
@@ -101,6 +101,7 @@ public class AccessControl extends AbstractPlugin
             return Result.ABSTAIN;
         }
  
+        _logger.debug("Checking " + operation + " " + objectType);
         return  _ruleSet.check(subject, operation, objectType, properties);
     }
 
diff --git a/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/RuleSetTest.java b/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/RuleSetTest.java
index bd9deac153..4d46a32f45 100644
--- a/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/RuleSetTest.java
+++ b/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/RuleSetTest.java
@@ -69,7 +69,6 @@ public class RuleSetTest extends QpidTestCase
         super.setUp();
 
         _ruleSet = new RuleSet();
-        _ruleSet.configure(RuleSet.TRANSITIVE, Boolean.FALSE);
     }
 
     @Override