RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Frank Yuan
frank.yuan at oracle.com
Mon Aug 8 09:11:09 UTC 2016
Thank you! I pushed the change into jaxp repo.
Frank
> -----Original Message-----
> From: Joe Wang [mailto:huizhe.wang at oracle.com]
> Sent: Saturday, August 06, 2016 1:59 AM
> To: Frank Yuan
> Cc: 'Daniel Fuchs'; 'core-libs-dev'
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
>
> Hi Frank,
>
> Looks good overall. Thanks for adding runs with/without SM, that's a lot
> of tedious work! I wish there's a way to do this in a general
> configuration. But it's fine since it does serve the purpose.
>
> Thanks,
> Joe
>
> On 8/5/16, 6:26 AM, Frank Yuan wrote:
> > Hi Joe and Daniel
> >
> > Please review the update: http://cr.openjdk.java.net/~fyuan/8067170/webrev.04/
> > In this version, I
> > 1. made all tests run twice, to pass in the different argument to the jtreg TestNG test, it has to run in othervm(jaxp test also
run
> > in othervm before this but it's possible to run in agentvm), however, I didn't delete the code supporting agentvm from
> > JAXPPolicyManager.java because jtreg may provide more support in agentvm mode someday:)
> > 2. enabled sm in unittest/catalog/CatalogTest.java as jdk-8161176's conclusion
> > 3. fixed the bug in runWithTmpPermission methods, Daniel mentioned it but I didn't understand at that time :P
> >
> > Thanks
> > Frank
> >
> >> -----Original Message-----
> >> From: Frank Yuan [mailto:frank.yuan at oracle.com]
> >> Sent: Thursday, August 04, 2016 6:06 PM
> >> To: 'Joe Wang'; 'Daniel Fuchs'
> >> Cc: 'core-libs-dev'
> >> Subject: RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
> >>
> >>
> >>
> >>> -----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/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.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/fu
> >> nctional/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/fu
> >> nctional/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/fu
> >> nctional/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/fu
> >> nctional/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/un
> >> ittest/transform/XSLTFunctionsTest.java.frames.html
> >>>>>>
> >>>>>>
> >>>>>> 70
> >>>>>>
> >> tf.setFeature("http://www.oracle.com/xml/jaxp/properties/enableExtensionFu
> >> nctions",
> >>>>>> 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/li
> >> bs/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
> >>>>>>>
More information about the core-libs-dev
mailing list