-
Notifications
You must be signed in to change notification settings - Fork 629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ISPN-5790 AuthorizationManager rework #3726
ISPN-5790 AuthorizationManager rework #3726
Conversation
} | ||
int permissionMask = requiredPermission.getMask(); | ||
return (subjectMask & permissionMask) == permissionMask && requestedRole.map(r -> subjectACL.roles.contains(r)).orElse(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: lambda could be replaced by subjectACL.roles::contains
4b92575
to
e310c8e
Compare
I have made some further changes:
|
e310c8e
to
84ed93f
Compare
some nitpick about
|
private int computeHashCode() { | ||
final int prime = 31; | ||
int result = 1; | ||
result = prime * result + ((cacheName == null) ? 0 : cacheName.hashCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the cacheName
or the principalName
be null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an interesting question, but I guess not a big deal either way.
A cache based on the Principals's name is only effective if you assume there is a limited number of principals accessing, otherwise you're risking to store a lot of data for no good reason - and probably still have a low hit rate. |
@Sanne not really if we assume that a principal will perform operations on the cache in bursts: you want to cache the ACL on the first op so that subsequent ops can reuse it. Set a low expiration for this case. |
@Sanne I see the use-case of lots of individual principals less practical / useful in the real world. |
754d61e
to
e6e534f
Compare
/** | ||
* Removes the internal caches from the specified set of cache names | ||
*/ | ||
void filterPrivateCaches(Set<String> names); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the name change? The javadoc still mentions internal caches...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The javadoc is wrong and the actual implementation was filtering the private caches :) I'll fix this
24525ca
to
eca499d
Compare
@danberindei ready to go ! |
eca499d
to
33204d6
Compare
@danberindei repushed :) |
@@ -12,7 +12,8 @@ | |||
*/ | |||
GlobalAuthorizationConfigurationBuilder authorization(); | |||
/** | |||
* Defines the timeout in milliseconds for which to cache user access roles | |||
* Defines the timeout in milliseconds for which to cache user access roles. A value of -1 means the entries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this might be too late now. But was there a reason we forced this to be milliseconds and didn't just do the new common method of passing a long and a TimeUnit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it. Added a new method and deprecated the old one
33204d6
to
c37756d
Compare
private SubjectACL computeSubjectACL(Subject subject, AuthorizationConfiguration configuration) { | ||
PrincipalRoleMapper roleMapper = globalConfiguration.authorization().principalRoleMapper(); | ||
Set<Principal> principals = subject.getPrincipals(); | ||
Set<String> allRoles = new HashSet<>(principals.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't there be a lot more roles than # of principals? How many principals would a subject have normally? I wonder if we just use the default constructor which sizes the array to 16 might be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that maybe not all principals would map to a role. The case where a principal might map to multiple roles is unlikely in my experience.
Just some minor things, otherwise looks good 👍 |
c37756d
to
d597935
Compare
@wburns I addressed your comments (unless where noted) |
@tristantarrant more test failures... |
@danberindei only one test failure is security related, but it's to do with JGroups SASL authentication, so not connected to this PR. |
- use an internal cache instead of the Cluster Registry - add a GlobalSecurityManager with the ability to flush acl caches - don't cache per-cache acl masks - add a role check in addition to the existing permission check
d597935
to
32bf1df
Compare
Integrated at long last! Thanks Tristan! |
https://issues.jboss.org/browse/ISPN-5790