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

[Joe Wang]

On 8/3/16, 3:45 AM, Daniel Fuchs wrote:
> Hi Frank,
>
> I looked at the diffs of the diffs between webrev.02 and webrev.03 and
> it looks good to me.
>
> +1 - provided all tests pass :-)

+1, thanks for re-enabling the tests that had dependencies on 8162848. I 
also closed 8162848.

>
> But I have the same question than Joe: aren't all the test supposed
> to run twice: once with security manager and once without?
> (except for those test that might explicitly require a security manager)

I agree, all of the tests need to run with and without security manager. 
It would be good to combine the  runWithSecurityManager and 
runWithoutSecurityManager methods into one with a condition that 
determines whether a security manager is present.

Best,
Joe

>
> best regards,
>
> -- daniel
>
> On 03/08/16 11:12, Frank Yuan wrote:
>> 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
>>>>
>>>
>>
>