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@1205782 13f79535-47bb-0310-9956-ffa450edef68

8 files changed, 144 insertions, 108 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/binding/BindingFactory.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/binding/BindingFactory.java
index 400ce50bc4..94ab43c851 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/binding/BindingFactory.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/binding/BindingFactory.java
@@ -170,11 +170,17 @@ public class BindingFactory
         {
             arguments = Collections.emptyMap();
         }
-      
-        //Perform ACLs
-        if (!getVirtualHost().getSecurityManager().authoriseBind(exchange, queue, new AMQShortString(bindingKey)))
+
+        // The default exchange bindings must reflect the existence of queues, allow
+        // all operations on it to succeed. It is up to the broker to prevent illegal
+        // attempts at binding to this exchange, not the ACLs.
+        if(exchange != _defaultExchange)
         {
-            throw new AMQSecurityException("Permission denied: binding " + bindingKey);
+            //Perform ACLs
+            if (!getVirtualHost().getSecurityManager().authoriseBind(exchange, queue, new AMQShortString(bindingKey)))
+            {
+                throw new AMQSecurityException("Permission denied: binding " + bindingKey);
+            }
         }
         
         BindingImpl b = new BindingImpl(bindingKey,queue,exchange,arguments);
@@ -238,10 +244,16 @@ public class BindingFactory
             arguments = Collections.emptyMap();
         }
 
-        // Check access
-        if (!getVirtualHost().getSecurityManager().authoriseUnbind(exchange, new AMQShortString(bindingKey), queue))
+        // The default exchange bindings must reflect the existence of queues, allow
+        // all operations on it to succeed. It is up to the broker to prevent illegal
+        // attempts at binding to this exchange, not the ACLs.
+        if(exchange != _defaultExchange)
         {
-            throw new AMQSecurityException("Permission denied: binding " + bindingKey);
+            // Check access
+            if (!getVirtualHost().getSecurityManager().authoriseUnbind(exchange, new AMQShortString(bindingKey), queue))
+            {
+                throw new AMQSecurityException("Permission denied: unbinding " + bindingKey);
+            }
         }
         
         BindingImpl b = _bindings.remove(new BindingImpl(bindingKey,queue,exchange,arguments));
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java
index 0d347873c2..e13e2f4d8f 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java
@@ -805,4 +805,9 @@ public class ServerConfiguration extends ConfigurationPlugin
         final List<String> disabledFeatures = getListValue("disabledFeatures", Collections.emptyList());
         return disabledFeatures;
     }
+
+    public boolean getManagementRightsInferAllAccess()
+    {
+        return getBooleanValue("management.managementRightsInferAllAccess", true);
+    }
 }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java
index 169195304c..0dd8c95ef3 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java
@@ -33,6 +33,7 @@ import javax.management.JMException;
 import javax.management.MBeanInfo;
 import javax.management.MBeanOperationInfo;
 import javax.management.MBeanServer;
+import javax.management.MalformedObjectNameException;
 import javax.management.Notification;
 import javax.management.NotificationListener;
 import javax.management.ObjectName;
@@ -45,6 +46,7 @@ import org.apache.log4j.Logger;
 import org.apache.qpid.server.logging.actors.ManagementActor;
 import org.apache.qpid.server.logging.messages.ManagementConsoleMessages;
 import org.apache.qpid.server.registry.ApplicationRegistry;
+import org.apache.qpid.server.registry.IApplicationRegistry;
 import org.apache.qpid.server.security.SecurityManager;
 import org.apache.qpid.server.security.access.Operation;
 
@@ -56,22 +58,54 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler, Notificati
 {
     private static final Logger _logger = Logger.getLogger(MBeanInvocationHandlerImpl.class);
 
+    private final IApplicationRegistry _appRegistry = ApplicationRegistry.getInstance();
     private final static String DELEGATE = "JMImplementation:type=MBeanServerDelegate";
     private MBeanServer _mbs;
-    private static ManagementActor  _logActor;
-    
+    private final ManagementActor _logActor = new ManagementActor(_appRegistry.getRootMessageLogger());
+    private final boolean _managementRightsInferAllAccess =
+        _appRegistry.getConfiguration().getManagementRightsInferAllAccess();
+
     public static MBeanServerForwarder newProxyInstance()
     {
         final InvocationHandler handler = new MBeanInvocationHandlerImpl();
         final Class<?>[] interfaces = new Class[] { MBeanServerForwarder.class };
 
-
-        _logActor = new ManagementActor(ApplicationRegistry.getInstance().getRootMessageLogger());
-
         Object proxy = Proxy.newProxyInstance(MBeanServerForwarder.class.getClassLoader(), interfaces, handler);
         return MBeanServerForwarder.class.cast(proxy);
     }
 
+    private boolean invokeDirectly(String methodName, Object[] args, Subject subject)
+    {
+        // Allow operations performed locally on behalf of the connector server itself
+        if (subject == null)
+        {
+            return true;
+        }
+
+        if (args == null || DELEGATE.equals(args[0]))
+        {
+            return true;
+        }
+
+        // Allow querying available object names
+        if (methodName.equals("queryNames"))
+        {
+            return true;
+        }
+
+        if (args[0] instanceof ObjectName)
+        {
+            ObjectName mbean = (ObjectName) args[0];
+
+            if(!DefaultManagedObject.DOMAIN.equalsIgnoreCase(mbean.getDomain()))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     public Object invoke(Object proxy, Method method, Object[] args) throws Throwable
     {
         final String methodName = getMethodName(method, args);
@@ -95,36 +129,24 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler, Notificati
             return null;
         }
 
+        // Restrict access to "createMBean" and "unregisterMBean" to any user
+        if (methodName.equals("createMBean") || methodName.equals("unregisterMBean"))
+        {
+            _logger.debug("User trying to create or unregister an MBean");
+            throw new SecurityException("Access denied: " + methodName);
+        }
+
         // Retrieve Subject from current AccessControlContext
         AccessControlContext acc = AccessController.getContext();
         Subject subject = Subject.getSubject(acc);
 
         try
         {
-            // Allow operations performed locally on behalf of the connector server itself
-            if (subject == null)
+            if(invokeDirectly(methodName, args, subject))
             {
                 return method.invoke(_mbs, args);
             }
-    
-            if (args == null || DELEGATE.equals(args[0]))
-            {
-                return method.invoke(_mbs, args);
-            }
-    
-            // Restrict access to "createMBean" and "unregisterMBean" to any user
-            if (methodName.equals("createMBean") || methodName.equals("unregisterMBean"))
-            {
-                _logger.debug("User trying to create or unregister an MBean");
-                throw new SecurityException("Access denied: " + methodName);
-            }
-    
-            // Allow querying available object names
-            if (methodName.equals("queryNames"))
-            {
-                return method.invoke(_mbs, args);
-            }
-    
+
             // Retrieve JMXPrincipal from Subject
             Set<JMXPrincipal> principals = subject.getPrincipals(JMXPrincipal.class);
             if (principals == null || principals.isEmpty())
@@ -134,23 +156,23 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler, Notificati
 
             // Save the subject
             SecurityManager.setThreadSubject(subject);
-   
+
             // Get the component, type and impact, which may be null
             String type = getType(method, args);
             String vhost = getVirtualHost(method, args);
             int impact = getImpact(method, args);
-            
+
             // Get the security manager for the virtual host (if set)
             SecurityManager security;
             if (vhost == null)
             {
-                security = ApplicationRegistry.getInstance().getSecurityManager();
+                security = _appRegistry.getSecurityManager();
             }
             else
             {
-                security = ApplicationRegistry.getInstance().getVirtualHostRegistry().getVirtualHost(vhost).getSecurityManager();
+                security = _appRegistry.getVirtualHostRegistry().getVirtualHost(vhost).getSecurityManager();
             }
-            
+
 			if (isAccessMethod(methodName) || impact == MBeanOperationInfo.INFO)
 			{
 				// Check for read-only method invocation permission
@@ -159,25 +181,33 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler, Notificati
                     throw new SecurityException("Permission denied: Access " + methodName);
                 }
 			}
-			else if (isUpdateMethod(methodName))
-            {
-	            // Check for setting properties permission
-                if (!security.authoriseMethod(Operation.UPDATE, type, methodName))
-                {
-                    throw new SecurityException("Permission denied: Update " + methodName);
-                }
-            }
-			else 
-            {
-	            // Check for invoking/executing method action/operation permission
-                if (!security.authoriseMethod(Operation.EXECUTE, type, methodName))
-                {
-                    throw new SecurityException("Permission denied: Execute " + methodName);
-                }
-            }
-			
-			// Actually invoke the method
-			return method.invoke(_mbs, args);
+			else
+			{
+			    // Check for setting properties permission
+			    if (!security.authoriseMethod(Operation.UPDATE, type, methodName))
+			    {
+			        throw new SecurityException("Permission denied: Update " + methodName);
+			    }
+			}
+
+			boolean oldAccessChecksDisabled = false;
+			if(_managementRightsInferAllAccess)
+			{
+			    oldAccessChecksDisabled = SecurityManager.setAccessChecksDisabled(true);
+			}
+
+			try
+			{
+			    // Actually invoke the method
+			    return method.invoke(_mbs, args);
+			}
+			finally
+			{
+			    if(_managementRightsInferAllAccess)
+			    {
+			        SecurityManager.setAccessChecksDisabled(oldAccessChecksDisabled);
+			    }
+			}
         }
         catch (InvocationTargetException e)
         {
@@ -290,13 +320,6 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler, Notificati
         return (methodName.startsWith("query") || methodName.startsWith("get") || methodName.startsWith("is"));
     }
 
-
-    private boolean isUpdateMethod(String methodName)
-    {
-        //handle standard set methods from MBeanServer
-        return methodName.startsWith("set");
-    }
-
     public void handleNotification(Notification notification, Object handback)
     {
         assert notification instanceof JMXConnectionNotification;
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/AbstractProxyPlugin.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/AbstractProxyPlugin.java
index 8b5ff6781d..ec11e2d39c 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/AbstractProxyPlugin.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/AbstractProxyPlugin.java
@@ -73,11 +73,6 @@ public abstract class AbstractProxyPlugin extends AbstractPlugin
         return getDefault();
     }
     
-    public Result authoriseExecute(ObjectType object, ObjectProperties properties)
-    {
-        return getDefault();
-    }
-    
     public Result authoriseUpdate(ObjectType object, ObjectProperties properties)
     {
         return getDefault();
@@ -121,8 +116,6 @@ public abstract class AbstractProxyPlugin extends AbstractPlugin
 				return authoriseDelete(objectType, properties);
 			case PURGE:
 				return authorisePurge(objectType, properties);
-			case EXECUTE:
-				return authoriseExecute(objectType, properties);
 			case UPDATE:
 				return authoriseUpdate(objectType, properties);
 		}
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java
index f582fed6a0..abf9e3379d 100755
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java
@@ -20,10 +20,8 @@ package org.apache.qpid.server.security;
 
 import static org.apache.qpid.server.security.access.ObjectType.EXCHANGE;
 import static org.apache.qpid.server.security.access.ObjectType.METHOD;
-import static org.apache.qpid.server.security.access.ObjectType.OBJECT;
 import static org.apache.qpid.server.security.access.ObjectType.QUEUE;
 import static org.apache.qpid.server.security.access.ObjectType.VIRTUALHOST;
-import static org.apache.qpid.server.security.access.Operation.ACCESS;
 import static org.apache.qpid.server.security.access.Operation.BIND;
 import static org.apache.qpid.server.security.access.Operation.CONSUME;
 import static org.apache.qpid.server.security.access.Operation.CREATE;
@@ -67,7 +65,14 @@ public class SecurityManager
     
     /** Container for the {@link Principal} that is using to this thread. */
     private static final ThreadLocal<Subject> _subject = new ThreadLocal<Subject>();
-    
+    private static final ThreadLocal<Boolean> _accessChecksDisabled = new ThreadLocal<Boolean>()
+    {
+        protected Boolean initialValue()
+        {
+            return false;
+        }
+    };
+
     private PluginManager _pluginManager;
     private Map<String, SecurityPluginFactory> _pluginFactories = new HashMap<String, SecurityPluginFactory>();
     private Map<String, SecurityPlugin> _globalPlugins = new HashMap<String, SecurityPlugin>();
@@ -194,6 +199,11 @@ public class SecurityManager
 
     private boolean checkAllPlugins(AccessCheck checker)
     {
+        if(_accessChecksDisabled.get())
+        {
+            return true;
+        }
+
         HashMap<String, SecurityPlugin> remainingPlugins = new HashMap<String, SecurityPlugin>(_globalPlugins);
 		
 		for (Entry<String, SecurityPlugin> hostEntry : _hostPlugins.entrySet())
@@ -273,21 +283,6 @@ public class SecurityManager
             }
         });
     }
-    
-    // TODO not implemented yet, awaiting consensus
-    public boolean authoriseObject(final String packageName, final String className)
-    {
-        return checkAllPlugins(new AccessCheck()
-        {
-            Result allowed(SecurityPlugin plugin)
-            {
-                ObjectProperties properties = new ObjectProperties();
-                properties.put(ObjectProperties.Property.PACKAGE, packageName);
-                properties.put(ObjectProperties.Property.CLASS, className);
-                return plugin.authorise(ACCESS, OBJECT, properties);
-            }
-        });
-    }
 
     public boolean authoriseMethod(final Operation operation, final String componentName, final String methodName)
     {
@@ -329,17 +324,6 @@ public class SecurityManager
         });
     }
 
-    public boolean authoriseConsume(final boolean exclusive, final boolean noAck, final boolean noLocal, final boolean nowait, final AMQQueue queue)
-    {
-        return checkAllPlugins(new AccessCheck()
-        {
-            Result allowed(SecurityPlugin plugin)
-            {
-                return plugin.authorise(CONSUME, QUEUE, new ObjectProperties(exclusive, noAck, noLocal, nowait, queue));
-            }
-        });
-    }
-
     public boolean authoriseCreateExchange(final Boolean autoDelete, final Boolean durable, final AMQShortString exchangeName,
             final Boolean internal, final Boolean nowait, final Boolean passive, final AMQShortString exchangeType)
     {
@@ -419,4 +403,14 @@ public class SecurityManager
             }
         });
     }
+
+    public static boolean setAccessChecksDisabled(final boolean status)
+    {
+        //remember current value
+        boolean current = _accessChecksDisabled.get();
+
+        _accessChecksDisabled.set(status);
+
+        return current;
+    }
 }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/ObjectType.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/ObjectType.java
index 7103b30283..69c7ff185a 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/ObjectType.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/ObjectType.java
@@ -32,14 +32,12 @@ import java.util.Set;
 public enum ObjectType
 {
     ALL(Operation.ALL),
-    VIRTUALHOST(ACCESS),
-    QUEUE(CREATE, DELETE, PURGE, CONSUME),
-    TOPIC(CREATE, DELETE, PURGE, CONSUME),
-    EXCHANGE(ACCESS, CREATE, DELETE, BIND, UNBIND, PUBLISH),
+    VIRTUALHOST(Operation.ALL, ACCESS),
+    QUEUE(Operation.ALL, CREATE, DELETE, PURGE, CONSUME),
+    EXCHANGE(Operation.ALL, ACCESS, CREATE, DELETE, BIND, UNBIND, PUBLISH),
     LINK, // Not allowed in the Java broker
     ROUTE, // Not allowed in the Java broker
-    METHOD(Operation.ALL, ACCESS, UPDATE, EXECUTE),
-    OBJECT(ACCESS);
+    METHOD(Operation.ALL, ACCESS, UPDATE);
     
     private EnumSet<Operation> _actions;
     
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/Operation.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/Operation.java
index 06d7f8fd0c..21ea042eed 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/Operation.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/Operation.java
@@ -32,8 +32,7 @@ public enum Operation
     UNBIND,
     DELETE,
     PURGE,
-    UPDATE,
-    EXECUTE;
+    UPDATE;
     
     public static Operation parse(String text)
     {
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/ServerConfigurationTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/ServerConfigurationTest.java
index 9afd2a45a9..9ee2ed3812 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/ServerConfigurationTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/ServerConfigurationTest.java
@@ -303,6 +303,18 @@ public class ServerConfigurationTest extends QpidTestCase
         assertEquals(false, _serverConfig.getManagementEnabled());
     }
 
+    public void testGetManagementRightsInferAllAccess() throws Exception
+    {
+        _serverConfig.initialise();
+
+        //check default
+        assertTrue("default should be true", _serverConfig.getManagementRightsInferAllAccess());
+
+        //update it
+        _config.setProperty("management.managementRightsInferAllAccess", "false");
+        assertFalse("New value should be false", _serverConfig.getManagementRightsInferAllAccess());
+    }
+
     public void testGetHeartBeatDelay() throws ConfigurationException
     {
         // Check default