QPID-3858: Updated code to include recent refactoring by Gordon (gsim) - see QPID-4178.

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

4 files changed, 136 insertions, 73 deletions

diff --git a/java/broker/etc/broker_example.acl b/java/broker/etc/broker_example.acl
index 1f32f8463e..45a48bda09 100644
--- a/java/broker/etc/broker_example.acl
+++ b/java/broker/etc/broker_example.acl
@@ -18,6 +18,7 @@
 #
 
 ### EXAMPLE ACL V2 FILE
+### NOTE: Rules are considered from top to bottom, and the first matching rule governs the decision.
 
 ### DEFINE GROUPS ###
 
@@ -27,30 +28,30 @@ GROUP messaging-users client server
 #Define a group for management web console users
 GROUP webadmins webadmin
 
-### MANAGEMENT ####
+### JMX MANAGEMENT ####
 
 # Allow everyone to perform read operations on the ServerInformation mbean
 # This is used for items such as querying the management API and broker release versions.
-ACL ALLOW-LOG ALL ACCESS METHOD component="ServerInformation"
+ACL ALLOW ALL ACCESS METHOD component="ServerInformation"
 
-# Allow 'admin' all management operations
+# Allow 'admin' all management operations. To reduce log file noise, only non-read-only operations are logged.
+ACL ALLOW admin ACCESS METHOD
 ACL ALLOW-LOG admin ALL METHOD
 
+# Allow 'guest' to view logger levels, and use getter methods on LoggingManagement
+ACL ALLOW guest ACCESS METHOD component="LoggingManagement" name="viewEffectiveRuntimeLoggerLevels"
+ACL ALLOW guest ACCESS METHOD component="LoggingManagement" name="get*"
+
 # Deny access to Shutdown, UserManagement, ConfigurationManagement and LoggingManagement for all other users
-# You could grant specific users access to these beans by adding ALLOW-LOG rules above for them
+# You could grant specific users access to these beans by adding rules above to allow them
 ACL DENY-LOG ALL ACCESS METHOD component="Shutdown"
 ACL DENY-LOG ALL ACCESS METHOD component="UserManagement"
 ACL DENY-LOG ALL ACCESS METHOD component="ConfigurationManagement"
 ACL DENY-LOG ALL ACCESS METHOD component="LoggingManagement"
 
-# Allow 'guest' to view logger levels, and use getter methods on LoggingManagement
-# These are examples of redundant rules! The DENY-LOG rule above will be invoked
-# first and will deny the access to all methods of LoggingManagement for guest
-ACL ALLOW-LOG guest ACCESS METHOD component="LoggingManagement" name="viewEffectiveRuntimeLoggerLevels"
-ACL ALLOW-LOG guest ACCESS METHOD component="LoggingManagement" name="get*"
-
-# Allow everyone to perform all read operations on the mbeans not listened in the DENY-LOG rules above
-ACL ALLOW-LOG ALL ACCESS METHOD
+# Allow everyone to perform all read operations (using ALLOW rather than ALLOW-LOG to reduce log file noise)
+# on the mbeans not listed in the DENY rules above
+ACL ALLOW ALL ACCESS METHOD
 
 ### MESSAGING ###
 
diff --git a/java/broker/src/main/java/org/apache/qpid/server/logging/actors/ManagementActor.java b/java/broker/src/main/java/org/apache/qpid/server/logging/actors/ManagementActor.java
index 9cd3c66629..a2f3506502 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/logging/actors/ManagementActor.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/logging/actors/ManagementActor.java
@@ -30,16 +30,7 @@ import java.text.MessageFormat;
 import java.util.Set;
 
 /**
- * NOTE: This actor is not thread safe.
- *
- * Sharing of a ManagementActor instance between threads may result in an
- * incorrect actor value being logged.
- *
- * This is due to the fact that calls to message will dynamically query the
- * thread name and use that to set the log format during each message() call.
- *
- * This is currently not an issue as each MBean operation creates a new Actor
- * that is unique for each operation.
+ * Management actor to use in {@link MBeanInvocationHandlerImpl} to log all management operational logging.
  */
 public class ManagementActor extends AbstractActor
 {
@@ -66,38 +57,45 @@ public class ManagementActor extends AbstractActor
     /**
      * The logString to be used for logging
      */
-    private String _logString;
+    private String _logStringContainingPrincipal;
+
+    /** used when the principal name cannot be discovered from the Subject */
+    private final String _fallbackPrincipalName;
 
     /** @param rootLogger The RootLogger to use for this Actor */
     public ManagementActor(RootMessageLogger rootLogger)
     {
         super(rootLogger);
+        _fallbackPrincipalName = UNKNOWN_PRINCIPAL;
+    }
+
+    public ManagementActor(RootMessageLogger rootLogger, String principalName)
+    {
+        super(rootLogger);
+        _fallbackPrincipalName = principalName;
     }
 
-    private void updateLogString()
+    private synchronized String getAndCacheLogString()
     {
         String currentName = Thread.currentThread().getName();
 
         String actor;
+        String logString = _logStringContainingPrincipal;
+
         // Record the last thread name so we don't have to recreate the log string
-        if (!currentName.equals(_lastThreadName))
+        if (_logStringContainingPrincipal == null || !currentName.equals(_lastThreadName))
         {
             _lastThreadName = currentName;
+            String principalName = getPrincipalName();
 
             // Management Thread names have this format.
             // RMI TCP Connection(2)-169.24.29.116
             // This is true for both LocalAPI and JMX Connections
             // However to be defensive lets test.
-
             String[] split = currentName.split("\\(");
             if (split.length == 2)
             {
                 String ip = currentName.split("-")[1];
-                String principalName = getPrincipalName();
-                if (principalName == null)
-                {
-                    principalName = UNKNOWN_PRINCIPAL;
-                }
                 actor = MessageFormat.format(MANAGEMENT_FORMAT, principalName, ip);
             }
             else
@@ -111,9 +109,14 @@ public class ManagementActor extends AbstractActor
                 actor = currentName;
             }
 
-            _logString = "[" + actor + "] ";
+            logString = "[" + actor + "] ";
+            if(principalName != UNKNOWN_PRINCIPAL )
+            {
+                _logStringContainingPrincipal = logString;
+            }
 
         }
+        return logString;
     }
 
     /**
@@ -121,9 +124,9 @@ public class ManagementActor extends AbstractActor
      *
      * @return principal name or null if principal can not be found
      */
-    protected String getPrincipalName()
+    private String getPrincipalName()
     {
-        String identity = null;
+        String identity = _fallbackPrincipalName;
 
         // retrieve Subject from current AccessControlContext
         final Subject subject = Subject.getSubject(AccessController.getContext());
@@ -142,8 +145,7 @@ public class ManagementActor extends AbstractActor
 
     public String getLogMessage()
     {
-        updateLogString();
-        return _logString;
+        return getAndCacheLogString();
     }
 
 }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/model/Model.java b/java/broker/src/main/java/org/apache/qpid/server/model/Model.java
index fd429321c8..36179fc105 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/model/Model.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/model/Model.java
@@ -29,33 +29,20 @@ import java.util.Map;
 
 public class Model
 {
-    private static final Map<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>
-            PARENTS = new HashMap<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>();
+    private static final Model MODEL_INSTANCE = new Model();
 
+    private final Map<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>
+            _parents = new HashMap<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>();
 
-    private static final Map<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>
-            CHILDREN = new HashMap<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>();
+    private final Map<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>
+            _children = new HashMap<Class<? extends ConfiguredObject>, Collection<Class<? extends ConfiguredObject>>>();
 
-    static void addRelationship(Class<? extends ConfiguredObject> parent, Class<? extends ConfiguredObject> child)
+    public static Model getInstance()
     {
-        Collection<Class<? extends ConfiguredObject>> parents = PARENTS.get(child);
-        if(parents == null)
-        {
-            parents = new ArrayList<Class<? extends ConfiguredObject>>();
-            PARENTS.put(child, parents);
-        }
-        parents.add(parent);
-
-        Collection<Class<? extends ConfiguredObject>> children = CHILDREN.get(parent);
-        if(children == null)
-        {
-            children = new ArrayList<Class<? extends ConfiguredObject>>();
-            CHILDREN.put(parent, children);
-        }
-        children.add(child);
+        return MODEL_INSTANCE;
     }
 
-    static
+    private Model()
     {
         addRelationship(Broker.class, VirtualHost.class);
         addRelationship(Broker.class, Port.class);
@@ -78,20 +65,39 @@ public class Model
 
         addRelationship(Session.class, Consumer.class);
         addRelationship(Session.class, Publisher.class);
-
     }
 
-    public static Collection<Class<? extends ConfiguredObject>> getParentTypes(Class<? extends ConfiguredObject> child)
+    public Collection<Class<? extends ConfiguredObject>> getParentTypes(Class<? extends ConfiguredObject> child)
     {
-        Collection<Class<? extends ConfiguredObject>> parentTypes = PARENTS.get(child);
-        return parentTypes == null ? Collections.EMPTY_LIST
+        Collection<Class<? extends ConfiguredObject>> parentTypes = _parents.get(child);
+        return parentTypes == null ? Collections.<Class<? extends ConfiguredObject>>emptyList()
                                    : Collections.unmodifiableCollection(parentTypes);
     }
 
-    public static Collection<Class<? extends ConfiguredObject>> getChildTypes(Class<? extends ConfiguredObject> parent)
+    public Collection<Class<? extends ConfiguredObject>> getChildTypes(Class<? extends ConfiguredObject> parent)
     {
-        Collection<Class<? extends ConfiguredObject>> childTypes = CHILDREN.get(parent);
-        return childTypes == null ? Collections.EMPTY_LIST
+        Collection<Class<? extends ConfiguredObject>> childTypes = _children.get(parent);
+        return childTypes == null ? Collections.<Class<? extends ConfiguredObject>>emptyList()
                                   : Collections.unmodifiableCollection(childTypes);
     }
+
+    private void addRelationship(Class<? extends ConfiguredObject> parent, Class<? extends ConfiguredObject> child)
+    {
+        Collection<Class<? extends ConfiguredObject>> parents = _parents.get(child);
+        if(parents == null)
+        {
+            parents = new ArrayList<Class<? extends ConfiguredObject>>();
+            _parents.put(child, parents);
+        }
+        parents.add(parent);
+
+        Collection<Class<? extends ConfiguredObject>> children = _children.get(parent);
+        if(children == null)
+        {
+            children = new ArrayList<Class<? extends ConfiguredObject>>();
+            _children.put(parent, children);
+        }
+        children.add(child);
+    }
+
 }
diff --git a/java/broker/src/test/java/org/apache/qpid/server/logging/actors/ManagementActorTest.java b/java/broker/src/test/java/org/apache/qpid/server/logging/actors/ManagementActorTest.java
index b431047d66..cb866245f0 100644
--- a/java/broker/src/test/java/org/apache/qpid/server/logging/actors/ManagementActorTest.java
+++ b/java/broker/src/test/java/org/apache/qpid/server/logging/actors/ManagementActorTest.java
@@ -26,15 +26,6 @@ import java.security.PrivilegedAction;
 import java.util.Collections;
 import java.util.List;
 
-/**
- * Test : AMQPManagementActorTest
- * Validate the AMQPManagementActor class.
- *
- * The test creates a new AMQPActor and then logs a message using it.
- *
- * The test then verifies that the logged message was the only one created and
- * that the message contains the required message.
- */
 public class ManagementActorTest extends BaseActorTestCase
 {
 
@@ -131,4 +122,67 @@ public class ManagementActorTest extends BaseActorTestCase
         assertTrue("Message contains the [mng: prefix", logMessage.contains("[mng:guest(" + IP + ")"));
     }
 
+    public void testGetLogMessageWithSubject()
+    {
+        assertLogMessageInRMIThreadWithPrincipal("RMI TCP Connection(" + CONNECTION_ID + ")-" + IP, "my_principal");
+    }
+
+    public void testGetLogMessageWithoutSubjectButWithActorPrincipal()
+    {
+        String principalName = "my_principal";
+        _amqpActor = new ManagementActor(_rootLogger, principalName);
+        String message = _amqpActor.getLogMessage();
+        assertEquals("Unexpected log message", "[mng:" + principalName + "(" + IP + ")] ", message);
+    }
+
+    /** It's necessary to test successive calls because ManagementActor caches its log message based on thread and principal name */
+    public void testGetLogMessageCaching()
+    {
+        String originalThreadName = "RMI TCP Connection(1)-" + IP;
+        assertLogMessageInRMIThreadWithoutPrincipal(originalThreadName);
+        assertLogMessageInRMIThreadWithPrincipal(originalThreadName, "my_principal");
+        assertLogMessageInRMIThreadWithPrincipal("RMI TCP Connection(2)-" + IP, "my_principal");
+    }
+
+    public void testGetLogMessageAfterRemovingSubject()
+    {
+        assertLogMessageInRMIThreadWithPrincipal("RMI TCP Connection(1)-" + IP, "my_principal");
+
+        Thread.currentThread().setName("RMI TCP Connection(2)-" + IP );
+        String message = _amqpActor.getLogMessage();
+        assertEquals("Unexpected log message", "[mng:N/A(" + IP + ")] ", message);
+
+        assertLogMessageWithoutPrincipal("TEST");
+    }
+
+    private void assertLogMessageInRMIThreadWithoutPrincipal(String threadName)
+    {
+        Thread.currentThread().setName(threadName );
+        String message = _amqpActor.getLogMessage();
+        assertEquals("Unexpected log message", "[mng:N/A(" + IP + ")] ", message);
+    }
+
+    private void assertLogMessageWithoutPrincipal(String threadName)
+    {
+        Thread.currentThread().setName(threadName );
+        String message = _amqpActor.getLogMessage();
+        assertEquals("Unexpected log message", "[" + threadName +"] ", message);
+    }
+
+    private void assertLogMessageInRMIThreadWithPrincipal(String threadName, String principalName)
+    {
+        Thread.currentThread().setName(threadName);
+        Subject subject = new Subject(true, Collections.singleton(new JMXPrincipal(principalName)), Collections.EMPTY_SET,
+                Collections.EMPTY_SET);
+
+        final String message = Subject.doAs(subject, new PrivilegedAction<String>()
+        {
+            public String run()
+            {
+                return _amqpActor.getLogMessage();
+            }
+        });
+
+        assertEquals("Unexpected log message", "[mng:" + principalName + "(" + IP + ")] ", message);
+    }
 }