RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Frank Yuan]

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