RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Daniel Fuchs
daniel.fuchs at oracle.com
Wed Jul 27 12:52:41 UTC 2016
Hi Frank,
Please see my comments inline.
On 27/07/16 10:27, Frank Yuan wrote:
> Hi Daniel
>
> Would you like to have a look at the following changes before I finish all rework?
> It shows runWithAllPerm, and the handling for user.dir property in FilePolicy.java. The code like runWithTmpPermission is still
> kept, please ignore them, I will remove them finally.
>
> diff -r 1bfe60e61bad test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java Wed Jul 27 02:23:58 2016 -0700
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 only, as
> + * published by the Free Software Foundation.
> + *
> + * This code is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * version 2 for more details (a copy is included in the LICENSE file that
> + * accompanied this code).
> + *
> + * You should have received a copy of the GNU General Public License version
> + * 2 along with this work; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> + * or visit www.oracle.com if you need additional information or have any
> + * questions.
> + */
> +package jaxp.library;
> +
> +import static jaxp.library.JAXPTestUtilities.getSystemProperty;
> +
> +import java.io.FilePermission;
> +
> +import org.testng.ITestContext;
> +
> +/**
> + * This policy can access local XML files.
> + */
> +public class FilePolicy extends BasePolicy {
> +
> + @Override
> + public void onStart(ITestContext arg0) {
> + JAXPPolicyManager policyManager = JAXPPolicyManager.getJAXPPolicyManager(true);
> + String userdir = getSystemProperty("user.dir");
> + policyManager.addPermission(new FilePermission(userdir + "/-", "read, write, delete"));
I see that FilePermission allows white spaces but I believe the
canonical form would be "read,write,delete";
> + policyManager.addPermission(new FilePermission(System.getProperty("test.src") + "/-", "read"));
> + policyManager.addPermission(new FilePermission(userdir, "read"));
> + }
> +}
> diff -r 1bfe60e61bad test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java Wed Jul 27 02:23:58 2016 -0700
> @@ -0,0 +1,312 @@
[...]
> + private void setDefaultPermissions() {
> + //Permissions to set security manager and policy
> + addPermission(new SecurityPermission("getPolicy"));
> + addPermission(new SecurityPermission("setPolicy"));
> + addPermission(new RuntimePermission("setSecurityManager"));
> + //Properties that jtreg and TestNG require
> + addPermission(new PropertyPermission("testng.show.stack.frames", "read"));
> + addPermission(new PropertyPermission("test.src", "read"));
> + addPermission(new PropertyPermission("test.classes", "read"));
> + addPermission(new PropertyPermission("dataproviderthreadcount", "read"));
> + addPermission(new PropertyPermission("experimental", "read"));
> +
> + //addPermission(new PropertyPermission("fileStringBuffer", "read"));
> + /*
> + addPermission(new RuntimePermission("getClassLoader"));
> + addPermission(new RuntimePermission("createClassLoader"));
> + addPermission(new RuntimePermission("createSecurityManager"));
> + addPermission(new RuntimePermission("modifyThread"));
> + addPermission(new PropertyPermission("*", "read, write"));
> + addPermission(new ReflectPermission("suppressAccessChecks"));
> + addPermission(new RuntimePermission("setIO"));
> + addPermission(new RuntimePermission("setContextClassLoader"));
> + addPermission(new RuntimePermission("accessDeclaredMembers"));*/
Good to see these permissions are now commented: make sure to remove
the commented code in the final version.
> + /*
> + * set allowAll in current thread context.
> + */
> + void setAllowAll(boolean allow) {
> + policy.setAllowAll(allow);
> + }
I'd suggest to return the previous value of allowAll here.
This will be more convenient to restore the previous value
in the finally block.
Since setters usually don't return values you might name it e.g.
/**
* Switches allowAll to the given value and return its
* previous value in current thread context.
* <p>
* Example of use:
* {@code
* final boolean before = switchAllowAll(true);
* try {
* // do some privileged operation here
* } finally {
* // restore allowAll to its previous value
* switchAllowAll(before);
* }
* }
*/
boolean switchAllowAll(boolean allowAll) {
// alternatively you could add a switchAllowAll method
// to TestPolicy
final boolean before = policy.allowAll();
policy.setAllowAll(allowAll);
return before;
}
[..]
> + /**
> + * Run the RunnableWithException with creating a JAXPPolicyManager and
> + * assigning temporary permissions. It's not thread-safe to use this
> + * function.
> + *
> + * @param r
> + * RunnableWithException to execute
> + * @param ps
> + * assigning permissions to add.
> + */
> + public static void tryRunWithPolicyManager(RunnableWithException r, Permission... ps) throws Exception {
> + JAXPPolicyManager policyManager = JAXPPolicyManager.getJAXPPolicyManager(true);
> + if (policyManager != null)
> + Stream.of(ps).forEach(p -> policyManager.addTmpPermission(p));
> + try {
> + r.run();
> + } finally {
> + JAXPPolicyManager.teardownPolicyManager();
> + }
> + }
This method is suspicious because if there existed a JAXPPolicyManager
before entering it it will be removed after the method finishes.
I'd suggest removing this method. You can accomplish the same things
by calling:
JAXPPolicyManager policyManager =
JAXPPolicyManager.getJAXPPolicyManager(true);
runWithTmpPermission(r, ps);
since runWithTmpPermission seems to do things right.
> +
> + /**
> + * Run the runnable with assigning temporary permissions. This won't impact
> + * global policy.
> + *
> + * @param r
> + * Runnable to run
> + * @param ps
> + * assigning permissions to add.
> + */
> + public static void runWithTmpPermission(Runnable r, Permission... ps) {
> + JAXPPolicyManager policyManager = JAXPPolicyManager.getJAXPPolicyManager(false);
> + List<Integer> tmpPermissionIndexes = new ArrayList();
> + if (policyManager != null)
> + Stream.of(ps).forEach(p -> tmpPermissionIndexes.add(policyManager.addTmpPermission(p)));
> + try {
> + r.run();
> + } finally {
if policyManager is null this will throw NPE.
Maybe you shoud make addTmpPermission and removeTmpPermission static
in JAXPPolicyManager, and delegate the null checking of the global
policyManager instance inside those methods.
> + tmpPermissionIndexes.forEach(index -> policyManager.removeTmpPermission(index));
> + }
> + }
> +
> + /**
> + * Run the supplier with all permissions. This won't impact global policy.
> + *
> + * @param s
> + * Supplier to run
> + */
> + public static <T> T runWithAllPerm(Supplier<T> s) {
> + Optional<JAXPPolicyManager> policyManager = Optional.ofNullable(JAXPPolicyManager
> + .getJAXPPolicyManager(false));
> + policyManager.ifPresent(manager -> manager.setAllowAll(true));
> + try {
> + return s.get();
> + } finally {
> + policyManager.ifPresent(manager -> manager.setAllowAll(false));
This should restore allowAll to its previous value rather than 'false'.
Maybe you should make setAllowAll/switchAllowAll static in
policyManager. The allowAll ThreadLocal can be static as well, so you
don't really to have an instance of JAXPPolicyManager or TestPolicy to
switch it on or off.
> + }
> + }
> +
> + /**
> + * Run the Runnable with all permissions. This won't impact global policy.
> + *
> + * @param s
> + * Supplier to run
> + */
> + public static void runWithAllPerm(Runnable r) {
> + Optional<JAXPPolicyManager> policyManager = Optional.ofNullable(JAXPPolicyManager
> + .getJAXPPolicyManager(false));
> + policyManager.ifPresent(manager -> manager.setAllowAll(true));
> + try {
> + r.run();
> + } finally {
> + policyManager.ifPresent(manager -> manager.setAllowAll(false));
This should restore allowAll to its previous value rather than 'false'.
>
> diff -r 1bfe60e61bad test/javax/xml/jaxp/unittest/transform/Bug6490921.java
> --- a/test/javax/xml/jaxp/unittest/transform/Bug6490921.java Mon Apr 04 14:54:38 2016 -0700
> +++ b/test/javax/xml/jaxp/unittest/transform/Bug6490921.java Wed Jul 27 02:22:40 2016 -0700
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2014, 2016, Oracle and/or its affiliates. All rights reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -23,6 +23,8 @@
>
> package transform;
>
> +import static jaxp.library.JAXPTestUtilities.setSystemProperty;
> +
> import java.io.IOException;
> import java.io.StringReader;
> import java.io.StringWriter;
> @@ -37,6 +39,7 @@
> import javax.xml.transform.stream.StreamResult;
>
> import org.testng.Assert;
> +import org.testng.annotations.Listeners;
> import org.testng.annotations.Test;
> import org.xml.sax.InputSource;
> import org.xml.sax.SAXException;
> @@ -46,6 +49,7 @@
> * @bug 6490921
> * @summary Test property org.xml.sax.driver is always applied in transformer API.
> */
> + at Listeners({jaxp.library.BasePolicy.class})
> public class Bug6490921 {
>
> public static class ReaderStub extends XMLFilterImpl {
> @@ -71,7 +75,7 @@
> public void test01() {
> String xml = "<?xml version='1.0'?><root/>";
> ReaderStub.used = false;
> - System.setProperty("org.xml.sax.driver", "");
> + setSystemProperty("org.xml.sax.driver", "");
>
> // Don't set 'org.xml.sax.driver' here, just use default
> try {
> @@ -91,7 +95,7 @@
> public void test02() {
> String xml = "<?xml version='1.0'?><root/>";
> ReaderStub.used = false;
> - System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
> + setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
> try {
> TransformerFactory transFactory = TransformerFactory.newInstance();
> Transformer transformer = transFactory.newTransformer();
> @@ -111,7 +115,7 @@
> + " <xsl:template match='/'>Hello World!</xsl:template>\n" + "</xsl:stylesheet>\n";
>
> ReaderStub.used = false;
> - System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
> + setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
> try {
> TransformerFactory transFactory = TransformerFactory.newInstance();
> if (transFactory.getFeature(SAXTransformerFactory.FEATURE) == false) {
>
I think the test looks fine.
cheers,
-- daniel
> Thanks
> Frank
>
> -----Original Message-----
> From: Daniel Fuchs [mailto:daniel.fuchs at oracle.com]
> Sent: Tuesday, July 26, 2016 3:46 PM
> To: Frank Yuan; 'huizhe wang'
> Cc: 'Amy Lu'; 'core-libs-dev'
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
>
> On 26/07/16 04:24, Frank Yuan wrote:
>> Thank you very much for your suggestions! Now I fully understand the rule(at least I think so :P)
>> I will use a runWithAllPerm block surrounding the user setup code as Daniel's way. Btw, Daniel, ThreadLocal should not need Atomic
>> any more, correct?
>>
>
> Hi Frank,
>
> runWithAllPerm is another way to do it.
> It uses a ThreadLocal<Permissions>, right?
>
> I agree it's adequate. Just be careful of what might
> happen if you run runWithAllPerm inside runWithPermissions,
> or runWithPermissions(runnable, a,b,c) with a runnable that
> later calls runWithPermission(runnable2, a, d, e) further down
> the road.
>
> At the moment I'm not sure whether your code will work correctly
> in the presence of such nested invocation (maybe it does),
> but because it seems to be index based it's not immediately
> obvious (I'm not asking you to change it - just to verify and
> confirm that it's something you have taken into account, and
> that you're confident that it works).
>
>
> Best regards,
>
> -- daniel
>
More information about the core-libs-dev
mailing list