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

[Daniel Fuchs]

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

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)

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