Skip to content

Conversation

@ranuradh
Copy link
Contributor

@ranuradh ranuradh commented Jan 31, 2025

What's changed?

Java 21 recipe: org.openrewrite.java.migrate.DeprecatedSecurityManager

What's your motivation?

The default value of the java.security.manager system property has been changed to disallow since Java 18. Unless the system property is set to allow on the command line, any invocation of System.setSecurityManager(SecurityManager) with a non-null argument will throw an UnsupportedOperationException. You can set system property as Djava.security.manager=allow.
This recipe calls
System.setProperty("java.security.manager", "allow");
whenever
System.setSecurityManager(SecurityManager) is called

Reference : https://bugs.openjdk.org/browse/JDK-8271301 & https://bugs.openjdk.org/browse/JDK-8270380

Anyone you would like to review specifically?

@timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

AnuRam123 and others added 4 commits January 31, 2025 15:03
@ranuradh ranuradh self-assigned this Jan 31, 2025
@ranuradh ranuradh requested a review from timtebeek January 31, 2025 20:17
@ranuradh ranuradh added the recipe Recipe requested label Feb 3, 2025
@MBoegers
Copy link
Contributor

MBoegers commented Feb 4, 2025

Is adding a System.setProperty() call every time System.setSecurityManager(SecurityManager) is called, real the best general migration?
It more feels like silencing and would overwrite -Djava.security.manager=falsesettings with might be intended by users.
Can we find a better general migration?

@ranuradh
Copy link
Contributor Author

ranuradh commented Feb 4, 2025

Hi @MBoegers thanks for the feedback, I based my recipe on https://bugs.openjdk.org/browse/JDK-8271301) where For most applications, the maintainer can simply add `-Djava.security.manager=allow` to the launcher command before they remove the `setSecurityManager` call. Since I was not sure how to add -Djava.security.manager=allow to the launcher I figured calling System.setProperty() before the invocation of the call could work.

@MBoegers
Copy link
Contributor

MBoegers commented Feb 4, 2025

I understand and your path. I have to gig a little deeper, what's the ultimate migration path.
I think adding via setProperty is not the same as adding the -D parameter. Let me please think for a night or two 🙏

@ranuradh ranuradh requested a review from MBoegers February 4, 2025 17:26
@timtebeek
Copy link
Member

I"m not sure this approach would work; effectively you'd be trying to set a system property from a Java process that has already started, whereas I think this property should be set before the Java process is started. I don't think this is a change we can make a source-level adjustment for; this should change in the way the Java process is invoked after a migration to Java 21+. That's how I interpret the "Unless the system property is set to allow on the command line" above. I'd lean towards not having this as a recipe, as much as we appreciate the effort you've put in.

@timtebeek timtebeek removed their request for review February 5, 2025 19:57
@ranuradh
Copy link
Contributor Author

ranuradh commented Feb 5, 2025

Got it thanks will close the PR

@ranuradh ranuradh closed this Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

recipe Recipe requested

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants