[ELY-2872] Security roles lost following failover#2294
[ELY-2872] Security roles lost following failover#2294skyllarr wants to merge 2 commits intowildfly-security:2.6.xfrom
Conversation
|
@rsearls Please review, thank you! |
|
The tests for this change are here wildfly-security/elytron-web#285 |
darranl
left a comment
darranl
left a comment
There was a problem hiding this comment.
I am going to have a further look but I think we need to continue to find a solution that does not result in JAAS APIs leaking into the Elytron public APIs and also does not iterate all roles for all realm types as that will be a big performance overhead.
| */ | ||
| Principal getRealmIdentityPrincipal(); | ||
|
|
||
| default Subject getSubject() { |
There was a problem hiding this comment.
I am not keen on this part of the change, the RealmIdentity / SecurityIdentity APIs are deliberatly an alternative to using the JAAS Subject - I understand why the JAAS realm is Subject aware as it is based on JAAS but other realms should not have this added.
| return this.identityCache.apply(securityDomain); | ||
| } | ||
|
|
||
| private static class Roles implements Principal { |
There was a problem hiding this comment.
I actually have something coming the Jakarta EE 11 migration work that may provide an alternative for this.
| * @param realmIdentity | ||
| */ | ||
| public void setSubject(RealmIdentity realmIdentity) { | ||
| checkNotNullParam("realmIdentity", realmIdentity); |
There was a problem hiding this comment.
This naming feels off to me, when I see a setClassName method I expect that the argument being passed in is an instance of that class name AND it is being set on the object that was called - this looks more like a utility method doing something else.
| subject = new Subject(); | ||
| Set<Principal> principals = subject.getPrincipals(); | ||
| principals.add(realmIdentity.getRealmIdentityPrincipal()); | ||
| cachedIdentity.getRoles().forEach(role -> principals.add(new Roles(role))); |
There was a problem hiding this comment.
This call is going to end up being applicable to all security realms, whilst JAAS may pro-actively load all roles in advance for others it is intended to be much more dynamic - in general this is why we prefer checking an identity is in a required role rather than listing all roles.
For configurations with a large set of roles this is going to be a big overhead with the end result being this Subject that they don't actually care about being immediately dropped and garbage collected.
There was a problem hiding this comment.
@darranl Btw, you are the author of the commit where these changes come from, IIRC I took this commit from this PR #2247 , I just added a smaller commit on top of it. I thought the commit from the PR that @pedro-hos created was approved before so I did not really go through it, but I should have
3 participants
https://issues.redhat.com/browse/ELY-2872