search

jakartaee / cdi

CDI specification
Apache License 2.0
215 stars 78 forks source link

remove support for Security Manager #830

Closed Ladicek closed 2 weeks ago

Ladicek commented 2 weeks ago

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

arjantijms commented 2 weeks ago

@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. https://github.com/jakartaee/security/commit/8f4adddd524dc6c7384576872411bb2ac574b668

Ladicek commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 (https://github.com/weld/core/pull/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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.