QPID-4186: MBeanInvocationHandler now sets log actor before performing authorisation, so that authorisation logging now includes principal name.

Applied patch from Philip Harvey <phil@philharveyonline.com> and Oleksandr Rudyy<orudyy@gmail.com>

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

2 files changed, 219 insertions, 51 deletions

diff --git a/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java b/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java
index 49f06d5121..ce4e75f8c2 100644
--- a/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java
+++ b/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java
@@ -157,57 +157,14 @@ 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 = _appRegistry.getSecurityManager();
-            }
-            else
-            {
-                security = _appRegistry.getVirtualHostRegistry().getVirtualHost(vhost).getSecurityManager();
-            }
-
-            methodName = getMethodName(method, args);
-            if (isAccessMethod(methodName) || impact == MBeanOperationInfo.INFO)
-            {
-                // Check for read-only method invocation permission
-                if (!security.authoriseMethod(Operation.ACCESS, type, methodName))
-                {
-                    throw new SecurityException("Permission denied: Access " + methodName);
-                }
-            }
-            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);
-            }
-
+            CurrentActor.set(_logActor);
             try
             {
-                return doInvokeWrappingWithManagementActor(method, args);
+                return authoriseAndInvoke(method, args);
             }
             finally
             {
-                if(_managementRightsInferAllAccess)
-                {
-                    SecurityManager.setAccessChecksDisabled(oldAccessChecksDisabled);
-                }
+                CurrentActor.remove();
             }
         }
         catch (InvocationTargetException e)
@@ -216,18 +173,59 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler, Notificati
         }
     }
 
-    private Object doInvokeWrappingWithManagementActor(Method method,
-            Object[] args) throws IllegalAccessException,
-            InvocationTargetException
+    private Object authoriseAndInvoke(Method method, Object[] args) throws IllegalAccessException, InvocationTargetException
     {
+        String methodName;
+        // 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 = _appRegistry.getSecurityManager();
+        }
+        else
+        {
+            security = _appRegistry.getVirtualHostRegistry().getVirtualHost(vhost).getSecurityManager();
+        }
+
+        methodName = getMethodName(method, args);
+        if (isAccessMethod(methodName) || impact == MBeanOperationInfo.INFO)
+        {
+            // Check for read-only method invocation permission
+            if (!security.authoriseMethod(Operation.ACCESS, type, methodName))
+            {
+                throw new SecurityException("Permission denied: Access " + methodName);
+            }
+        }
+        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
         {
-            CurrentActor.set(_logActor);
             return method.invoke(_mbs, args);
         }
         finally
         {
-            CurrentActor.remove();
+            if(_managementRightsInferAllAccess)
+            {
+                SecurityManager.setAccessChecksDisabled(oldAccessChecksDisabled);
+            }
         }
     }
 
diff --git a/qpid/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/ManagementLogActorTest.java b/qpid/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/ManagementLogActorTest.java
new file mode 100644
index 0000000000..c1df9afc5d
--- /dev/null
+++ b/qpid/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/ManagementLogActorTest.java
@@ -0,0 +1,170 @@
+/*
+ *
+ * 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.jmx;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.management.JMException;
+import javax.management.MBeanServerConnection;
+import javax.management.ObjectName;
+import javax.management.remote.JMXConnector;
+import javax.management.remote.JMXConnectorFactory;
+import javax.management.remote.JMXServiceURL;
+
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration.XMLConfiguration;
+import org.apache.qpid.server.configuration.ServerConfiguration;
+import org.apache.qpid.server.configuration.plugins.ConfigurationPlugin;
+import org.apache.qpid.server.logging.actors.CurrentActor;
+import org.apache.qpid.server.registry.ApplicationRegistry;
+import org.apache.qpid.server.security.Result;
+import org.apache.qpid.server.security.SecurityPlugin;
+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.util.TestApplicationRegistry;
+import org.apache.qpid.test.utils.QpidTestCase;
+
+public class ManagementLogActorTest extends QpidTestCase
+{
+    private ApplicationRegistry _registry;
+    private JMXManagedObjectRegistry _objectRegistry;
+    private int _registryPort;
+    private int _connectorPort;
+    private TestPlugin _plugin;
+
+    @Override
+    public void setUp() throws Exception
+    {
+        super.setUp();
+
+        _registryPort = findFreePort();
+        _connectorPort = getNextAvailable(_registryPort + 1);
+        XMLConfiguration config = new XMLConfiguration();
+        config.addProperty(ServerConfiguration.MGMT_JMXPORT_REGISTRYSERVER, _registryPort + "");
+        config.addProperty(ServerConfiguration.MGMT_JMXPORT_CONNECTORSERVER, _connectorPort + "");
+        _registry = new TestApplicationRegistry(new ServerConfiguration(config));
+        ApplicationRegistry.initialise(_registry);
+
+        _plugin = new TestPlugin();
+        _registry.getSecurityManager().addHostPlugin(_plugin);
+
+        _objectRegistry = new JMXManagedObjectRegistry();
+        new TestMBean(_objectRegistry);
+        _objectRegistry.start();
+    }
+
+    public void tearDown() throws Exception
+    {
+        _objectRegistry.close();
+        ApplicationRegistry.remove();
+        super.tearDown();
+    }
+
+    public void testPrincipalInLogMessage() throws Throwable
+    {
+        Map<String, Object> environment = new HashMap<String, Object>();
+        environment.put(JMXConnector.CREDENTIALS, new String[] { "admin", "admin" });
+        String urlString = "service:jmx:rmi:///jndi/rmi://localhost:" + _registryPort + "/jmxrmi";
+        JMXServiceURL url = new JMXServiceURL(urlString);
+        JMXConnector jmxConnector = JMXConnectorFactory.connect(url, environment);
+        MBeanServerConnection mbsc = jmxConnector.getMBeanServerConnection();
+        ObjectName mbeanObject = new ObjectName("org.apache.qpid:type=TestMBean,name=test");
+
+        String actorLogMessage = (String) mbsc.getAttribute(mbeanObject, "ActorLogMessage");
+
+        assertTrue("Unexpected log principal in security plugin", _plugin.getLogMessage().startsWith("[mng:admin"));
+        assertTrue("Unexpected log principal in MBean", actorLogMessage.startsWith("[mng:admin"));
+    }
+
+    public static class TestMBean extends DefaultManagedObject implements CurrentActorRetriever
+    {
+
+        public TestMBean(ManagedObjectRegistry registry) throws JMException
+        {
+            super(CurrentActorRetriever.class, "TestMBean", registry);
+            register();
+        }
+
+        @Override
+        public String getObjectInstanceName()
+        {
+            return "test";
+        }
+
+        @Override
+        public ManagedObject getParentObject()
+        {
+            return null;
+        }
+
+        @Override
+        public String getActorLogMessage()
+        {
+            return CurrentActor.get().getLogMessage();
+        }
+
+    }
+
+    public static interface CurrentActorRetriever
+    {
+        String getActorLogMessage();
+    }
+
+    public static class TestPlugin implements SecurityPlugin
+    {
+        private String _logMessage;
+
+        @Override
+        public void configure(ConfigurationPlugin config) throws ConfigurationException
+        {
+        }
+
+        @Override
+        public Result getDefault()
+        {
+            return Result.ALLOWED;
+        }
+
+        @Override
+        public Result access(ObjectType objectType, Object instance)
+        {
+            return Result.ALLOWED;
+        }
+
+        @Override
+        public Result authorise(Operation operation, ObjectType objectType, ObjectProperties properties)
+        {
+            // set thread name to work around logic in MangementActor
+            Thread.currentThread().setName("RMI TCP Connection(1)-" + System.currentTimeMillis());
+            _logMessage = CurrentActor.get().getLogMessage();
+            return Result.ALLOWED;
+        }
+
+        public String getLogMessage()
+        {
+            return _logMessage;
+        }
+
+    }
+
+}