QPID-3310 - Principal/Subject refactoring.

Refactoring to the connection/session objects to pass the Subject from Authentication tier to Access tier, rather than just
the Principal.  Change the access-control to be able to make access decisions based on Groups from the Authentication tier
whilst retaining support for groups declared within the ACL file itself.  Improve unit tests.

Applied patch by Keith Wall <keith.wall@gmail.com>

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

6 files changed, 697 insertions, 384 deletions

diff --git a/java/broker-plugins/access-control/MANIFEST.MF b/java/broker-plugins/access-control/MANIFEST.MF
index 1cd285ba20..78072850e4 100644
--- a/java/broker-plugins/access-control/MANIFEST.MF
+++ b/java/broker-plugins/access-control/MANIFEST.MF
@@ -35,6 +35,7 @@ Import-Package: org.apache.qpid,
  org.apache.log4j;version=1.0.0,
  javax.management;version=1.0.0,
  javax.management.openmbean;version=1.0.0,
+ javax.security.auth;version=1.0.0,
  org.osgi.util.tracker;version=1.0.0,
  org.osgi.framework;version=1.3
 Private-Package: org.apache.qpid.server.security.access.config,
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 ebc73440ed..78355a7501 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
@@ -18,20 +18,24 @@
  */
 package org.apache.qpid.server.security.access.config;
 
+import java.security.Principal;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.EnumMap;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.WeakHashMap;
 
+import javax.security.auth.Subject;
+
 import org.apache.commons.lang.BooleanUtils;
 import org.apache.commons.lang.StringUtils;
-import org.apache.log4j.Logger;
 import org.apache.qpid.exchange.ExchangeDefaults;
 import org.apache.qpid.server.logging.actors.CurrentActor;
 import org.apache.qpid.server.security.Result;
@@ -45,147 +49,132 @@ import org.apache.qpid.server.security.access.logging.AccessControlMessages;
  * Models the rule configuration for the access control plugin.
  *
  * The access control rule definitions are loaded from an external configuration file, passed in as the
- * target to the {@link load(ConfigurationFile)} method. The file specified 
+ * target to the {@link load(ConfigurationFile)} method. The file specified
  */
 public class RuleSet
 {
-    private static final Logger _logger = Logger.getLogger(RuleSet.class);
-    
     private static final String AT = "@";
-	private static final String SLASH = "/";
+    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 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
         );
-    
+
     private static final Integer _increment = 10;
-	
-    private final Map<String, List<String>> _groups = new HashMap<String, List<String>>();
+
+    private final Map<String, List<String>> _aclGroups = new HashMap<String, List<String>>();
     private final SortedMap<Integer, Rule> _rules = new TreeMap<Integer, Rule>();
-    private final Map<String, Map<Operation, Map<ObjectType, List<Rule>>>> _cache =
-                        new WeakHashMap<String, Map<Operation, Map<ObjectType, List<Rule>>>>();
+    private final Map<Subject, Map<Operation, Map<ObjectType, List<Rule>>>> _cache =
+                        new WeakHashMap<Subject, Map<Operation, Map<ObjectType, List<Rule>>>>();
     private final Map<String, Boolean> _config = new HashMap<String, Boolean>();
-    
+
     public RuleSet()
     {
         // set some default configuration properties
         configure(DEFAULT_DENY, Boolean.TRUE);
         configure(TRANSITIVE, Boolean.TRUE);
     }
-    
+
     /**
-     * Clear the contents, invluding groups, rules and configuration.
+     * Clear the contents, including acl groups, rules and configuration.
      */
     public void clear()
     {
         _rules.clear();
         _cache.clear();
         _config.clear();
-        _groups.clear();
+        _aclGroups.clear();
     }
-    
+
     public int getRuleCount()
     {
         return _rules.size();
     }
-	
-	/**
-	 * Filtered rules list based on an identity and operation.
-	 * 
-	 * Allows only enabled rules with identity equal to all, the same, or a group with identity as a member,
-	 * and operation is either all or the same operation.
-	 */		
-	public List<Rule> getRules(String identity, Operation operation, ObjectType objectType)
-	{
-        // Lookup identity in cache and create empty operation map if required
-		Map<Operation, Map<ObjectType, List<Rule>>> operations = _cache.get(identity);		
-		if (operations == null)
-		{	
-			operations = new EnumMap<Operation, Map<ObjectType, List<Rule>>>(Operation.class);
-			_cache.put(identity, operations);
-		}
-		
-        // Lookup operation and create empty object type map if required        
-        Map<ObjectType, List<Rule>> objects = operations.get(operation);
-		if (objects == null)
-		{
-            objects = new EnumMap<ObjectType, List<Rule>>(ObjectType.class);
-            operations.put(operation, objects);
-        }
+
+    /**
+     * Filtered rules list based on a subject and operation.
+     *
+     * Allows only enabled rules with identity equal to all, the same, or a group with identity as a member,
+     * and operation is either all or the same operation.
+     */
+    public List<Rule> getRules(final Subject subject, final Operation operation, final ObjectType objectType)
+    {
+        final Map<ObjectType, List<Rule>> objects = getObjectToRuleCache(subject, operation);
 
         // Lookup object type rules for the operation
         if (!objects.containsKey(objectType))
         {
+            final Set<Principal> principals = subject.getPrincipals();
             boolean controlled = false;
             List<Rule> filtered = new LinkedList<Rule>();
             for (Rule rule : _rules.values())
             {
+                final Action ruleAction = rule.getAction();
                 if (rule.isEnabled()
-                    && (rule.getAction().getOperation() == Operation.ALL || rule.getAction().getOperation() == operation)
-                    && (rule.getAction().getObjectType() == ObjectType.ALL || rule.getAction().getObjectType() == objectType))
+                    && (ruleAction.getOperation() == Operation.ALL || ruleAction.getOperation() == operation)
+                    && (ruleAction.getObjectType() == ObjectType.ALL || ruleAction.getObjectType() == objectType))
                 {
                     controlled = true;
 
-                    if (rule.getIdentity().equalsIgnoreCase(Rule.ALL)
-                        || rule.getIdentity().equalsIgnoreCase(identity)
-                        || (_groups.containsKey(rule.getIdentity()) && _groups.get(rule.getIdentity()).contains(identity)))
+                    if (isRelevant(principals,rule))
                     {
                         filtered.add(rule);
                     }
                 }
             }
-            
+
             // Return null if there are no rules at all for this operation and object type
             if (filtered.isEmpty() && controlled == false)
             {
                 filtered = null;
             }
-            
+
             // Save the rules we selected
             objects.put(objectType, filtered);
         }
-		
+
         // Return the cached rules
-		return objects.get(objectType);
-	}
-    
+        return objects.get(objectType);
+    }
+
+
     public boolean isValidNumber(Integer number)
     {
         return !_rules.containsKey(number);
     }
-	
+
     public void grant(Integer number, String identity, Permission permission, Operation operation)
     {
         Action action = new Action(operation);
         addRule(number, identity, permission, action);
     }
-    
+
     public void grant(Integer number, String identity, Permission permission, Operation operation, ObjectType object, ObjectProperties properties)
     {
         Action action = new Action(operation, object, properties);
         addRule(number, identity, permission, action);
     }
-    
+
     public boolean ruleExists(String identity, Action action)
     {
-		for (Rule rule : _rules.values())
-		{
-		    if (rule.getIdentity().equals(identity) && rule.getAction().equals(action))
-		    {
-		        return true;
-		    }
-		}
-		return false;
+        for (Rule rule : _rules.values())
+        {
+            if (rule.getIdentity().equals(identity) && rule.getAction().equals(action))
+            {
+                return true;
+            }
+        }
+        return false;
     }
-    
+
     private Permission noLog(Permission permission)
     {
         switch (permission)
@@ -203,15 +192,17 @@ public class RuleSet
     // 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)
     {
-		if (!action.isAllowed())
-		{
-			throw new IllegalArgumentException("Action is not allowd: " + action);
-		}
+        _cache.clear();
+
+        if (!action.isAllowed())
+        {
+            throw new IllegalArgumentException("Action is not allowd: " + action);
+        }
         if (ruleExists(identity, action))
         {
             return;
         }
-        
+
         // expand actions - possibly multiply number by
         if (isSet(EXPAND))
         {
@@ -234,8 +225,8 @@ public class RuleSet
                 return;
             }
         }
-        
-		// transitive action dependencies
+
+        // transitive action dependencies
         if (isSet(TRANSITIVE))
         {
             if (action.getOperation() == Operation.CREATE && action.getObjectType() == ObjectType.QUEUE)
@@ -244,10 +235,10 @@ public class RuleSet
                 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()));
-				}
+                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)
             {
@@ -261,9 +252,9 @@ public class RuleSet
                 addRule(null, identity, noLog(permission), new Action(Operation.ACCESS, ObjectType.VIRTUALHOST));
             }
         }
-        
+
         // set rule number if needed
-        Rule rule = new Rule(number, identity, action, permission);        
+        Rule rule = new Rule(number, identity, action, permission);
         if (rule.getNumber() == null)
         {
             if (_rules.isEmpty())
@@ -275,34 +266,36 @@ public class RuleSet
                 rule.setNumber(_rules.lastKey() + _increment);
             }
         }
-        
+
         // save rule
         _cache.remove(identity);
         _rules.put(rule.getNumber(), rule);
-	}
-    
+    }
+
     public void enableRule(int ruleNumber)
     {
         _rules.get(Integer.valueOf(ruleNumber)).enable();
     }
-    
+
     public void disableRule(int ruleNumber)
     {
         _rules.get(Integer.valueOf(ruleNumber)).disable();
     }
-    
+
     public boolean addGroup(String group, List<String> constituents)
     {
-        if (_groups.containsKey(group))
+        _cache.clear();
+
+        if (_aclGroups.containsKey(group))
         {
             // cannot redefine
             return false;
         }
         else
         {
-            _groups.put(group, new ArrayList<String>());
+            _aclGroups.put(group, new ArrayList<String>());
         }
-        
+
         for (String name : constituents)
         {
             if (name.equalsIgnoreCase(group))
@@ -310,17 +303,17 @@ public class RuleSet
                 // recursive definition
                 return false;
             }
-            
+
             if (!checkName(name))
             {
                 // invalid name
                 return false;
             }
-            
-            if (_groups.containsKey(name))
+
+            if (_aclGroups.containsKey(name))
             {
                 // is a group
-                _groups.get(group).addAll(_groups.get(name));
+                _aclGroups.get(group).addAll(_aclGroups.get(name));
             }
             else
             {
@@ -330,12 +323,12 @@ public class RuleSet
                     // invalid username
                     return false;
                 }
-                _groups.get(group).add(name);
+                _aclGroups.get(group).add(name);
             }
         }
         return true;
     }
-    
+
     /** Return true if the name is well-formed (contains legal characters). */
     protected boolean checkName(String name)
     {
@@ -349,79 +342,79 @@ public class RuleSet
         }
         return true;
     }
-    
+
     /** Returns true if a username has the name[@domain][/realm] format  */
     protected boolean isvalidUserName(String name)
-    {	
-		// check for '@' and '/' in namne
-		int atPos = name.indexOf(AT);
-		int slashPos = name.indexOf(SLASH);
-		boolean atFound = atPos != StringUtils.INDEX_NOT_FOUND && atPos == name.lastIndexOf(AT);
-		boolean slashFound = slashPos != StringUtils.INDEX_NOT_FOUND && slashPos == name.lastIndexOf(SLASH);
-				
-		// must be at least one character after '@' or '/'
-		if (atFound && atPos > name.length() - 2)
-		{
-			return false;
-		}
-		if (slashFound && slashPos > name.length() - 2)
-		{
-			return false;
-		}
-		
-		// must be at least one character between '@' and '/'
-		if (atFound && slashFound)
-		{
-			return (atPos < (slashPos - 1));
-		}
-		
-		// otherwise all good
-		return true; 
+    {
+        // check for '@' and '/' in namne
+        int atPos = name.indexOf(AT);
+        int slashPos = name.indexOf(SLASH);
+        boolean atFound = atPos != StringUtils.INDEX_NOT_FOUND && atPos == name.lastIndexOf(AT);
+        boolean slashFound = slashPos != StringUtils.INDEX_NOT_FOUND && slashPos == name.lastIndexOf(SLASH);
+
+        // must be at least one character after '@' or '/'
+        if (atFound && atPos > name.length() - 2)
+        {
+            return false;
+        }
+        if (slashFound && slashPos > name.length() - 2)
+        {
+            return false;
+        }
+
+        // must be at least one character between '@' and '/'
+        if (atFound && slashFound)
+        {
+            return (atPos < (slashPos - 1));
+        }
+
+        // otherwise all good
+        return true;
     }
 
-	// C++ broker authorise function prototype
+    // C++ broker authorise function prototype
     // virtual bool authorise(const std::string& id, const Action& action, const ObjectType& objType,
-	//		const std::string& name, std::map<Property, std::string>* params=0);
-	
-	// Possibly add a String name paramater?
+    //        const std::string& name, std::map<Property, std::string>* params=0);
+
+    // Possibly add a String name paramater?
 
     /**
      * Check the authorisation granted to a particular identity for an operation on an object type with
      * specific properties.
      *
-     * Looks up the entire ruleset, whcih may be cached, for the user and operation and goes through the rules
+     * Looks up the entire ruleset, which may be cached, for the user and operation and goes through the rules
      * in order to find the first one that matches. Either defers if there are no rules, returns the result of
      * the first match found, or denies access if there are no matching rules. Normally, it would be expected
      * to have a default deny or allow rule at the end of an access configuration however.
      */
-    public Result check(String identity, Operation operation, ObjectType objectType, ObjectProperties properties)
+    public Result check(Subject subject, Operation operation, ObjectType objectType, ObjectProperties properties)
     {
         // Create the action to check
         Action action = new Action(operation, objectType, properties);
 
-		// get the list of rules relevant for this request
-		List<Rule> rules = getRules(identity, operation, objectType);
-		if (rules == null)
-		{
-		    if (isSet(CONTROLLED))
-		    {
-    		    // Abstain if there are no rules for this operation
+        // get the list of rules relevant for this request
+        List<Rule> rules = getRules(subject, operation, objectType);
+        if (rules == null)
+        {
+            if (isSet(CONTROLLED))
+            {
+                // Abstain if there are no rules for this operation
                 return Result.ABSTAIN;
-		    }
-		    else
-		    {
-		        return getDefault();
-		    }
-		}
-		
-		// Iterate through a filtered set of rules dealing with this identity and operation
+            }
+            else
+            {
+                return getDefault();
+            }
+        }
+
+        // Iterate through a filtered set of rules dealing with this identity and operation
         for (Rule current : rules)
-		{
-			// Check if action matches
+        {
+            // Check if action matches
             if (action.matches(current.getAction()))
             {
                 Permission permission = current.getPermission();
-                
+
                 switch (permission)
                 {
                     case ALLOW_LOG:
@@ -439,15 +432,15 @@ public class RuleSet
                 return Result.DENIED;
             }
         }
-        
+
         // Defer to the next plugin of this type, if it exists
-		return Result.DEFER;
+        return Result.DEFER;
     }
-	
-	/** Default deny. */
-	public Result getDefault()
-	{
-	    if (isSet(DEFAULT_ALLOW))
+
+    /** Default deny. */
+    public Result getDefault()
+    {
+        if (isSet(DEFAULT_ALLOW))
         {
             return Result.ALLOWED;
         }
@@ -456,19 +449,19 @@ public class RuleSet
             return Result.DENIED;
         }
         return Result.ABSTAIN;
-	}
-	
-	/**
-	 * Check if a configuration property is set.
-	 */
-	protected boolean isSet(String key)
-	{
-	    return BooleanUtils.isTrue(_config.get(key));
-	}
+    }
+
+    /**
+     * Check if a configuration property is set.
+     */
+    protected boolean isSet(String key)
+    {
+        return BooleanUtils.isTrue(_config.get(key));
+    }
 
     /**
      * Configure properties for the plugin instance.
-     * 
+     *
      * @param properties
      */
     public void configure(Map<String, Boolean> properties)
@@ -478,7 +471,7 @@ public class RuleSet
 
     /**
      * Configure a single property for the plugin instance.
-     * 
+     *
      * @param key
      * @param value
      */
@@ -486,4 +479,48 @@ public class RuleSet
     {
         _config.put(key, value);
     }
+
+    private boolean isRelevant(final Set<Principal> principals, final Rule rule)
+    {
+        if (rule.getIdentity().equalsIgnoreCase(Rule.ALL))
+        {
+            return true;
+        }
+        else
+        {
+            for (Iterator<Principal> iterator = principals.iterator(); iterator.hasNext();)
+            {
+                final Principal principal = iterator.next();
+
+                if (rule.getIdentity().equalsIgnoreCase(principal.getName())
+                    || (_aclGroups.containsKey(rule.getIdentity()) && _aclGroups.get(rule.getIdentity()).contains(principal.getName())))
+                {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
+
+    private Map<ObjectType, List<Rule>> getObjectToRuleCache(final Subject subject, final Operation operation)
+    {
+        // Lookup identity in cache and create empty operation map if required
+        Map<Operation, Map<ObjectType, List<Rule>>> operations = _cache.get(subject);
+        if (operations == null)
+        {
+            operations = new EnumMap<Operation, Map<ObjectType, List<Rule>>>(Operation.class);
+            _cache.put(subject, operations);
+        }
+
+        // Lookup operation and create empty object type map if required
+        Map<ObjectType, List<Rule>> objects = operations.get(operation);
+        if (objects == null)
+        {
+            objects = new EnumMap<ObjectType, List<Rule>>(ObjectType.class);
+            operations.put(operation, objects);
+        }
+        return objects;
+    }
+
 }
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 69cfa173bd..a7b3059262 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
@@ -20,7 +20,7 @@
  */
 package org.apache.qpid.server.security.access.plugins;
 
-import java.security.Principal;
+import javax.security.auth.Subject;
 
 import org.apache.commons.configuration.ConfigurationException;
 import org.apache.log4j.Logger;
@@ -89,20 +89,19 @@ public class AccessControl extends AbstractPlugin
     
     /**
      * Check if an operation is authorised by asking the  configuration object about the access
-     * control rules granted to the current thread's {@link Principal}. If there is no current
+     * control rules granted to the current thread's {@link Subject}. If there is no current
      * user the plugin will abstain.
      */
     public Result authorise(Operation operation, ObjectType objectType, ObjectProperties properties)
     {
-        Principal principal = SecurityManager.getThreadPrincipal();
-        
-        // Abstain if there is no user associated with this thread
-        if (principal == null)
+        final Subject subject = SecurityManager.getThreadSubject();
+        // Abstain if there is no subject/principal associated with this thread
+        if (subject == null  || subject.getPrincipals().size() == 0)
         {
             return Result.ABSTAIN;
         }
-        
-        return _ruleSet.check(principal.getName(), operation, objectType, properties);
+ 
+        return  _ruleSet.check(subject, operation, objectType, properties);
     }
 
     public void configure(ConfigurationPlugin config)
diff --git a/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/AccessControlTest.java b/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/AccessControlTest.java
index 309a3aeb2c..09d26e5451 100644
--- a/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/AccessControlTest.java
+++ b/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/AccessControlTest.java
@@ -1,195 +1,172 @@
 /*
- *  Licensed to the Apache Software Foundation (ASF) under one
- *  or more contributor license agreements.  See the NOTICE file
- *  distributed with this work for additional information
- *  regarding copyright ownership.  The ASF licenses this file
- *  to you under the Apache License, Version 2.0 (the
- *  "License"); you may not use this file except in compliance
- *  with the License.  You may obtain a copy of the License at
  *
- *    http://www.apache.org/licenses/LICENSE-2.0
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
  *
- *  Unless required by applicable law or agreed to in writing,
- *  software distributed under the License is distributed on an
- *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- *  KIND, either express or implied.  See the License for the
- *  specific language governing permissions and limitations
- *  under the License.
  */
 package org.apache.qpid.server.security.access.plugins;
 
-import java.io.File;
-import java.io.FileNotFoundException;
-import java.io.FileWriter;
-import java.io.PrintWriter;
+import java.util.Arrays;
 
 import junit.framework.TestCase;
 
-import org.apache.commons.configuration.ConfigurationException;
-import org.apache.qpid.server.security.access.config.ConfigurationFile;
-import org.apache.qpid.server.security.access.config.PlainConfiguration;
+import org.apache.qpid.server.configuration.plugins.ConfigurationPlugin;
+import org.apache.qpid.server.logging.UnitTestMessageLogger;
+import org.apache.qpid.server.logging.actors.CurrentActor;
+import org.apache.qpid.server.logging.actors.TestLogActor;
+import org.apache.qpid.server.security.Result;
+import org.apache.qpid.server.security.SecurityManager;
+import org.apache.qpid.server.security.access.ObjectProperties;
+import org.apache.qpid.server.security.access.ObjectType;
+import org.apache.qpid.server.security.access.Operation;
+import org.apache.qpid.server.security.access.Permission;
+import org.apache.qpid.server.security.access.config.Rule;
 import org.apache.qpid.server.security.access.config.RuleSet;
+import org.apache.qpid.server.security.auth.sasl.TestPrincipalUtils;
 
 /**
- * These tests check that the ACL file parsing works correctly.
+ * Unit test for ACL V2 plugin.  
+ * 
+ * This unit test tests the AccessControl class and it collaboration with {@link RuleSet},
+ * {@link SecurityManager} and {@link CurrentActor}.   The ruleset is configured programmatically,
+ * rather than from an external file.
  * 
- * For each message that can be returned in a {@link ConfigurationException}, an ACL file is created that should trigger this
- * particular message.
+ * @see RuleSetTest
  */
 public class AccessControlTest extends TestCase
 {
-    public void writeACLConfig(String...aclData) throws Exception
+    private AccessControl _plugin = null;  // Class under test
+    private final UnitTestMessageLogger messageLogger = new UnitTestMessageLogger();
+
+    protected void setUp() throws Exception
     {
-        File acl = File.createTempFile(getClass().getName() + getName(), "acl");
-        acl.deleteOnExit();
-        
-        // Write ACL file
-        PrintWriter aclWriter = new PrintWriter(new FileWriter(acl));
-        for (String line : aclData)
-        {
-            aclWriter.println(line);
-        }
-        aclWriter.close();
+        super.setUp();
 
-        // Load ruleset
-        ConfigurationFile configFile = new PlainConfiguration(acl);
-        RuleSet ruleSet = configFile.load();
-    }
+        final RuleSet rs = new RuleSet();
+        rs.addGroup("aclGroup1", Arrays.asList(new String[] {"member1", "member2"}));
 
-    public void testMissingACLConfig() throws Exception
-    {
-        try
-        {
-            // Load ruleset
-	        ConfigurationFile configFile = new PlainConfiguration(new File("doesnotexist"));
-	        RuleSet ruleSet = configFile.load();
-            
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.CONFIG_NOT_FOUND_MSG, "doesnotexist"), ce.getMessage());
-            assertTrue(ce.getCause() instanceof FileNotFoundException);
-            assertEquals("doesnotexist (No such file or directory)", ce.getCause().getMessage());
-        }
-    }
+        // Rule expressed with username
+        rs.grant(0, "user1", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        // Rule expressed with a acl group
+        rs.grant(1, "aclGroup1", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        // Rule expressed with an external group
+        rs.grant(2, "extGroup1", Permission.DENY, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        // Catch all rule
+        rs.grant(3, Rule.ALL, Permission.DENY_LOG, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
 
-    public void testACLFileSyntaxContinuation() throws Exception
-    {
-        try
-        {
-            writeACLConfig("ACL ALLOW ALL \\ ALL");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.PREMATURE_CONTINUATION_MSG, 1), ce.getMessage());
-        }
-    }
+        _plugin = (AccessControl) AccessControl.FACTORY.newInstance(createConfiguration(rs));
 
-    public void testACLFileSyntaxTokens() throws Exception
-    {
-        try
-        {
-            writeACLConfig("ACL unparsed ALL ALL");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.PARSE_TOKEN_FAILED_MSG, 1), ce.getMessage());
-            assertTrue(ce.getCause() instanceof IllegalArgumentException);
-            assertEquals("Not a valid permission: unparsed", ce.getCause().getMessage());
-        }
+        SecurityManager.setThreadSubject(null);
+        
+        CurrentActor.set(new TestLogActor(messageLogger));
     }
 
-    public void testACLFileSyntaxNotEnoughGroup() throws Exception
+    protected void tearDown() throws Exception
     {
-        try
-        {
-            writeACLConfig("GROUP blah");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_GROUP_MSG, 1), ce.getMessage());
-        }
+        super.tearDown();
+        SecurityManager.setThreadSubject(null);
     }
 
-    public void testACLFileSyntaxNotEnoughACL() throws Exception
+    /** 
+     * ACL plugin must always abstain if there is no  subject attached to the thread.
+     */
+    public void testNoSubjectAlwaysAbstains()
     {
-        try
-        {
-            writeACLConfig("ACL ALLOW");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_ACL_MSG, 1), ce.getMessage());
-        }
+        SecurityManager.setThreadSubject(null);
+
+        final Result result = _plugin.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(Result.ABSTAIN, result);
     }
 
-    public void testACLFileSyntaxNotEnoughConfig() throws Exception
+    /** 
+     * Tests that an allow rule expressed with a username allows an operation performed by a thread running
+     * with the same username.
+     */
+    public void testUsernameAllowsOperation()
     {
-        try
-        {
-            writeACLConfig("CONFIG");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_TOKENS_MSG, 1), ce.getMessage());
-        }
+        SecurityManager.setThreadSubject(TestPrincipalUtils.createTestSubject("user1"));
+
+        final Result result = _plugin.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(Result.ALLOWED, result);
     }
 
-    public void testACLFileSyntaxNotEnough() throws Exception
+    /** 
+     * Tests that an allow rule expressed with an <b>ACL groupname</b> allows an operation performed by a thread running
+     * by a user who belongs to the same group..
+     */
+    public void testAclGroupMembershipAllowsOperation()
     {
-        try
-        {
-            writeACLConfig("INVALID");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_TOKENS_MSG, 1), ce.getMessage());
-        }
+        SecurityManager.setThreadSubject(TestPrincipalUtils.createTestSubject("member1"));
+
+        final Result result = _plugin.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(Result.ALLOWED, result);
     }
 
-    public void testACLFileSyntaxPropertyKeyOnly() throws Exception
+    /** 
+     * Tests that a deny rule expressed with an <b>External groupname</b> denies an operation performed by a thread running
+     * by a user who belongs to the same group.
+     */
+    public void testExternalGroupMembershipDeniesOperation()
     {
-        try
-        {
-            writeACLConfig("ACL ALLOW adk CREATE QUEUE name");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.PROPERTY_KEY_ONLY_MSG, 1), ce.getMessage());
-        }
+        SecurityManager.setThreadSubject(TestPrincipalUtils.createTestSubject("user3", "extGroup1"));
+        
+        final Result result = _plugin.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(Result.DENIED, result);
     }
 
-    public void testACLFileSyntaxPropertyNoEquals() throws Exception
+    /** 
+     * Tests that the catch all deny denies the operation and logs with the logging actor.
+     */
+    public void testCatchAllRuleDeniesUnrecognisedUsername()
     {
-        try
-        {
-            writeACLConfig("ACL ALLOW adk CREATE QUEUE name test");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.PROPERTY_NO_EQUALS_MSG, 1), ce.getMessage());
-        }
+        SecurityManager.setThreadSubject(TestPrincipalUtils.createTestSubject("unknown", "unkgroup1", "unkgroup2"));
+        
+        assertEquals("Expecting zero messages before test", 0, messageLogger.getLogMessages().size());
+        final Result result = _plugin.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(Result.DENIED, result);
+        
+        assertEquals("Expecting one message before test", 1, messageLogger.getLogMessages().size());
+        assertTrue("Logged message does not contain expected string", messageLogger.messageContains(0, "ACL-1002"));
     }
-
-    public void testACLFileSyntaxPropertyNoValue() throws Exception
+    
+    /**
+     * Creates a configuration plugin for the {@link AccessControl} plugin.
+     */
+    private ConfigurationPlugin createConfiguration(final RuleSet rs)
     {
-        try
-        {
-            writeACLConfig("ACL ALLOW adk CREATE QUEUE name =");
-            fail("fail");
-        }
-        catch (ConfigurationException ce)
-        {
-            assertEquals(String.format(PlainConfiguration.PROPERTY_NO_VALUE_MSG, 1), ce.getMessage());
-        }
+        final ConfigurationPlugin cp = new ConfigurationPlugin()
+        {
+            public AccessControlConfiguration  getConfiguration(final String plugin)
+            {
+                return new AccessControlConfiguration()
+                {
+                    public RuleSet getRuleSet()
+                    {
+                        return rs;
+                    }
+                };
+            }
+
+            public String[] getElementsProcessed()
+            {
+                throw new UnsupportedOperationException();
+            }
+        };
+
+        return cp;
     }
 }
diff --git a/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java b/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java
new file mode 100644
index 0000000000..e16f9943ba
--- /dev/null
+++ b/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java
@@ -0,0 +1,194 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.apache.qpid.server.security.access.plugins;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileWriter;
+import java.io.PrintWriter;
+
+import junit.framework.TestCase;
+
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.qpid.server.security.access.config.ConfigurationFile;
+import org.apache.qpid.server.security.access.config.PlainConfiguration;
+
+/**
+ * These tests check that the ACL file parsing works correctly.
+ * 
+ * For each message that can be returned in a {@link ConfigurationException}, an ACL file is created that should trigger this
+ * particular message.
+ */
+public class PlainConfigurationTest extends TestCase
+{
+    public void writeACLConfig(String...aclData) throws Exception
+    {
+        File acl = File.createTempFile(getClass().getName() + getName(), "acl");
+        acl.deleteOnExit();
+        
+        // Write ACL file
+        PrintWriter aclWriter = new PrintWriter(new FileWriter(acl));
+        for (String line : aclData)
+        {
+            aclWriter.println(line);
+        }
+        aclWriter.close();
+
+        // Load ruleset
+        ConfigurationFile configFile = new PlainConfiguration(acl);
+        configFile.load();
+    }
+
+    public void testMissingACLConfig() throws Exception
+    {
+        try
+        {
+            // Load ruleset
+            ConfigurationFile configFile = new PlainConfiguration(new File("doesnotexist"));
+            configFile.load();
+            
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.CONFIG_NOT_FOUND_MSG, "doesnotexist"), ce.getMessage());
+            assertTrue(ce.getCause() instanceof FileNotFoundException);
+            assertEquals("doesnotexist (No such file or directory)", ce.getCause().getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxContinuation() throws Exception
+    {
+        try
+        {
+            writeACLConfig("ACL ALLOW ALL \\ ALL");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.PREMATURE_CONTINUATION_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxTokens() throws Exception
+    {
+        try
+        {
+            writeACLConfig("ACL unparsed ALL ALL");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.PARSE_TOKEN_FAILED_MSG, 1), ce.getMessage());
+            assertTrue(ce.getCause() instanceof IllegalArgumentException);
+            assertEquals("Not a valid permission: unparsed", ce.getCause().getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxNotEnoughGroup() throws Exception
+    {
+        try
+        {
+            writeACLConfig("GROUP blah");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_GROUP_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxNotEnoughACL() throws Exception
+    {
+        try
+        {
+            writeACLConfig("ACL ALLOW");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_ACL_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxNotEnoughConfig() throws Exception
+    {
+        try
+        {
+            writeACLConfig("CONFIG");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_TOKENS_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxNotEnough() throws Exception
+    {
+        try
+        {
+            writeACLConfig("INVALID");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.NOT_ENOUGH_TOKENS_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxPropertyKeyOnly() throws Exception
+    {
+        try
+        {
+            writeACLConfig("ACL ALLOW adk CREATE QUEUE name");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.PROPERTY_KEY_ONLY_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxPropertyNoEquals() throws Exception
+    {
+        try
+        {
+            writeACLConfig("ACL ALLOW adk CREATE QUEUE name test");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.PROPERTY_NO_EQUALS_MSG, 1), ce.getMessage());
+        }
+    }
+
+    public void testACLFileSyntaxPropertyNoValue() throws Exception
+    {
+        try
+        {
+            writeACLConfig("ACL ALLOW adk CREATE QUEUE name =");
+            fail("fail");
+        }
+        catch (ConfigurationException ce)
+        {
+            assertEquals(String.format(PlainConfiguration.PROPERTY_NO_VALUE_MSG, 1), ce.getMessage());
+        }
+    }
+}
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 aad7290557..bd9deac153 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
@@ -21,13 +21,21 @@
 
 package org.apache.qpid.server.security.access.plugins;
 
+import java.security.Principal;
+import java.util.Arrays;
+
+import javax.security.auth.Subject;
+
 import org.apache.qpid.framing.AMQShortString;
 import org.apache.qpid.server.security.Result;
 import org.apache.qpid.server.security.access.ObjectProperties;
 import org.apache.qpid.server.security.access.ObjectType;
 import org.apache.qpid.server.security.access.Operation;
 import org.apache.qpid.server.security.access.Permission;
+import org.apache.qpid.server.security.access.config.Rule;
 import org.apache.qpid.server.security.access.config.RuleSet;
+import org.apache.qpid.server.security.auth.sasl.TestPrincipalUtils;
+import org.apache.qpid.server.security.auth.sasl.UsernamePrincipal;
 import org.apache.qpid.test.utils.QpidTestCase;
 
 /**
@@ -36,16 +44,24 @@ import org.apache.qpid.test.utils.QpidTestCase;
  * The ruleset is configured directly rather than using an external file by adding rules individually, calling the
  * {@link RuleSet#grant(Integer, String, Permission, Operation, ObjectType, ObjectProperties)} method. Then, the
  * access control mechanism is validated by checking whether operations would be authorised by calling the
- * {@link RuleSet#check(String, Operation, ObjectType, ObjectProperties)} method.
+ * {@link RuleSet#check(Principal, Operation, ObjectType, ObjectProperties)} method.
+ * 
+ * It ensure that permissions can be granted correctly on users directly, ACL groups (that is those
+ * groups declared directly in the ACL itself), and External groups (that is a group from an External
+ * Authentication Provider, such as an LDAP).
+
  */
 public class RuleSetTest extends QpidTestCase
 {
-    private RuleSet _ruleSet;
+    private RuleSet _ruleSet; // Object under test
+
+    private static final String TEST_USER = "user";
 
     // Common things that are passed to frame constructors
     private AMQShortString _queueName = new AMQShortString(this.getClass().getName() + "queue");
     private AMQShortString _exchangeName = new AMQShortString("amq.direct");
     private AMQShortString _exchangeType = new AMQShortString("direct");
+    private Subject _testSubject = TestPrincipalUtils.createTestSubject(TEST_USER);
 
     @Override
     public void setUp() throws Exception
@@ -63,34 +79,36 @@ public class RuleSetTest extends QpidTestCase
         super.tearDown();
     }
 
-    public void assertDenyGrantAllow(String identity, Operation operation, ObjectType objectType)
+    public void assertDenyGrantAllow(Subject subject, Operation operation, ObjectType objectType)
     {
-        assertDenyGrantAllow(identity, operation, objectType, ObjectProperties.EMPTY);
+        assertDenyGrantAllow(subject, operation, objectType, ObjectProperties.EMPTY);
     }
     
-    public void assertDenyGrantAllow(String identity, Operation operation, ObjectType objectType, ObjectProperties properties)
+    public void assertDenyGrantAllow(Subject subject, Operation operation, ObjectType objectType, ObjectProperties properties)
     {
-        assertEquals(Result.DENIED, _ruleSet.check(identity, operation, objectType, properties));
-        _ruleSet.grant(0, identity, Permission.ALLOW, operation, objectType, properties);
+        final Principal identity = UsernamePrincipal.getUsernamePrincipalFromSubject(subject);
+        
+        assertEquals(Result.DENIED, _ruleSet.check(subject, operation, objectType, properties));
+        _ruleSet.grant(0, identity.getName(), Permission.ALLOW, operation, objectType, properties);
         assertEquals(1, _ruleSet.getRuleCount());
-        assertEquals(Result.ALLOWED, _ruleSet.check(identity, operation, objectType, properties));
+        assertEquals(Result.ALLOWED, _ruleSet.check(subject, operation, objectType, properties));
     }
 
     public void testEmptyRuleSet()
     {
         assertNotNull(_ruleSet);
         assertEquals(_ruleSet.getRuleCount(), 0);
-        assertEquals(_ruleSet.getDefault(), _ruleSet.check("user", Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+        assertEquals(_ruleSet.getDefault(), _ruleSet.check(_testSubject, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
     }
     
     public void testVirtualHostAccess() throws Exception
     {
-        assertDenyGrantAllow("user", Operation.ACCESS, ObjectType.VIRTUALHOST);
+        assertDenyGrantAllow(_testSubject, Operation.ACCESS, ObjectType.VIRTUALHOST);
     }
 
     public void testQueueCreateNamed() throws Exception
     {
-        assertDenyGrantAllow("user", Operation.CREATE, ObjectType.QUEUE, new ObjectProperties(_queueName));
+        assertDenyGrantAllow(_testSubject, Operation.CREATE, ObjectType.QUEUE, new ObjectProperties(_queueName));
     }
 
     public void testQueueCreatenamedNullRoutingKey()
@@ -98,7 +116,7 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties properties = new ObjectProperties(_queueName);
         properties.put(ObjectProperties.Property.ROUTING_KEY, (String) null);
         
-        assertDenyGrantAllow("user", Operation.CREATE, ObjectType.QUEUE, properties);
+        assertDenyGrantAllow(_testSubject, Operation.CREATE, ObjectType.QUEUE, properties);
     }
 
     public void testExchangeCreate()
@@ -106,17 +124,17 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties properties = new ObjectProperties(_exchangeName);
         properties.put(ObjectProperties.Property.TYPE, _exchangeType.asString());
         
-        assertDenyGrantAllow("user", Operation.CREATE, ObjectType.EXCHANGE, properties);
+        assertDenyGrantAllow(_testSubject, Operation.CREATE, ObjectType.EXCHANGE, properties);
     }
 
     public void testConsume()
     {
-        assertDenyGrantAllow("user", Operation.CONSUME, ObjectType.QUEUE);
+        assertDenyGrantAllow(_testSubject, Operation.CONSUME, ObjectType.QUEUE);
     }
 
     public void testPublish()
     {
-        assertDenyGrantAllow("user", Operation.PUBLISH, ObjectType.EXCHANGE);
+        assertDenyGrantAllow(_testSubject, Operation.PUBLISH, ObjectType.EXCHANGE);
     }
 
     /**
@@ -131,13 +149,13 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties normal = new ObjectProperties();
         normal.put(ObjectProperties.Property.AUTO_DELETE, Boolean.FALSE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, temporary));
-        _ruleSet.grant(0, "user", Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, temporary);
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, temporary));
+        _ruleSet.grant(0, TEST_USER, Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, temporary);
         assertEquals(1, _ruleSet.getRuleCount());
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, temporary));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, temporary));
         
         // defer to global if exists, otherwise default answer - this is handled by the security manager
-        assertEquals(Result.DEFER, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, normal));
+        assertEquals(Result.DEFER, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, normal));
     }
 
     /**
@@ -151,15 +169,15 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties normal = new ObjectProperties(_queueName);
         normal.put(ObjectProperties.Property.AUTO_DELETE, Boolean.FALSE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, temporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, temporary));
 
         // should not matter if the temporary permission is processed first or last
-        _ruleSet.grant(1, "user", Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, normal);
-        _ruleSet.grant(2, "user", Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, temporary);
+        _ruleSet.grant(1, TEST_USER, Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, normal);
+        _ruleSet.grant(2, TEST_USER, Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, temporary);
         assertEquals(2, _ruleSet.getRuleCount());
         
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, normal));
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, temporary));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, normal));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, temporary));
     }
 
     /**
@@ -173,15 +191,15 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties normal = new ObjectProperties(_queueName);
         normal.put(ObjectProperties.Property.AUTO_DELETE, Boolean.FALSE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, temporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, temporary));
 
         // should not matter if the temporary permission is processed first or last
-        _ruleSet.grant(1, "user", Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, temporary);
-        _ruleSet.grant(2, "user", Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, normal);
+        _ruleSet.grant(1, TEST_USER, Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, temporary);
+        _ruleSet.grant(2, TEST_USER, Permission.ALLOW, Operation.CONSUME, ObjectType.QUEUE, normal);
         assertEquals(2, _ruleSet.getRuleCount());
         
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, normal));
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CONSUME, ObjectType.QUEUE, temporary));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, normal));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CONSUME, ObjectType.QUEUE, temporary));
     }
 
     /*
@@ -197,15 +215,15 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties namedTemporary = new ObjectProperties(_queueName);
         namedTemporary.put(ObjectProperties.Property.AUTO_DELETE, Boolean.TRUE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
 
-        _ruleSet.grant(1, "user", Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
-        _ruleSet.grant(2, "user", Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
+        _ruleSet.grant(1, TEST_USER, Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
+        _ruleSet.grant(2, TEST_USER, Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
         assertEquals(2, _ruleSet.getRuleCount());
         
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
     }
     
     /**
@@ -217,15 +235,15 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties namedTemporary = new ObjectProperties(_queueName);
         namedTemporary.put(ObjectProperties.Property.AUTO_DELETE, Boolean.TRUE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
 
-        _ruleSet.grant(1, "user", Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
-        _ruleSet.grant(2, "user", Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
+        _ruleSet.grant(1, TEST_USER, Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
+        _ruleSet.grant(2, TEST_USER, Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
         assertEquals(2, _ruleSet.getRuleCount());
         
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
     }
     
     /**
@@ -239,18 +257,18 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties namedDurable = new ObjectProperties(_queueName);
         namedDurable.put(ObjectProperties.Property.DURABLE, Boolean.TRUE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedDurable));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedDurable));
 
-        _ruleSet.grant(1, "user", Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
-        _ruleSet.grant(2, "user", Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedDurable);
-        _ruleSet.grant(3, "user", Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
+        _ruleSet.grant(1, TEST_USER, Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
+        _ruleSet.grant(2, TEST_USER, Permission.DENY, Operation.CREATE, ObjectType.QUEUE, namedDurable);
+        _ruleSet.grant(3, TEST_USER, Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
         assertEquals(3, _ruleSet.getRuleCount());
         
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedDurable));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedDurable));
     }
     
     public void testNamedTemporaryQueueAllowed()
@@ -259,15 +277,15 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties namedTemporary = new ObjectProperties(_queueName);
         namedTemporary.put(ObjectProperties.Property.AUTO_DELETE, Boolean.TRUE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
 
-        _ruleSet.grant(1, "user", Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
-        _ruleSet.grant(2, "user", Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
+        _ruleSet.grant(1, TEST_USER, Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
+        _ruleSet.grant(2, TEST_USER, Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, named);
         assertEquals(2, _ruleSet.getRuleCount());
         
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
     }
     
     public void testNamedTemporaryQueueDeniedAllowed()
@@ -276,14 +294,101 @@ public class RuleSetTest extends QpidTestCase
         ObjectProperties namedTemporary = new ObjectProperties(_queueName);
         namedTemporary.put(ObjectProperties.Property.AUTO_DELETE, Boolean.TRUE);
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
 
-        _ruleSet.grant(1, "user", Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
-        _ruleSet.grant(2, "user", Permission.DENY, Operation.CREATE, ObjectType.QUEUE, named);
+        _ruleSet.grant(1, TEST_USER, Permission.ALLOW, Operation.CREATE, ObjectType.QUEUE, namedTemporary);
+        _ruleSet.grant(2, TEST_USER, Permission.DENY, Operation.CREATE, ObjectType.QUEUE, named);
         assertEquals(2, _ruleSet.getRuleCount());
         
-        assertEquals(Result.DENIED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, named));
-        assertEquals(Result.ALLOWED, _ruleSet.check("user", Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+        assertEquals(Result.DENIED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, named));
+        assertEquals(Result.ALLOWED, _ruleSet.check(_testSubject, Operation.CREATE, ObjectType.QUEUE, namedTemporary));
+    }
+
+    /** 
+     * Tests support for the {@link Rule#ALL} keyword. 
+     */
+    public void testAllowToAll()
+    {
+        _ruleSet.grant(1, Rule.ALL, Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(1, _ruleSet.getRuleCount());
+
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("usera"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("userb"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+    }
+
+    /** 
+     * Tests support for ACL groups (i.e. inline groups declared in the ACL file itself).
+     */
+    public void testAclGroupsSupported()
+    {
+        assertTrue(_ruleSet.addGroup("aclgroup", Arrays.asList(new String[] {"usera", "userb"})));  
+        
+        _ruleSet.grant(1, "aclgroup", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(1, _ruleSet.getRuleCount());
+
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("usera"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("userb"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+        assertEquals(Result.DEFER, _ruleSet.check(TestPrincipalUtils.createTestSubject("userc"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+    }
+
+    /** 
+     * Tests support for nested ACL groups.
+     */
+    public void testNestedAclGroupsSupported()
+    {
+        assertTrue(_ruleSet.addGroup("aclgroup1", Arrays.asList(new String[] {"userb"})));
+        assertTrue(_ruleSet.addGroup("aclgroup2", Arrays.asList(new String[] {"usera", "aclgroup1"})));  
+        
+        _ruleSet.grant(1, "aclgroup2", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(1, _ruleSet.getRuleCount());
+
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("usera"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("userb"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+    }
+
+    /** 
+     * Tests support for nested External groups (i.e. those groups coming from an external source such as an LDAP).
+     */
+    public void testExternalGroupsSupported()
+    {
+        _ruleSet.grant(1, "extgroup1", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        _ruleSet.grant(2, "extgroup2", Permission.DENY, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(2, _ruleSet.getRuleCount());
+
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("usera", "extgroup1"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+        assertEquals(Result.DENIED, _ruleSet.check(TestPrincipalUtils.createTestSubject("userb", "extgroup2"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+    }
+    
+    /**
+     * Rule order in the ACL determines the outcome of the check.  This test ensures that a user who is
+     * granted explicit permission on an object, is granted that access even although late a group
+     * to which the user belongs is later denied the permission.
+     */
+    public void testAllowDeterminedByRuleOrder()
+    {
+        assertTrue(_ruleSet.addGroup("aclgroup", Arrays.asList(new String[] {"usera"})));
+        
+        _ruleSet.grant(1, "usera", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        _ruleSet.grant(2, "aclgroup", Permission.DENY, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        assertEquals(2, _ruleSet.getRuleCount());
+
+        assertEquals(Result.ALLOWED, _ruleSet.check(TestPrincipalUtils.createTestSubject("usera"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
+    }
+
+    /**
+     * Rule order in the ACL determines the outcome of the check.  This tests ensures that a user who is denied
+     * access by group, is denied access, despite there being a later rule granting permission to that user.
+     */
+    public void testDenyDeterminedByRuleOrder()
+    {
+        assertTrue(_ruleSet.addGroup("aclgroup", Arrays.asList(new String[] {"usera"})));
+        
+        _ruleSet.grant(1, "aclgroup", Permission.DENY, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        _ruleSet.grant(2, "usera", Permission.ALLOW, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
+        
+        assertEquals(2, _ruleSet.getRuleCount());
+
+        assertEquals(Result.DENIED, _ruleSet.check(TestPrincipalUtils.createTestSubject("usera"),Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY));
     }
 }