Optimize SecurityContext#isInOneOf attempt 2 (#1879)

This is an update to #1874 which fixes the previous issue with wildcard
principal handling which caused things to break

This method was iterating over a set to find matching entries instead of
just using contains(). This update switches to using contains() which will
be faster and also cleans up the code with a for each loop.

The wildcard principal handling was simplified by using a
predefined static object inside SecurityContext to mark
that everything should be allowed if the name is "*".
DefaultAuthorizationMap now references this and uses this object
when parsing ACLs if a wildcard is used. Because it's been moved
to SecurityContext it's flexible and can be used by other impls as well.

A small test was added but there are several other authorization tests that
already exist that also exercise this method.

Author: cshannon

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

@@ -16,9 +16,11 @@
  */
 package org.apache.activemq.security;
 
+import static org.apache.activemq.security.SecurityContext.WILDCARD_PRINCIPAL;
+import static org.apache.activemq.security.SecurityContext.isWildcardPrincipal;
+
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
-import java.security.Principal;
 import java.util.*;
 
 import org.apache.activemq.command.ActiveMQDestination;
@@ -99,8 +101,7 @@ public Set<Object> getAdminACLs(ActiveMQDestination destination) {
         Set<Object> answer = new WildcardAwareSet<Object>();
 
         // now lets go through each entry adding individual
-        for (Iterator<AuthorizationEntry> iter = entries.iterator(); iter.hasNext();) {
-            AuthorizationEntry entry = iter.next();
+        for (AuthorizationEntry entry : entries) {
             answer.addAll(entry.getAdminACLs());
         }
         return answer;
@@ -112,8 +113,7 @@ public Set<Object> getReadACLs(ActiveMQDestination destination) {
         Set<Object> answer = new WildcardAwareSet<Object>();
 
         // now lets go through each entry adding individual
-        for (Iterator<AuthorizationEntry> iter = entries.iterator(); iter.hasNext();) {
-            AuthorizationEntry entry = iter.next();
+        for (AuthorizationEntry entry : entries) {
             answer.addAll(entry.getReadACLs());
         }
         return answer;
@@ -125,8 +125,7 @@ public Set<Object> getWriteACLs(ActiveMQDestination destination) {
         Set<Object> answer = new WildcardAwareSet<Object>();
 
         // now lets go through each entry adding individual
-        for (Iterator<AuthorizationEntry> iter = entries.iterator(); iter.hasNext();) {
-            AuthorizationEntry entry = iter.next();
+        for (AuthorizationEntry entry : entries) {
             answer.addAll(entry.getWriteACLs());
         }
         return answer;
@@ -157,10 +156,9 @@ public synchronized Set get(ActiveMQDestination key) {
         if (key.isComposite()) {
             ActiveMQDestination[] destinations = key.getCompositeDestinations();
             Set answer = null;
-            for (int i = 0; i < destinations.length; i++) {
-                ActiveMQDestination childDestination = destinations[i];
+            for (ActiveMQDestination childDestination : destinations) {
                 answer = union(answer, get(childDestination));
-                if (answer == null  || answer.isEmpty()) {
+                if (answer == null || answer.isEmpty()) {
                     break;
                 }
             }
@@ -210,26 +208,14 @@ public void setGroupClass(String groupClass) {
         this.groupClass = groupClass;
     }
 
-    final static String WILDCARD = "*";
+    static final String WILDCARD = SecurityContext.WILDCARD;
+
     public static Object createGroupPrincipal(String name, String groupClass) throws Exception {
         if (WILDCARD.equals(name)) {
             // simple match all group principal - match any name and class
-            return new Principal() {
-                @Override
-                public String getName() {
-                    return WILDCARD;
-                }
-                @Override
-                public boolean equals(Object other) {
-                    return true;
-                }
-
-                @Override
-                public int hashCode() {
-                    return WILDCARD.hashCode();
-                }
-            };
+            return WILDCARD_PRINCIPAL;
         }
+
         Object[] param = new Object[]{name};
 
         Class<?> cls = Class.forName(groupClass);
@@ -266,7 +252,7 @@ public int hashCode() {
         return instance;
     }
 
-    class WildcardAwareSet<T> extends HashSet<T> {
+    static class WildcardAwareSet<T> extends HashSet<T> {
         boolean hasWildcard = false;
 
         @Override
@@ -281,10 +267,8 @@ public boolean contains(Object e) {
         @Override
         public boolean addAll(Collection<? extends T> collection) {
             boolean modified = false;
-            Iterator<? extends T> e = collection.iterator();
-            while (e.hasNext()) {
-                final T item = e.next();
-                if (isWildcard(item)) {
+            for (T item : collection) {
+                if (isWildcardPrincipal(item)) {
                     hasWildcard = true;
                 }
                 if (add(item)) {
@@ -293,15 +277,5 @@ public boolean addAll(Collection<? extends T> collection) {
             }
             return modified;
         }
-
-        private boolean isWildcard(T item) {
-            try {
-                if (item.getClass().getMethod("getName", new Class[]{}).invoke(item).equals("*")) {
-                    return true;
-                }
-            } catch (Exception ignored) {
-            }
-            return false;
-        }
     }
 }

activemq-broker/src/main/java/org/apache/activemq/security/SecurityContext.java

@@ -16,10 +16,9 @@
  */
 package org.apache.activemq.security;
 
+
 import java.security.Principal;
 import java.util.Collections;
-import java.util.HashSet;
-import java.util.Iterator;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -33,6 +32,10 @@
  */
 public abstract class SecurityContext {
 
+    static final String WILDCARD = "*";
+
+    static final Principal WILDCARD_PRINCIPAL = () -> WILDCARD;
+
     public static final SecurityContext BROKER_SECURITY_CONTEXT = new SecurityContext("ActiveMQBroker") {
         @Override
         public boolean isBrokerContext() {
@@ -53,22 +56,38 @@ public SecurityContext(String userName) {
         this.userName = userName;
     }
 
+    public static boolean isWildcardPrincipal(Object principal) {
+        return principal == WILDCARD_PRINCIPAL;
+    }
+
+    public static boolean containsWildcardPrincipal(Set<?> principals) {
+        return principals.contains(WILDCARD_PRINCIPAL);
+    }
+
     public boolean isInOneOf(Set<?> allowedPrincipals) {
-        Iterator<?> allowedIter = allowedPrincipals.iterator();
-        HashSet<?> userPrincipals = new HashSet<Object>(getPrincipals());
-        while (allowedIter.hasNext()) {
-            Iterator<?> userIter = userPrincipals.iterator();
-            Object allowedPrincipal = allowedIter.next();
-            while (userIter.hasNext()) {
-                if (allowedPrincipal.equals(userIter.next()))
-                    return true;
+        // Wildcard Principal is a special principal that allows all
+        // Check for that first using an identify check. This is
+        // a fast check that allows us to avoid iterating if true
+        if (containsWildcardPrincipal(allowedPrincipals)) {
+            return true;
+        }
+
+        // Iterate over all the principals to see if there are matches
+        for (Object allowedPrincipal : allowedPrincipals) {
+            if (contains(allowedPrincipal)) {
+                return true;
             }
         }
         return false;
     }
 
     public abstract Set<Principal> getPrincipals();
 
+    public boolean contains(Object principal) {
+        Set<Principal> principals = getPrincipals();
+        return principals != null && principals.contains(principal);
+    }
+
     public String getUserName() {
         return userName;
     }

activemq-broker/src/test/java/org/apache/activemq/security/SecurityContextTest.java

@@ -0,0 +1,111 @@
+/*
+ * 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.activemq.security;
+
+import static org.apache.activemq.security.SecurityContextTest.TestPrincipal.testPrincipal;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
+import java.security.Principal;
+import java.util.Objects;
+import java.util.Set;
+import org.junit.Test;
+
+public class SecurityContextTest {
+
+    @Test
+    public void testIsOneOf() {
+        SecurityContext context = newContext(Set.of(testPrincipal("one"),
+                testPrincipal("two"), testPrincipal("three")));
+
+        // test valid combos
+        assertTrue(context.isInOneOf(Set.of(testPrincipal("one"))));
+        assertTrue(context.isInOneOf(Set.of(testPrincipal("two"))));
+        assertTrue(context.isInOneOf(Set.of(testPrincipal("three"))));
+        assertTrue(context.isInOneOf(Set.of(testPrincipal("three"),
+                testPrincipal("four"), testPrincipal("five"))));
+
+        // test no matching
+        assertFalse(context.isInOneOf(Set.of(testPrincipal("four"),
+                testPrincipal("five"))));
+        assertFalse(context.isInOneOf(Set.of()));
+        // different impl types, should not find
+        assertFalse(context.isInOneOf(Set.of((Principal) () -> "one")));
+
+        // empty set
+        context = newContext(Set.of());
+        assertFalse(context.isInOneOf(Set.of(testPrincipal("one"))));
+        assertFalse(context.isInOneOf(Set.of()));
+    }
+
+    @Test
+    public void testWildcard() throws Exception {
+        SecurityContext context = newContext(Set.of(testPrincipal("one"),
+                testPrincipal("two"), testPrincipal("three")));
+
+        // test valid combos
+        Object wildcard1 = DefaultAuthorizationMap.createGroupPrincipal("*", "someIgnoredClass1");
+        Object wildcard2 = DefaultAuthorizationMap.createGroupPrincipal("*", "someIgnoredClass2");
+
+        // should be identical objects
+        assertSame(wildcard1, wildcard2);
+        // wildcard matches anything
+        assertTrue(context.isInOneOf(Set.of(wildcard1)));
+    }
+
+    private SecurityContext newContext(Set<Principal> principals) {
+        return new SecurityContext("user") {
+            @Override
+            public Set<Principal> getPrincipals() {
+                return principals;
+            }
+        };
+    }
+
+    static class TestPrincipal implements Principal {
+        private final String name;
+
+        private TestPrincipal(String name) {
+            this.name = name;
+        }
+
+        @Override
+        public String getName() {
+            return name;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+            TestPrincipal that = (TestPrincipal) o;
+            return Objects.equals(name, that.name);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hashCode(name);
+        }
+
+        static TestPrincipal testPrincipal(String name) {
+            return new TestPrincipal(name);
+        }
+    }
+
+}

activemq-unit-tests/src/test/java/org/apache/activemq/security/SimpleSecurityBrokerSystemTest.java

@@ -61,7 +61,8 @@ public class SimpleSecurityBrokerSystemTest extends SecurityTestSupport {
     public static Principal WILDCARD;
     static {
         try {
-         WILDCARD = (Principal) DefaultAuthorizationMap.createGroupPrincipal("*", GroupPrincipal.class.getName());
+         WILDCARD = (Principal) DefaultAuthorizationMap.createGroupPrincipal(
+                 SecurityContext.WILDCARD, GroupPrincipal.class.getName());
         } catch (Exception e) {
             LOG.error("Failed to make wildcard principal", e);
         }