This is an automated email from the ASF dual-hosted git repository.
cshannon pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/activemq.git
The following commit(s) were added to refs/heads/main by this push:
new c8b71abe2f Optimize SecurityContext#isInOneOf attempt 2 (#1879)
c8b71abe2f is described below
commit c8b71abe2f9f9a3b8643b0d888623053e37e9fb2
Author: Christopher L. Shannon <[email protected]>
AuthorDate: Mon Apr 6 15:28:05 2026 -0400
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.
---
.../activemq/security/DefaultAuthorizationMap.java | 56 +++--------
.../apache/activemq/security/SecurityContext.java | 39 ++++++--
.../activemq/security/SecurityContextTest.java | 111 +++++++++++++++++++++
.../security/SimpleSecurityBrokerSystemTest.java | 3 +-
4 files changed, 157 insertions(+), 52 deletions(-)
diff --git
a/activemq-broker/src/main/java/org/apache/activemq/security/DefaultAuthorizationMap.java
b/activemq-broker/src/main/java/org/apache/activemq/security/DefaultAuthorizationMap.java
index 290f6bc07b..2c95c3740e 100644
---
a/activemq-broker/src/main/java/org/apache/activemq/security/DefaultAuthorizationMap.java
+++
b/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 class DefaultAuthorizationMap extends DestinationMap
implements Authoriza
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 class DefaultAuthorizationMap extends DestinationMap
implements Authoriza
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 class DefaultAuthorizationMap extends DestinationMap
implements Authoriza
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 class DefaultAuthorizationMap extends
DestinationMap implements Authoriza
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 class DefaultAuthorizationMap extends
DestinationMap implements Authoriza
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 class DefaultAuthorizationMap extends DestinationMap
implements Authoriza
return instance;
}
- class WildcardAwareSet<T> extends HashSet<T> {
+ static class WildcardAwareSet<T> extends HashSet<T> {
boolean hasWildcard = false;
@Override
@@ -281,10 +267,8 @@ public class DefaultAuthorizationMap extends
DestinationMap implements Authoriza
@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 class DefaultAuthorizationMap extends
DestinationMap implements Authoriza
}
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;
- }
}
}
diff --git
a/activemq-broker/src/main/java/org/apache/activemq/security/SecurityContext.java
b/activemq-broker/src/main/java/org/apache/activemq/security/SecurityContext.java
index fd677ce590..39835117fd 100644
---
a/activemq-broker/src/main/java/org/apache/activemq/security/SecurityContext.java
+++
b/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 @@ import org.apache.activemq.command.ActiveMQDestination;
*/
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,15 +56,26 @@ public abstract class SecurityContext {
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;
@@ -69,6 +83,11 @@ public abstract class SecurityContext {
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;
}
diff --git
a/activemq-broker/src/test/java/org/apache/activemq/security/SecurityContextTest.java
b/activemq-broker/src/test/java/org/apache/activemq/security/SecurityContextTest.java
new file mode 100644
index 0000000000..471888715b
--- /dev/null
+++
b/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);
+ }
+ }
+
+}
diff --git
a/activemq-unit-tests/src/test/java/org/apache/activemq/security/SimpleSecurityBrokerSystemTest.java
b/activemq-unit-tests/src/test/java/org/apache/activemq/security/SimpleSecurityBrokerSystemTest.java
index bee67c372a..d87902add3 100644
---
a/activemq-unit-tests/src/test/java/org/apache/activemq/security/SimpleSecurityBrokerSystemTest.java
+++
b/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);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
