RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Frank Yuan
frank.yuan at oracle.com
Wed Aug 3 10:12:55 UTC 2016
Hi Joe
Thank you for your review!
I updated the tests according to the latest comments as well as had unittest/transform/TransformerTest.java up to date, please check
http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/
Thanks
Frank
-----Original Message-----
From: Joe Wang [mailto:huizhe.wang at oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 8/2/16, 5:30 AM, Daniel Fuchs wrote:
> Hi Frank,
>
> I am intrigued by these change which do not seem
> to have anything to do with the rest. I mean, I'm not opposed
> as long as they are intended and that Joe validates them:
> ========================================================
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/ptests/XMLReaderNSTableTest.java.fram
es.html
>
>
> 118 spf.setNamespaceAware(false);
Not sure why this was changed.
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra
mes.html
>
The test itself wasn't very clear on the content it tests. If it only
verifies whether a resolver is used as is said, then this and related
changes in ResolverTest and publish.xml are fine. To the extent it
verifies, it didn't have to output to a file.
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.h
tml
>
We have to let Frank explain why namespace was turned off for these
tests :-) In this case, XMLReaderNSTableTest. In general, I would say
enabling security shouldn't change tests or gold files in this case.
>
> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/publish.xml.frames.html
>
> This looks like a desirable change - but what does it have to do
> with enabling security manager?
Probably to remove the reference to that particular server? Seems to be
fine as a cleanup. Again, it (the original test) only verifies a resolve
is used, it didn't even need to write out a file to prove that.
>
> Also:
> ========================================================
>
> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/unittest/transform/XSLTFunctionsTest.java.frames.html
>
>
> 70
> tf.setFeature("http://www.oracle.com/xml/jaxp/properties/enableExtensionFunctions",
> true);
>
> Is this needed only when there is a security manager?
Yes, when Security Manager is present, this feature is turned off by
default.
>
> ========================================================
>
> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java.html
>
>
>
> 119 /*
> 120 addPermission(new RuntimePermission("getClassLoader"));
> 121 addPermission(new RuntimePermission("createClassLoader"));
> 122 addPermission(new
> RuntimePermission("createSecurityManager"));
> 123 addPermission(new RuntimePermission("modifyThread"));
> 124 addPermission(new PropertyPermission("*", "read,write"));
> 125
> 126 addPermission(new RuntimePermission("setIO"));
> 127 addPermission(new
> RuntimePermission("setContextClassLoader"));
> 128 addPermission(new
> RuntimePermission("accessDeclaredMembers"));*/
>
>
> Please remove before pushing.
>
>
> ========================================================
>
> test/javax/xml/jaxp/internaltest/javax/xml/transform/cli/tigertest-in.xml
> test/javax/xml/jaxp/internaltest/javax/xml/transform/cli/tigertest.xsl
>
> It seems these two files have been removed, but shouldn't they have
> been moved to test/javax/xml/jaxp/internaltest/javax/xml/transform
> instead?
> I see they are used by
> test/javax/xml/jaxp/internaltest/javax/xml/transform/CLITest.java
>
> Did you forget to hg add them?
>
> ========================================================
>
>
> Otherwise the new JAXPPolicyManager & its Policy implementation
> look good. This is much simpler and better than the first
> iteration :-)
>
> This was a *very* long patch - so congratulations for seeing
> this through!
Very long patch, indeed! Thanks for the great effort! Very well done
with the unified Policy/Permission management.
Best,
Joe
>
> cheers,
>
> -- daniel
>
>
> On 02/08/16 11:20, Frank Yuan wrote:
>> Hi Joe and Daniel
>>
>> I have finished the rework as your comments, please check
>> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/
>>
>> JAXP tests use Policy classes, as well as 3 other patterns provided
>> by JAXPTestUtilities:
>> 1. runWithAllPerm methods, are only used for user setup code, never
>> run jaxp code with them.
>> 2. tryRunWithPolicyManager method, is used to run some test methods
>> with security manager and run the others without security
>> manager in single test class
>> 3. runWithTmpPermission methods, which may run jaxp code with some
>> limited permissions, those permissions belong to user permissions
>> and jaxp code won't doPrivileged for them. E.g.
>> PropertyPermission("MyInputFactory", "read") or
>> FilePermission("/tmp/this/does/not/exist/but/that/is/ok", "read")
>>
>>
>> Btw, some tests or test methods are disabled or commented sm support
>> temporarily for some known jaxp security bugs.
>>
>> Thanks
>> Frank
>>
>
More information about the core-libs-dev
mailing list