Move SecurityManager relevant parts to SecurityBridge #1068
Conversation
| public class PolicySecurityController extends SecurityController | ||
| { | ||
| @Deprecated | ||
| public class PolicySecurityController extends SecurityController { |
There was a problem hiding this comment.
Class made deprecated. No other code changes
| */ | ||
| @Deprecated | ||
| public class RhinoSecurityManager extends SecurityManager { |
There was a problem hiding this comment.
Class made deprecated. No other code changes
| public abstract class SecureCaller | ||
| { | ||
| @Deprecated | ||
| public abstract class SecureCaller { |
There was a problem hiding this comment.
Class made deprecated. No other code changes (Note: this class is used nowhere in Rhino, so possible used only by embedders)
| public class JavaPolicySecurity extends SecurityProxy | ||
| { | ||
| @Deprecated | ||
| public class JavaPolicySecurity extends SecurityProxy { |
There was a problem hiding this comment.
Class is deprecated. No other code changes
|
This looks OK to me and I think it's OK to have in 1.7.14 because it's preparing us better for the future. Do any of the other folks who are very familiar with the security manager want to try it out? |
| * Bridge to security relevant operations, that have to be handled with SecurityManager up to JDK | ||
| * 17. | ||
| * | ||
| * <p>Notice: With JEP411, the SecurityManager is deprecated. |
There was a problem hiding this comment.
Shouldn't the
tag be closed?
|
Shouldn't Context.get/setSecurityController() be marked as deprecated as well? And for all @deprecated annotations: should we not mark these api's as deprecated for removal? Or is that problematic as that is a Java 9 thing and we still target Java 8? |
|
I've added a commit where I've changed all @p-bakker I'm not sure, if it is required to make Context.get/setSecurityController deprecated. There may come security implementations with >=jdk18 where the current implementation can be reused. |
| * should be routed over this class, so that it could be easily replaced by an other implementation | ||
| * like {@link SecurityBridge_NoOp}. | ||
| * | ||
| * <p>This implementation should be work up to JDK17 |
There was a problem hiding this comment.
According to JEP 411, the only API that will not work out the box in Java 18 will be System.setSecurityManager() (to make it work, one needs to set java -Djava.security.manager=allow). Rhino only calls System.setSecurityManager() in SecurityControllerTest, so this implementation should work in Java 18 as well. It is unclear when the actual APIs are going to be removed.
I'd suggest setting java.security.manager=allow in the tests section of build.gradle (or I could do that in a separate PR) to deal with SecurityControllerTest, and avoid mentioning which Java release will/won't work.
| String[] classNames = { | ||
| "org.mozilla.javascript.SecurityBridge_custom", | ||
| // Check, if SecurityManager class exists | ||
| // TODO: Shoud we check JDK version here? |
There was a problem hiding this comment.
It is not known when the actual APIs are going to be removed, so I would not check the Java version.
| public class SecurityBridge_SecurityManager implements SecurityBridge { | ||
| private static final Permission allPermission = new AllPermission(); | ||
| /** | ||
| * Retrieves a system property within a privileged block. Use it only when the property is used | ||
| * from within Rhino code and is not passed out of it. | ||
| * | ||
| * @param name the name of the system property | ||
| * @return the value of the system property | ||
| */ | ||
| @Override | ||
| public String getSystemProperty(final String name) { | ||
| return AccessController.doPrivileged( | ||
| new PrivilegedAction<String>() { | ||
| @Override | ||
| public String run() { | ||
| return System.getProperty(name); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The getSystemProperty() method is public and belongs to a public class in an exported package (I mean once Rhino is modularised), allowing the retrieval of system properties with the privileges of Rhino. The comment says "Use it only when the property is used from within Rhino code and is not passed out of it" but the code is not enforcing it: any code with access to the classes in the classpath/modulepath would be allowed to do that.
Probably not a big deal, but I wonder if the security bridge classes could be made package-visible.
There was a problem hiding this comment.
This could be really a problem
p-bakker
added
the
Java Compatibility
|
This PR is probably outdated. Anyway, the SecurityManager stuff should be removed soon (with Rhino 2.0?) |
To be prepared for JEP411, I've started to move all security relevant stuff to "SecurityBridge_SecurityManager".
I've also made other classes deprecated, that relies on SecurityManger (like RhinoSecurityManager) or other deprecated classes, that are marked for removal in JDK17
There are now only two usages of deprecated AccessController left:
https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/ContextFactory.java#L338
https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/LazilyLoadedCtor.java#L86
This relates to
#1045