Optimize SecurityContext#isInOneOf (#1874)

This method was iterating over a set to find matching entries instead of
just using contains(). This updates switches to using contains() which
will be faster and also cleans up the code with a for each loop.
This also removes the copying of the principal set set copy that is 
unnecessary and actually not thread safe without external sync when using
Jaas.

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/SecurityContext.java

@@ -54,21 +54,21 @@ public SecurityContext(String userName) {
     }
 
     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;
+        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,95 @@
+/*
+ * 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.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()));
+    }
+
+    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);
+        }
+    }
+
+}