[ELY-179] Initial implementation (JACC-based authorization) #138
Conversation
| } | ||
|
|
||
| try { | ||
| ElytronPolicyConfigurationFactory policyConfigurationFactory = (ElytronPolicyConfigurationFactory) PolicyConfigurationFactory.getPolicyConfigurationFactory(); |
There was a problem hiding this comment.
Should this maybe not be static/global? Would this prevent (for example) two WildFly EE containers from running within the same JVM?
There was a problem hiding this comment.
Good point. However, a JACC PolicyConfigurationFactory is actually static/global, created only once. Also, this is pretty much what we have today in WFLY, so it may not be an issue at all.
But I think we can better support two or more WFLY EE containers in the same JVM if we define a format to the contextID in order to avoid collisions and keep uniqueness. Need to check, but I think this is already done by WFLY when deploying modules ...
There was a problem hiding this comment.
What we have today is one of the many things which prevents us from embedding properly. I am quite firm that I want to reduce or eliminate any global realizations of container-specific things, so if there's any logical way to make this context-aware then we should be doing it.
There was a problem hiding this comment.
Ok, that method is no longer static/global.
Like I said, the whole problem to make this context-aware is that PolicyConfigurationFactory is static and global. So, if you are sharing the same ClassLoader in different WFLY EE instances in the same JVM, you'll always get the same instance. However, I suppose that two WFLY EE containers in the same JVM will not share the same ClassLoader, right ?
Another possible solution when sharing the same instance of PolicyConfigurationFactory between different WFLY EE containers running in the same JVM is to define a format to the contextID, so we can append the container's identifier to the contextID to avoid collisions and keep uniqueness.
|
There's some discussion to be had here so I'm marking this "hold" for now. |
| String contextID; | ||
|
|
||
| if (WildFlySecurityManager.isChecking()) { | ||
| contextID = doPrivileged(new GetContextIDAction()); |
There was a problem hiding this comment.
Since this action appears to be stateless, it should probably be constant as well, or better yet just use a method reference to PolicyContext::getContextID() instead of an action object.
|
Rebased. |
| @@ -48,7 +48,7 @@ | |||
| PermissionCollection mapPermissions(Principal principal, Set<String> roles); | |||
|
|
|||
| /** | |||
| * A default implementation that does nothing but returns an empty and read-only {@link PermissionCollection}. | |||
| * A default implementation that does nothing but returns an empty and read-only {@link Policy#UNSUPPORTED_EMPTY_COLLECTION}. | |||
There was a problem hiding this comment.
This change in specification is probably not warranted or desired - if this policy becomes problematic, we may have to replace it with a different empty one.
|
Otherwise seems OK to me. There are still some static globals that IMO should be changed but we can deal with that later. |
pedroigor
force-pushed
the
ELY-179
branch
2 times, most recently
from
5614467
to
9b73137
Compare
| checkNotNullParam("link", link); | ||
| checkIfInOpenState(); | ||
|
|
||
| synchronized (this.linkedPolicies) { |
There was a problem hiding this comment.
This however is not fine. You're synchronizing on the collection that the field points to, but you're updating that same field inside the block. Thus anyone reading the field may get an outdated copy.
pedroigor
force-pushed
the
ELY-179
branch
2 times, most recently
from
0deb183
to
8bb84f7
Compare
|
Seems OK to start with. |
[ELY-179] Initial implementation (JACC-based authorization)
References
Overview
Provides an implementation of the JSR-115, Java Authorization Contract for Containers (JACC).
Backward Compatibility in WildFly
These changes were tested to check backward compatibility with the existing support provided by PicketBox in WildFly. Testes were executed based on: