Skip to content

Optimize SecurityContext#isInOneOf#1874

Merged
cshannon merged 2 commits intoapache:mainfrom
cshannon:optimize-security-context
Apr 3, 2026
Merged

Optimize SecurityContext#isInOneOf#1874
cshannon merged 2 commits intoapache:mainfrom
cshannon:optimize-security-context

Conversation

@cshannon
Copy link
Copy Markdown
Contributor

@cshannon cshannon commented Apr 3, 2026

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.

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

@cshannon
Optimize SecurityContext#isInOneOf
db8c589 
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.

A small test was added but there are several other authorization tests
that already exist that also exercise this method.
@cshannon cshannon force-pushed the optimize-security-context branch from bf09c56 to db8c589 Compare April 3, 2026 19:44
@cshannon cshannon changed the title Optimize SecurityContext#isOneOf() Optimize SecurityContext#isInOneOf Apr 3, 2026
@cshannon cshannon force-pushed the optimize-security-context branch from aa14c8c to daf2b9f Compare April 3, 2026 22:00
@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Apr 3, 2026 •
edited
Loading

I removed the copying of the set from getPrincipals() because this was not only inefficient but also not thread safe.

I looked around and the sub classes of SecurityContext.getPrincipals() all generally return null, empty set, a new copy of a set or they return Subject.getPrincipals() if using Jaas. For null/empty set and a new copy ther is no reason to make another copy of course. For Subject.getPrincipals(), that set is actually backed by a SynchronizedSet which is not thread safe to iterate over (which happens when making a copy) without adding synchronization externally, so by removing the copy not only do we avoid the overhead but it's actually thread safe now becase calling Subject.getPrincipals().contains() is thread safe.

@cshannon cshannon requested a review from tabish121 April 3, 2026 22:06
tabish121
tabish121 approved these changes Apr 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@tabish121 tabish121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM definitely an improvement on overall code quality.

@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Apr 3, 2026

No reason to back port this to older branches really as it's just some optimization and cleanup so we can just keep it for 6.3.0

@cshannon cshannon moved this from Backlog to Done in Apache ActiveMQ v6.3.0 Apr 3, 2026
@cshannon cshannon merged commit b404e1e into apache:main Apr 3, 2026
9 of 10 checks passed
@cshannon cshannon deleted the optimize-security-context branch April 3, 2026 22:20
cshannon added a commit to cshannon/activemq that referenced this pull request Apr 3, 2026
@cshannon
Revert "Optimize SecurityContext#isInOneOf (apache#1874)"
4bab0fe 
This reverts commit b404e1e.
cshannon added a commit to cshannon/activemq that referenced this pull request Apr 4, 2026
@cshannon
Revert "Optimize SecurityContext#isInOneOf (apache#1874)"
3497a52 
This reverts commit b404e1e.

This change needs more work because DefaultAuthorizationMap requires
equals() to be used the way it handles a wildcard Principal. This needs
to be fixed before the change can be made
@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Apr 4, 2026

This is being reverted in #1875 because it needs some more work

cshannon added a commit that referenced this pull request Apr 4, 2026
@cshannon
Revert "Optimize SecurityContext#isInOneOf (#1874)" (#1875)
e612481 
This reverts commit b404e1e.

This change needs more work because DefaultAuthorizationMap requires
equals() to be used the way it handles a wildcard Principal. This needs
to be fixed before the change can be made
cshannon added a commit to cshannon/activemq that referenced this pull request Apr 6, 2026
@cshannon
Optimize SecurityContext#isInOneOf attempt 2
460ac57 
This is an update to apache#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 inside
DefaultAuthorizationMap by just using a predefined static object to mark
that everything should be allowed if the name is "*"

A small test was added but there are several other authorization tests that
already exist that also exercise this method.
@cshannon cshannon mentioned this pull request Apr 6, 2026
Merged
cshannon added a commit to cshannon/activemq that referenced this pull request Apr 6, 2026
@cshannon
Optimize SecurityContext#isInOneOf attempt 2
5452f31 
This is an update to apache#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.
cshannon added a commit to cshannon/activemq that referenced this pull request Apr 6, 2026
@cshannon
Optimize SecurityContext#isInOneOf attempt 2
92a4256 
This is an update to apache#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.
cshannon added a commit that referenced this pull request Apr 6, 2026
@cshannon
Optimize SecurityContext#isInOneOf attempt 2 (#1879)
c8b71ab 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tabish121 tabish121 tabish121 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cshannon @tabish121