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 #831

Open
arjantijms opened this issue Nov 11, 2024 · 7 comments
Open

Remove support for Security Manager #831

arjantijms opened this issue Nov 11, 2024 · 7 comments

Comments

@arjantijms
Copy link

According to the Jakarta EE 11 plan, we should have removed all references to the Java SE security manager APIs -> "Remove all use of SecurityManager" (emphasis mine). In CDI we forgot to do this.

As it is purely an implementation detail, I believe we can (and should) do this in a service release of the CDI API.

See also #830

cc @ivargrimstad @edburns

@Azquelt
Copy link
Contributor

Azquelt commented Nov 13, 2024

I'll add the analysis I put on the discussion on the PR:

  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 will still be present in Java 24 (at least according to JEP 486, section Advice to maintainers of libraries that support the Security Manager).

Given that there is currently no plan to remove the methods we do still use, I don't see any reason to change the code in the API jar that has already been released, particularly as nothing would prevent us from doing a patch release at some point in the future if this did become an actual problem for anyone.

@arjantijms
Copy link
Author

Given that there is currently no plan to remove the methods we do still use,

Can you eleborate on that? The plan AFAIK was firmly in place. We just forgot to act on it for CDI 4.1, right?

I don't see any reason to change the code in the API jar that has already been released

Would that be even possible? Maven central artefacts are immutable. I don't think we should change anything in a jar that was already released. We could easily do a 4.1.1. I just did two of such releases for the Faces API, and that was really no big deal at all.

@Azquelt
Copy link
Contributor

Azquelt commented Nov 13, 2024

Can you eleborate on that? The plan AFAIK was firmly in place. We just forgot to act on it for CDI 4.1, right?

There is no plan to remove the two methods that we use (System.getSecurityManager and AccessController.doPrivileged) from Java. Hence, there is no need to remove our code that calls them.

We could easily do a 4.1.1.

Sorry, I was meaning that I see no reason to do a patch release. As a user, I would generally consider a patch release to be bug fixes only and essentially a replacement for the previous release. I do not see a reason to do a 4.1.1 release. I don't deny that it would be easy, but I don't think we should be changing anything unnecessarily in a patch release.

@arjantijms
Copy link
Author

Hence, there is no need to remove our code that calls them.

I still don't understand, sorry for persisting here.

I meant; didn't we all agree on the Jakarta EE 11 plan, where we all committed to remove all references to the security manager in our APIs?

@Ladicek
Copy link
Contributor

Ladicek commented Nov 14, 2024

Well clearly many specifications (it's not just CDI, but also REST and others) didn't get the memo. I know I didn't. Communication is hard.

Apparently, the REST people agree with the opinions stated here -- this is an OK change for the next release, but there's no need to do a micro release of the current APIs just to remove references to the security manager. Honestly -- the security manager APIs are not being removed yet, and there's no public timeline for that, so it's just not a big deal.

@manovotn
Copy link
Contributor

Well clearly many specifications (it's not just CDI, but also jakartaee/rest#1262 and others) didn't get the memo. I know I didn't. Communication is hard.

It is hard indeed.
I think it might have easily been the case of this decision coming late in our cycle when we had the release basically done.
While I recall it being discussed during early stages of EE 11 development, I also remember that there was no decision at the time (possibly because nobody was sure if SM is going to stop working with 17 or not really).

As for maintenance release of 4.1, I am with Ladislav and Andrew - I don't think this is worth the hassle. I am not saying that it would be difficult, it's just that the presence or absence of this code has no effect on users consuming CDI APIs, regardless of what supported JDK version they pick.

IMO the important part is that we removed SM references for future CDI versions, where the actual removal of SM happens.

@arjantijms
Copy link
Author

and there's no public timeline for that, so it's just not a big deal.

Isn't that re-discussing something that was already decided? I mean, if it wasn't a big deal, why did we put it in the EE 11 plan as a major item?

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

No branches or pull requests

4 participants