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.w...@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 >> >
