[#] NO-JIRA: SecurityManager Shim and MRJAR #1534
Conversation
…yManager directly
| * This specific class uses legacy methods toward usage on Java 17 - 23. | ||
| * | ||
| * The API of this class must be kept the same as the Java 24+ implementation | ||
| * variant of this class which can be found at: | ||
| * src/main/java24/org/apache/activemq/artemis/utils/sm/SecurityManagerShim.java |
There was a problem hiding this comment.
All of the Javadoc in this class is inaccurate now as you havent updated it to reflect how/where it is being used.
There was a problem hiding this comment.
Yes, JIRA was out yesterday, but I can create the issue. Need to check with @jbonofre if there is a parent issue I need to create a sub-task.
And also yes, I need to update the entire javadoc. That was a plain copy to follow up on the email thread.
There was a problem hiding this comment.
Yeah the ASF has been under DDoS attack for the last couple days, but Jira would usually load after another attempt or two if it failed initially.
There was a problem hiding this comment.
@jeanouii no need to do a JIRA. Keep the convo here.
There was a problem hiding this comment.
The Jira is for issue and commit tracking, not discussion of the code. Changes to make things work on Java 24+ when they don't currently, seems pretty clearly deserving of being committed against a Jira. Almost everything should be.
There was a problem hiding this comment.
This PR is definitely going to need a Jira before it is merged but that can be done before it is ready to merge
| if (audit != OFF) { | ||
| // [AMQ-9563] TODO: JDK 21 use Subject.current() instead | ||
| Subject subject = Subject.getSubject(AccessController.getContext()); | ||
| Subject subject = SecurityManagerShim.currentSubject(); |
There was a problem hiding this comment.
Do we really need the shim? I'm wondering if we just ship two AnnotatedBean classes using MR jar and switch the JAAS API implementation when getting the Subject.
There was a problem hiding this comment.
If there are no other SM / related API usage to handle then you wouldn't need it all, just the Subject method.
However I'm not seeing how duplicating a sizable complex class that may need to be updated, just to change a couple lines, would really be better than having 2 new tiny classes , that likely won't ever change, to do the switch out and avoid the need to duplicate the complex class?
For sure the classes don't need to be in their own module if that's what you dislike about it.
There was a problem hiding this comment.
If we only need a small part of the Shim around handling Subject then we should just use that piece for now to keep the changes minimal.
|
Updated approach w/ all three changes here: #1533
Two Jenkins jobs running on Apache CI, one with JDK 17 and another kicked off using JDK 25 |
|
@mattrpav We can probably close this PR then and keep going with what you pulled in your own branch/PR |
4 participants
This PR leverages Matt's PR [#] NO-JIRA: Update Jenkinsfile to add JDK 25](#1533)
It is meant to check CI status on tests and gather community feedback. We might prefer to create a PR against Matt's branch to join efforts.