> -----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/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. >
Alright, I will make the tests run twice. To impl this, I have to add jtreg tags like the following: /* * @test * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest * @run testng/othervm -DrunSecMngr=true common.Bug6350682 * @run testng/othervm common.Bug6350682 */ And modify the Policy class accordingly. I am writing a small program to update the tests, will send the new version tomorrow... Frank > 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.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 > >>>> > >>> > >> > >
