Skip to content
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

remove support for Security Manager #830

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 11, 2024

The Security Manager was deprecated for removal in Java 17 (see JEP 411). It is about time to remove support for it in CDI.

The Security Manager was deprecated for removal in Java 17 (see JEP 411).
It is about time to remove support for it in CDI.
@Ladicek Ladicek added this to the CDI 5.0 milestone Nov 11, 2024
@arjantijms
Copy link

@Ladicek Can you backport these changes to 4.1? We were actually required to do this for Jakarta EE 11, and I've done so for the APIs I'm directly responsible for. See e.g. jakartaee/security@8f4addd

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 11, 2024

I'm honestly not sure that's necessary. This doesn't affect the API surface at all, it's just an implementation detail.

This feels similar to the compilation level -- even though Jakarta EE 11 minimum compilation level was Java 17, the CDI 4.1 API is compiled to Java 11 :-) CDI 5.0 API will be compiled to Java 17, which feels like a perfect moment to remove Security Manager support, because 17 is the version in which it was deprecated.

@arjantijms
Copy link

I'm honestly not sure that's necessary.

I'm on the other hand 100% sure this is fully necessary. How can we break this tie? This is not just a nice-to-have but an absolute must. I'm quite suprised you have even the slightest doubt about this.

@manovotn
Copy link
Contributor

I'm honestly not sure that's necessary.

I'm on the other hand 100% sure this is fully necessary. How can we break this tie? This is not just a nice-to-have but an absolute must. I'm quite suprised you have even the slightest doubt about this.

@arjantijms why do you need to backport this?
What problems is the presence of this code in 4.1 causing? Or is it just a matter of preference?

While I can say that Weld 6 (targetting CDI 4.1, EE 11) already removed it's SM parts (weld/core#2983), I don't see an actual issue with keeping the code as is for older versions and just removing it for the next.

@arjantijms
Copy link

@arjantijms why do you need to backport this?

It needs to be backported first and foremost because it was an oversight for the release of 4.1. According to the Jakarta EE 11 plan, all APIs had to remove their usage of the security manager.

What problems is the presence of this code in 4.1 causing?

Running any Jakarta EE 11 implementation on JDK 24 and above is the most pressing problem. JDK 24 will be released next March, long before Jakarta EE 12 / CDI 4.2/5.0 will be available.

@manovotn
Copy link
Contributor

manovotn commented Nov 11, 2024

Running any Jakarta EE 11 implementation on JDK 24 and above is the most pressing problem. JDK 24 will be released next March, long before Jakarta EE 12 / CDI 4.2/5.0 will be available.

JDK 24 should disallow enablement of SM, but IIUIC, the plan was to still keep the APIs in place meaning it would not really cause any problems. Here's the JEP I found - https://openjdk.org/jeps/486
Maybe I misundestood it?

@Azquelt
Copy link
Contributor

Azquelt commented Nov 11, 2024

@arjantijms The situation here is quite different from the Jakarta EE Security commit that you linked.

  1. We have no spec requirements for SecurityManager
  2. Our API classes do not extend, implement or reference in their method signatures any SecurityManager-related classes
    • In the worst case, I think this means that an implementation could use their own versions of these API classes with all mention of SecurityManager removed and still pass the TCK and signature tests.
  3. The only methods we do call within our API classes are System.getSecurityManager and AccessController.doPrivileged. These methods are still supported in Java 24 (at least according to JEP 486 that @manovotn linked above, section Advice to maintainers of libraries that support the Security Manager).

If you still think we need to make similar changes in a patch release for CDI 4.1, it probably makes sense to open a separate issue for it.

@arjantijms
Copy link

If you still think we need to make similar changes in a patch release for CDI 4.1, it probably makes sense to open a separate issue for it.

In this particular case, I think removing the security manager always makes sense. I do appreciate that CDI can be used standalone (without any other Jakarta EE APIs), but other than that usecase it would be (IMHO) a disservice to our users to let them think they can still use the security manager with our APIs.

As it's an implementation detail in the CDI APIs, what would be the problem with removing them in a service release? If we (and myself specifically) would have been more dligent, those references would have been removed anyway as we planned.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 11, 2024

I think we made the call relatively early on to do a CDI 4.1 in Jakarta EE 11, and since that was a minor bump, that meant no breaking changes. We're doing a major bump in Jakarta EE 12, which means breaking changes allowed. As far as I am concerned, removing Security Manager support is a breaking change, no matter how small, and so should be reserved to CDI 5.0.

@arjantijms
Copy link

As far as I am concerned, removing Security Manager support is a breaking change

Almost all other specs that removed the Security Manager support did not update their major version just because of that. We have a ton of minor updates in EE 11 which removed the Security Manager support.

@manovotn manovotn merged commit c057683 into jakartaee:main Nov 11, 2024
3 checks passed
@Ladicek Ladicek deleted the remove-security-manager branch November 11, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants