https://issues.apache.org/jira/browse/AMQ-3749 - Composite destinations break simple authorisation through role aggregation. additional tests and fix - ldap did need some work

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

activemq-core/src/main/java/org/apache/activemq/filter/DestinationMap.java

@@ -17,6 +17,7 @@
 package org.apache.activemq.filter;
 
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
 import java.util.SortedSet;
@@ -230,4 +231,23 @@ public class DestinationMap {
         topicRootNode = new DestinationMapNode(null);
         tempTopicRootNode = new DestinationMapNode(null);
     }
+
+    public static Set union(Set existing, Set candidates) {
+        if ( candidates != null ) {
+            if (existing != null) {
+                for (Iterator<Object> iterator = existing.iterator(); iterator.hasNext();) {
+                    Object toMatch = iterator.next();
+                    if (!candidates.contains(toMatch)) {
+                        iterator.remove();
+                    }
+                }
+            } else {
+                existing = candidates;
+            }
+        } else if ( existing != null ) {
+            existing.clear();
+        }
+        return existing;
+    }
+
 }

activemq-core/src/main/java/org/apache/activemq/security/DefaultAuthorizationMap.java

@@ -153,23 +153,6 @@ public class DefaultAuthorizationMap extends DestinationMap implements Authoriza
         return findWildcardMatches(key);
     }
 
-    private Set union(Set existing, Set candidates) {
-        if ( candidates != null ) {
-            if (existing != null) {
-                for (Iterator<Object> iterator = existing.iterator(); iterator.hasNext();) {
-                    Object toMatch = iterator.next();
-                    if (!candidates.contains(toMatch)) {
-                        iterator.remove();
-                    }
-                }
-            } else {
-                existing = candidates;
-            }
-        } else if ( existing != null ) {
-            existing.clear();
-        }
-        return existing;
-    }
 
     /**
      * Sets the individual entries on the authorization map

activemq-core/src/main/java/org/apache/activemq/security/LDAPAuthorizationMap.java

@@ -35,6 +35,7 @@ import javax.naming.directory.SearchResult;
 
 import org.apache.activemq.advisory.AdvisorySupport;
 import org.apache.activemq.command.ActiveMQDestination;
+import org.apache.activemq.filter.DestinationMap;
 import org.apache.activemq.jaas.GroupPrincipal;
 import org.apache.activemq.jaas.LDAPLoginModule;
 import org.slf4j.Logger;
@@ -362,9 +363,12 @@ public class LDAPAuthorizationMap implements AuthorizationMap {
 
     protected Set<GroupPrincipal> getCompositeACLs(ActiveMQDestination destination, String roleBase, String roleAttribute) {
         ActiveMQDestination[] dests = destination.getCompositeDestinations();
-        Set<GroupPrincipal> acls = new HashSet<GroupPrincipal>();
+        Set<GroupPrincipal> acls = null;
         for (ActiveMQDestination dest : dests) {
-            acls.addAll(getACLs(dest, roleBase, roleAttribute));
+            acls = DestinationMap.union(acls, getACLs(dest, roleBase, roleAttribute));
+            if (acls == null || acls.isEmpty()) {
+                break;
+            }
         }
         return acls;
     }

activemq-core/src/test/java/org/apache/activemq/security/CachedLDAPSecurityTest.java

@@ -33,6 +33,7 @@ import org.junit.runner.RunWith;
 import javax.jms.*;
 
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
 
 
 @RunWith( FrameworkRunner.class )
@@ -76,6 +77,38 @@ public class CachedLDAPSecurityTest extends AbstractLdapTestUnit {
         assertNotNull(msg);
     }
 
+    @Test
+    public void testSendDenied() throws Exception {
+        ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory("tcp://localhost:61616");
+        Connection conn = factory.createQueueConnection("jdoe", "sunflower");
+        Session sess = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
+        conn.start();
+        Queue queue = sess.createQueue("ADMIN.FOO");
+
+        MessageProducer producer = sess.createProducer(queue);
+        try {
+            producer.send(sess.createTextMessage("test"));
+            fail("expect auth exception");
+        } catch (JMSException expected) {
+        }
+    }
+
+    @Test
+    public void testCompositeSendDenied() throws Exception {
+        ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory("tcp://localhost:61616");
+        Connection conn = factory.createQueueConnection("jdoe", "sunflower");
+        Session sess = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
+        conn.start();
+        Queue queue = sess.createQueue("TEST.FOO,ADMIN.FOO");
+
+        MessageProducer producer = sess.createProducer(queue);
+        try {
+            producer.send(sess.createTextMessage("test"));
+            fail("expect auth exception");
+        } catch (JMSException expected) {
+        }
+    }
+
     @Test
     public void testTempDestinations() throws Exception {
         ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory("tcp://localhost:61616");

activemq-core/src/test/resources/org/apache/activemq/security/activemq-ldap.xml

@@ -27,6 +27,10 @@
 
   <broker useJmx="false"  xmlns="http://activemq.apache.org/schema/core" persistent="false">
 
+      <destinations>
+         <queue physicalName="ADMIN.FOO" />
+      </destinations>
+
       <plugins>
 		<simpleAuthenticationPlugin>
 			<users>