RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Thank you! I pushed the change into jaxp repo. Frank > -Original Message- > From: Joe Wang [mailto:huizhe.w...@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.y...@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.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 > >>>>> > >>>>> > >>>>> > >>
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.y...@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.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/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
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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.y...@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.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/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
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.
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 throu
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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.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 JAXPTestUtilit
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel
> -Original Message-
> From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> tests
>
> 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:
I corrected, cleaned up some code during I addressed the major task, there are
also some others besides you found, I even couldn't
recall all of them now. Sorry it confused you...
>
>
>
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);
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra
me
> s.html
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.h
t
> ml
>
> 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?
>
They are unrelated to sm enabling, XMLReaderNSTableTest was not added with
@Test, so it was never run, after I added, I had to
correct the golden file to let it passed.
spf.setNamespaceAware(false); is redundant, I will replace with a comment line
to state NamespaceAware is false by default.
> 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?
>
>
>
> 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.
>
>
Yes, I will. Forgot yesterday...
>
>
> 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?
>
The new destination directory which CLITest.java is moved to, contains same
files. So don't need to move 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!
>
Really thank you very much for having reviewed my changes and given me so much
valuable comments!
Frank
> 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
> >
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.frames.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.frames.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.html
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
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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.frames.html
118 spf.setNamespaceAware(false);
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.frames.html
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.html
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?
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?
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!
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
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel
Thank you very much for your comments! Please check my reply inline below:
>
> Hi Frank,
>
> Please see my comments inline.
>
> On 27/07/16 10:27, Frank Yuan wrote:
> > Hi Daniel
> >
> > Would you like to have a look at the following changes before I finish all
> > rework?
> > It shows runWithAllPerm, and the handling for user.dir property in
> > FilePolicy.java. The code like runWithTmpPermission is still
> > kept, please ignore them, I will remove them finally.
> >
> > diff -r 1bfe60e61bad test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java
> > --- /dev/null Thu Jan 01 00:00:00 1970 +
> > +++ b/test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java Wed Jul 27
> > 02:23:58 2016 -0700
> > @@ -0,0 +1,44 @@
...
> > +addPermission(new RuntimePermission("setIO"));
> > +addPermission(new RuntimePermission("setContextClassLoader"));
> > +addPermission(new RuntimePermission("accessDeclaredMembers"));*/
>
> Good to see these permissions are now commented: make sure to remove
> the commented code in the final version.
>
Corrected, thanks!
> > +/*
> > + * set allowAll in current thread context.
> > + */
> > +void setAllowAll(boolean allow) {
> > +policy.setAllowAll(allow);
> > +}
>
> I'd suggest to return the previous value of allowAll here.
> This will be more convenient to restore the previous value
> in the finally block.
> Since setters usually don't return values you might name it e.g.
>
> /**
> * Switches allowAll to the given value and return its
> * previous value in current thread context.
> *
> * Example of use:
> * {@code
> *final boolean before = switchAllowAll(true);
> *try {
> * // do some privileged operation here
> *} finally {
> * // restore allowAll to its previous value
> * switchAllowAll(before);
> *}
> * }
> */
> boolean switchAllowAll(boolean allowAll) {
> // alternatively you could add a switchAllowAll method
> // to TestPolicy
> final boolean before = policy.allowAll();
> policy.setAllowAll(allowAll);
> return before;
> }
>
> [..]
switchAllowAll is more flexible, but I would take only one use case, that is
setAllowAll(true));
try {
do something
} finally {
setAllowAll(false);
}
> > +/**
> > + * Run the RunnableWithException with creating a JAXPPolicyManager and
> > + * assigning temporary permissions. It's not thread-safe to use this
> > + * function.
> > + *
> > + * @param r
> > + *RunnableWithException to execute
> > + * @param ps
> > + *assigning permissions to add.
> > + */
> > +public static void tryRunWithPolicyManager(RunnableWithException r,
> > Permission... ps) throws Exception {
> > +JAXPPolicyManager policyManager =
> > JAXPPolicyManager.getJAXPPolicyManager(true);
> > +if (policyManager != null)
> > +Stream.of(ps).forEach(p -> policyManager.addTmpPermission(p));
> > +try {
> > +r.run();
> > +} finally {
> > +JAXPPolicyManager.teardownPolicyManager();
> > +}
> > +}
>
> This method is suspicious because if there existed a JAXPPolicyManager
> before entering it it will be removed after the method finishes.
> I'd suggest removing this method. You can accomplish the same things
> by calling:
>
> JAXPPolicyManager policyManager =
> JAXPPolicyManager.getJAXPPolicyManager(true);
> runWithTmpPermission(r, ps);
>
> since runWithTmpPermission seems to do things right.
>
JAXPTestUtilities.tryRunWithPolicyManager(Runnable, Permission.)
is used for cases where some test methods need to be run with security
manager and some others need to be run without security manager because
they have different behaviors when having sm or not. E.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/FactoryFindTest.java.sdiff.html.
Such cases must run in single thread, all other cases don't require it, are
thread safe.
> > +
> > +/**
> > + * Run the runnable with assigning temporary permissions. This won't
> > impact
> > + * global policy.
> > + *
> > + * @param r
> > + *Runnable to run
> > + * @param ps
> > + *assigning permissions to add.
> > + */
> > +public static void runWithTmpPermission(Runnable r, Permission... ps) {
> > +JAXPPolicyManager policyManager =
> > JAXPPolicyManager.getJAXPPolicyManager(false);
> > +List tmpPermissionIndexes = new ArrayList();
> > +if (policyManager != null)
> > +Stream.of(ps).forEach(p ->
> > tmpPermissionIndexes.add(policyManager.addTmpPermission(p)));
> > +try {
> > +r.run();
> > +} finally {
>
> if policyManager is null this will throw NPE.
> Maybe you shoud make addTmpPermission and removeTmpPermission static
> in
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
> > +/*
> > + * Install a SecurityManager along with a default Policy to allow
> > testNG to
> > + * run when there is a security manager.
> > + */
> > +private JAXPPolicyManager() {
> > +// Backing up policy and security manager for restore
> > +policyBackup = Policy.getPolicy();
> > +smBackup = System.getSecurityManager();
> > +
> > +// Set customized policy
> > +setDefaultPermissions();
> > +Policy.setPolicy(policy);
> > +System.setSecurityManager(new SecurityManager());
> > +}
>
> Will the test suite be configured to run with and without
> SecurityManager? It seems to me, with a TestNG test listener, a
> SecurityManager is always installed. I don't seem to see it checks
> whether the test shall run with a SecurityManager.
>
> -Joe
>
It's same as the original code, I think it's to be ready for running in agent
mode.
Frank
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
y {
+policyManager.ifPresent(manager -> manager.setAllowAll(false));
+}
+}
+
+/**
+ * Acquire a system property.
+ *
+ * @param name
+ *System property name to be acquired.
+ * @return property value
+ */
+public static String getSystemProperty(String name) {
+return runWithAllPerm(() -> System.getProperty(name));
+}
+
+/**
+ * Set a system property by given system value.
+ *
+ * @param name
+ *System property name to be set.
+ * @param value
+ *System property value to be set.
+ */
+public static void setSystemProperty(String name, String value) {
+runWithAllPerm(() -> System.setProperty(name, value));
+}
+
+/**
+ * Clear a system property.
+ *
+ * @param name
+ *System property name to be cleared.
+ */
+public static void clearSystemProperty(String name) {
+runWithAllPerm(() -> clearSystemProperty(name));
+}
+
+/**
+ * Run the RunnableWithException with assigning temporary permissions. This
+ * won't impact global policy.
+ *
+ * @param r
+ *RunnableWithException to execute
+ * @param ps
+ *assigning permissions to add.
+ */
+public static void tryRunWithTmpPermission(RunnableWithException r,
Permission... ps) throws Exception {
+JAXPPolicyManager policyManager =
JAXPPolicyManager.getJAXPPolicyManager(false);
+List tmpPermissionIndexes = new ArrayList();
+if (policyManager != null)
+Stream.of(ps).forEach(p ->
tmpPermissionIndexes.add(policyManager.addTmpPermission(p)));
+try {
+r.run();
+} finally {
+tmpPermissionIndexes.forEach(index ->
policyManager.removeTmpPermission(index));
+}
+}
+
+@FunctionalInterface
+public interface RunnableWithException {
+void run() throws Exception;
+}
+
+/**
+ * Current test directory.
+ */
+public static final String USER_DIR = getSystemProperty("user.dir") +
FILE_SEP;;
+
}
diff -r 1bfe60e61bad test/javax/xml/jaxp/unittest/transform/Bug6490921.java
--- a/test/javax/xml/jaxp/unittest/transform/Bug6490921.javaMon Apr 04
14:54:38 2016 -0700
+++ b/test/javax/xml/jaxp/unittest/transform/Bug6490921.javaWed Jul 27
02:22:40 2016 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2016, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -23,6 +23,8 @@
package transform;
+import static jaxp.library.JAXPTestUtilities.setSystemProperty;
+
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
@@ -37,6 +39,7 @@
import javax.xml.transform.stream.StreamResult;
import org.testng.Assert;
+import org.testng.annotations.Listeners;
import org.testng.annotations.Test;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
@@ -46,6 +49,7 @@
* @bug 6490921
* @summary Test property org.xml.sax.driver is always applied in transformer
API.
*/
+@Listeners({jaxp.library.BasePolicy.class})
public class Bug6490921 {
public static class ReaderStub extends XMLFilterImpl {
@@ -71,7 +75,7 @@
public void test01() {
String xml = "";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", "");
+setSystemProperty("org.xml.sax.driver", "");
// Don't set 'org.xml.sax.driver' here, just use default
try {
@@ -91,7 +95,7 @@
public void test02() {
String xml = "";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
try {
TransformerFactory transFactory =
TransformerFactory.newInstance();
Transformer transformer = transFactory.newTransformer();
@@ -111,7 +115,7 @@
+ "Hello World!\n" +
"\n";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
try {
TransformerFactory transFactory =
TransformerFactory.newInstance();
if (transFactory.getFeature(SAXTransformerFactory.FEATURE) ==
false) {
Thanks
Frank
-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Tuesday, July 26, 2016 3:46 PM
To: Frank Yuan; 'huizhe wang'
Cc: 'Amy Lu'; 'core-libs-dev'
Subject: Re: RFR
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
.library.JAXPTestUtilities.setSystemProperty;
+
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
@@ -37,6 +39,7 @@
import javax.xml.transform.stream.StreamResult;
import org.testng.Assert;
+import org.testng.annotations.Listeners;
import org.testng.annotations.Test;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
@@ -46,6 +49,7 @@
* @bug 6490921
* @summary Test property org.xml.sax.driver is always applied in transformer
API.
*/
+@Listeners({jaxp.library.BasePolicy.class})
public class Bug6490921 {
public static class ReaderStub extends XMLFilterImpl {
@@ -71,7 +75,7 @@
public void test01() {
String xml = "";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", "");
+setSystemProperty("org.xml.sax.driver", "");
// Don't set 'org.xml.sax.driver' here, just use default
try {
@@ -91,7 +95,7 @@
public void test02() {
String xml = "";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
try {
TransformerFactory transFactory = TransformerFactory.newInstance();
Transformer transformer = transFactory.newTransformer();
@@ -111,7 +115,7 @@
+ " Hello World!\n" +
"\n";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
try {
TransformerFactory transFactory = TransformerFactory.newInstance();
if (transFactory.getFeature(SAXTransformerFactory.FEATURE) ==
false) {
I think the test looks fine.
cheers,
-- daniel
Thanks
Frank
-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Tuesday, July 26, 2016 3:46 PM
To: Frank Yuan; 'huizhe wang'
Cc: 'Amy Lu'; 'core-libs-dev'
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 26/07/16 04:24, Frank Yuan wrote:
Thank you very much for your suggestions! Now I fully understand the rule(at
least I think so :P)
I will use a runWithAllPerm block surrounding the user setup code as Daniel's
way. Btw, Daniel, ThreadLocal should not need Atomic
any more, correct?
Hi Frank,
runWithAllPerm is another way to do it.
It uses a ThreadLocal, right?
I agree it's adequate. Just be careful of what might
happen if you run runWithAllPerm inside runWithPermissions,
or runWithPermissions(runnable, a,b,c) with a runnable that
later calls runWithPermission(runnable2, a, d, e) further down
the road.
At the moment I'm not sure whether your code will work correctly
in the presence of such nested invocation (maybe it does),
but because it seems to be index based it's not immediately
obvious (I'm not asking you to change it - just to verify and
confirm that it's something you have taken into account, and
that you're confident that it works).
Best regards,
-- daniel
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
return runWithAllPerm(() -> System.getProperty(name));
+}
+
+/**
+ * Set a system property by given system value.
+ *
+ * @param name
+ *System property name to be set.
+ * @param value
+ *System property value to be set.
+ */
+public static void setSystemProperty(String name, String value) {
+runWithAllPerm(() -> System.setProperty(name, value));
+}
+
+/**
+ * Clear a system property.
+ *
+ * @param name
+ *System property name to be cleared.
+ */
+public static void clearSystemProperty(String name) {
+runWithAllPerm(() -> clearSystemProperty(name));
+}
+
+/**
+ * Run the RunnableWithException with assigning temporary permissions. This
+ * won't impact global policy.
+ *
+ * @param r
+ *RunnableWithException to execute
+ * @param ps
+ *assigning permissions to add.
+ */
+public static void tryRunWithTmpPermission(RunnableWithException r,
Permission... ps) throws Exception {
+JAXPPolicyManager policyManager =
JAXPPolicyManager.getJAXPPolicyManager(false);
+List tmpPermissionIndexes = new ArrayList();
+if (policyManager != null)
+Stream.of(ps).forEach(p ->
tmpPermissionIndexes.add(policyManager.addTmpPermission(p)));
+try {
+r.run();
+} finally {
+tmpPermissionIndexes.forEach(index ->
policyManager.removeTmpPermission(index));
+}
+}
+
+@FunctionalInterface
+public interface RunnableWithException {
+void run() throws Exception;
+}
+
+/**
+ * Current test directory.
+ */
+public static final String USER_DIR = getSystemProperty("user.dir") +
FILE_SEP;;
+
}
diff -r 1bfe60e61bad test/javax/xml/jaxp/unittest/transform/Bug6490921.java
--- a/test/javax/xml/jaxp/unittest/transform/Bug6490921.javaMon Apr 04
14:54:38 2016 -0700
+++ b/test/javax/xml/jaxp/unittest/transform/Bug6490921.javaWed Jul 27
02:22:40 2016 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2016, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -23,6 +23,8 @@
package transform;
+import static jaxp.library.JAXPTestUtilities.setSystemProperty;
+
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
@@ -37,6 +39,7 @@
import javax.xml.transform.stream.StreamResult;
import org.testng.Assert;
+import org.testng.annotations.Listeners;
import org.testng.annotations.Test;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
@@ -46,6 +49,7 @@
* @bug 6490921
* @summary Test property org.xml.sax.driver is always applied in transformer
API.
*/
+@Listeners({jaxp.library.BasePolicy.class})
public class Bug6490921 {
public static class ReaderStub extends XMLFilterImpl {
@@ -71,7 +75,7 @@
public void test01() {
String xml = "";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", "");
+setSystemProperty("org.xml.sax.driver", "");
// Don't set 'org.xml.sax.driver' here, just use default
try {
@@ -91,7 +95,7 @@
public void test02() {
String xml = "";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
try {
TransformerFactory transFactory = TransformerFactory.newInstance();
Transformer transformer = transFactory.newTransformer();
@@ -111,7 +115,7 @@
+ " Hello World!\n" +
"\n";
ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
try {
TransformerFactory transFactory = TransformerFactory.newInstance();
if (transFactory.getFeature(SAXTransformerFactory.FEATURE) ==
false) {
Thanks
Frank
-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Tuesday, July 26, 2016 3:46 PM
To: Frank Yuan; 'huizhe wang'
Cc: 'Amy Lu'; 'core-libs-dev'
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 26/07/16 04:24, Frank Yuan wrote:
> Thank you very much for your suggestions! Now I fully understand the rule(at
> least I think so :P)
> I will use a runWithAllPerm block surrounding the user setup code as Daniel's
> way. Btw, Daniel, ThreadLocal should not need Atomic
> any more, corre
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel I didn't state clear, actually, I want/wanted to take the absolutely same way as your allowAll except I am going to add a common function(called runWithAllPerm) to run it, no Permission arguments any longer. I will send you a draft to you and Joe to make it clear before I finish all rework. Thanks Frank > -Original Message- > From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] > Sent: Tuesday, July 26, 2016 3:46 PM > To: Frank Yuan; 'huizhe wang' > Cc: 'Amy Lu'; 'core-libs-dev' > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > On 26/07/16 04:24, Frank Yuan wrote: > > Thank you very much for your suggestions! Now I fully understand the > > rule(at least I think so :P) > > I will use a runWithAllPerm block surrounding the user setup code as > > Daniel's way. Btw, Daniel, ThreadLocal should not need Atomic > > any more, correct? > > > > Hi Frank, > > runWithAllPerm is another way to do it. > It uses a ThreadLocal, right? > > I agree it's adequate. Just be careful of what might > happen if you run runWithAllPerm inside runWithPermissions, > or runWithPermissions(runnable, a,b,c) with a runnable that > later calls runWithPermission(runnable2, a, d, e) further down > the road. > > At the moment I'm not sure whether your code will work correctly > in the presence of such nested invocation (maybe it does), > but because it seems to be index based it's not immediately > obvious (I'm not asking you to change it - just to verify and > confirm that it's something you have taken into account, and > that you're confident that it works). > > > Best regards, > > -- daniel
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 26/07/16 04:24, Frank Yuan wrote: Thank you very much for your suggestions! Now I fully understand the rule(at least I think so :P) I will use a runWithAllPerm block surrounding the user setup code as Daniel's way. Btw, Daniel, ThreadLocal should not need Atomic any more, correct? Hi Frank, runWithAllPerm is another way to do it. It uses a ThreadLocal, right? I agree it's adequate. Just be careful of what might happen if you run runWithAllPerm inside runWithPermissions, or runWithPermissions(runnable, a,b,c) with a runnable that later calls runWithPermission(runnable2, a, d, e) further down the road. At the moment I'm not sure whether your code will work correctly in the presence of such nested invocation (maybe it does), but because it seems to be index based it's not immediately obvious (I'm not asking you to change it - just to verify and confirm that it's something you have taken into account, and that you're confident that it works). Best regards, -- daniel
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Joe and Daniel
Thank you very much for your suggestions! Now I fully understand the rule(at
least I think so :P)
I will use a runWithAllPerm block surrounding the user setup code as Daniel's
way. Btw, Daniel, ThreadLocal should not need Atomic
any more, correct?
Frank
> -Original Message-
> From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
> Sent: Tuesday, July 26, 2016 8:47 AM
> To: huizhe wang; Frank Yuan
> Cc: 'Amy Lu'; 'core-libs-dev'
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> tests
>
> On 26/07/16 00:53, huizhe wang wrote:
> > To avoid having to grant the permission, the test may choose to read the
> > property before setting the SecurityManager. You might not be able to
> > use TestNG Listeners in such case, or maybe you can by initializing the
> > properties before the test starts.
>
> Or you can use my trick with an AtomicBoolean for such cases:
>
> set allowAll to true
> try {
> read system property
> } finally {
> set allowAll to false (or to the value it had before)
> }
>
> Adding a ThreadLocal allowAll to BasePolicy
> for that purpose is very easy :-)
>
> That should ensure that you only need to give those permissions
> to the test that a regular user of the JAXP API would need to
> use the JAXP API.
>
> If the test read/writes an XML document from file, then the
> FilePermission to read/write that document is something that
> a regular user of JAXP would need. So those permission should
> be granted to the test through Policy.
>
> If the test reads/writes a system property or creates a directory
> or create a class loader to set up an initial configuration for
> the test to run, then this is not something a regular user of the
> JAXP API would need - so it would then be legitimate to perform this
> setup inside a block that sets allowAll to true to locally escape
> permissions checks during this setup, thus avoiding to grant those
> permissions to all in the Policy (alternatively you could use your
> tmpPermissions trick to do that as well - but it is a bit more
> complex and adds more clutter than a simple on/off switch).
>
> cheers,
>
> -- daniel
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 26/07/16 00:53, huizhe wang wrote:
To avoid having to grant the permission, the test may choose to read the
property before setting the SecurityManager. You might not be able to
use TestNG Listeners in such case, or maybe you can by initializing the
properties before the test starts.
Or you can use my trick with an AtomicBoolean for such cases:
set allowAll to true
try {
read system property
} finally {
set allowAll to false (or to the value it had before)
}
Adding a ThreadLocal allowAll to BasePolicy
for that purpose is very easy :-)
That should ensure that you only need to give those permissions
to the test that a regular user of the JAXP API would need to
use the JAXP API.
If the test read/writes an XML document from file, then the
FilePermission to read/write that document is something that
a regular user of JAXP would need. So those permission should
be granted to the test through Policy.
If the test reads/writes a system property or creates a directory
or create a class loader to set up an initial configuration for
the test to run, then this is not something a regular user of the
JAXP API would need - so it would then be legitimate to perform this
setup inside a block that sets allowAll to true to locally escape
permissions checks during this setup, thus avoiding to grant those
permissions to all in the Policy (alternatively you could use your
tmpPermissions trick to do that as well - but it is a bit more
complex and adds more clutter than a simple on/off switch).
cheers,
-- daniel
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 7/25/2016 2:46 AM, Frank Yuan wrote:
-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 7/22/2016 5:53 AM, Daniel Fuchs wrote:
On 22/07/16 10:15, Frank Yuan wrote:
Hi Daniel
Thank you very much for your review and the comments!
-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
unit tests
Hi Frank,
I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:
These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.
Very true. Some of these permissions came from our standalone JAXP tests
that were granted to ant and junit.
Yes, I agree completely. I would control the permission assignment
more tightly:
1. only allow the following necessary permissions in default:
addPermission(new SecurityPermission("getPolicy"));
addPermission(new SecurityPermission("setPolicy"));
addPermission(new RuntimePermission("setSecurityManager"));
These are safe for the current code base. So may be add a note to remind
for any future changes.
2. other permissions in current default set are commonly used for the
tests, so I would add more Policy classes like existing
FilePolicy, etc.
//ClassLoaderPolicy, an individual test may only require some of
them, but I would put them in only one class if you agree
addPermission(new RuntimePermission("getClassLoader"));
addPermission(new RuntimePermission("createClassLoader"));
addPermission(new RuntimePermission("modifyThread"));
How about you think?
My understanding is that you intend to grand specific permissions
limited to the test that will extend these policies, e.g. FilePolicy. I
think this is ok and an improvement.
It would definitely improve things - but then all the classes
in the test that runs with this new policy class will inherit
from these permissions as well. This may or may not be an issue.
(I confess I haven't looked at all the webrev ;-()
I will reduce the scope of the extra permissions as possible, and make sure the
safety, e.g. surround the user code(i.e. the test
code) which requires a permission explicitly with corresponding permission
assignment
Yeah, it would be good to make sure a policy is safe with the code. If
both the test code and the code may need a permission, we may have a
conflict that we need to solve. It may be good to start with the basic
permission, and add only if required by the test code, with a note
preferably noting what exactly is needed. "DefaultFeaturesTest" in this
regard, doesn't seem to require FilePolicy, but
CatalogReferCircularityTest requires permission to where the source
files are located, in this case, it would be good to make it specific.
For example, instead of being called "FilePolicy", it may be clearer to
add a parameter so that it's known where the File permission is given
(e.g. the source dir in this case).
Currently FilePolicy(maybe it's better to rename to TestFilePolicy) has full
access permission to user.dir and read permission to
test.src, I think they belong to user permission, jaxp lib won't doPrivileged
on this.
Btw, it's unable to add parameter in @Listeners({ xxx.class }) unless using
more tricky and complex means.
I believe I will meet problem to identify if a doPrivileged is missing when I
strip the extra permissions and then solve the test
failures case by case, so I would ask you when should we add doPrivileged in
jaxp/jdk library code?
1. Should doPrivileged only for where the permission can't be granted to the
user code, e.g. read some secret system property
2. Or should doPrivileged for where the permission needn't be granted to the
user code, e.g. read some internal property
The library code shall have *minimal permission* to perform operations
as required by the *specification/javadoc*. The tests should avoid
blanket permission so as to expose issues in the code they test.
For example, the Catalog API specified several System properties. It
therefore shall give privilege to reading those and only those System
properties. To verify that the code performs properly when Security
Manager is present, a blanket permission, e.g. new
PropertyPermission("*", "read, write"), should be avoided. Note that
when this permission is removed from the BasePolicy, FilePolicy won't
run since it requires permission to read "user.dir". To avoid having to
grant the permission, the test may choose to read the property before
setting the S
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
> -Original Message-
> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> tests
>
>
>
> On 7/22/2016 5:53 AM, Daniel Fuchs wrote:
> > On 22/07/16 10:15, Frank Yuan wrote:
> >> Hi Daniel
> >>
> >> Thank you very much for your review and the comments!
> >>
> >>> -Original Message-
> >>> From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
> >>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
> >>> unit tests
> >>>
> >>> Hi Frank,
> >>>
> >>> I see that in order to be able to run the tests, you were forced
> >>> to add a few permissions that the test/test infrastructure need
> >>> to setup things:
> >>>
> >>>
> >>> These are quite powerful permissions, and adding them by default
> >>> also means that you might miss a bug - if e.g. a doPrivileged is
> >>> missing somewhere in the JAXP code when jaxp tries to e.g. get/create
> >>> a classloader, or read a system property, you might not see
> >>> it.
>
> Very true. Some of these permissions came from our standalone JAXP tests
> that were granted to ant and junit.
>
> >> Yes, I agree completely. I would control the permission assignment
> >> more tightly:
> >> 1. only allow the following necessary permissions in default:
> >> addPermission(new SecurityPermission("getPolicy"));
> >> addPermission(new SecurityPermission("setPolicy"));
> >> addPermission(new RuntimePermission("setSecurityManager"));
>
> These are safe for the current code base. So may be add a note to remind
> for any future changes.
>
> >> 2. other permissions in current default set are commonly used for the
> >> tests, so I would add more Policy classes like existing
> >> FilePolicy, etc.
> >> //ClassLoaderPolicy, an individual test may only require some of
> >> them, but I would put them in only one class if you agree
> >> addPermission(new RuntimePermission("getClassLoader"));
> >> addPermission(new RuntimePermission("createClassLoader"));
> >> addPermission(new RuntimePermission("modifyThread"));
> >>
> >> How about you think?
>
> My understanding is that you intend to grand specific permissions
> limited to the test that will extend these policies, e.g. FilePolicy. I
> think this is ok and an improvement.
>
> >
> > It would definitely improve things - but then all the classes
> > in the test that runs with this new policy class will inherit
> > from these permissions as well. This may or may not be an issue.
> > (I confess I haven't looked at all the webrev ;-()
>
I will reduce the scope of the extra permissions as possible, and make sure the
safety, e.g. surround the user code(i.e. the test
code) which requires a permission explicitly with corresponding permission
assignment
> Yeah, it would be good to make sure a policy is safe with the code. If
> both the test code and the code may need a permission, we may have a
> conflict that we need to solve. It may be good to start with the basic
> permission, and add only if required by the test code, with a note
> preferably noting what exactly is needed. "DefaultFeaturesTest" in this
> regard, doesn't seem to require FilePolicy, but
> CatalogReferCircularityTest requires permission to where the source
> files are located, in this case, it would be good to make it specific.
> For example, instead of being called "FilePolicy", it may be clearer to
> add a parameter so that it's known where the File permission is given
> (e.g. the source dir in this case).
Currently FilePolicy(maybe it's better to rename to TestFilePolicy) has full
access permission to user.dir and read permission to
test.src, I think they belong to user permission, jaxp lib won't doPrivileged
on this.
Btw, it's unable to add parameter in @Listeners({ xxx.class }) unless using
more tricky and complex means.
I believe I will meet problem to identify if a doPrivileged is missing when I
strip the extra permissions and then solve the test
failures case by case, so I would ask you when should we add doPrivileged in
jaxp/jdk library code?
1. Should doPrivileged only for where the permission can't be granted to the
user code, e.g. read some secret system property
2. Or should doPrivileged for where the permission needn't be granted to the
user code, e.g. read some internal property
Frank
>
> Best,
> Joe
>
> >
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 7/22/2016 5:53 AM, Daniel Fuchs wrote:
On 22/07/16 10:15, Frank Yuan wrote:
Hi Daniel
Thank you very much for your review and the comments!
-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
unit tests
Hi Frank,
I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:
107 addPermission(new SecurityPermission("getPolicy"));
108 addPermission(new SecurityPermission("setPolicy"));
109 addPermission(new RuntimePermission("getClassLoader"));
110 addPermission(new
RuntimePermission("createClassLoader"));
111 addPermission(new
RuntimePermission("setSecurityManager"));
112 addPermission(new
RuntimePermission("createSecurityManager"));
113 addPermission(new RuntimePermission("modifyThread"));
114 addPermission(new PropertyPermission("*", "read,
write"));
115 addPermission(new
ReflectPermission("suppressAccessChecks"));
116 addPermission(new RuntimePermission("setIO"));
117 addPermission(new
RuntimePermission("setContextClassLoader"));
118 addPermission(new
RuntimePermission("accessDeclaredMembers"));
These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.
Very true. Some of these permissions came from our standalone JAXP tests
that were granted to ant and junit.
Yes, I agree completely. I would control the permission assignment
more tightly:
1. only allow the following necessary permissions in default:
addPermission(new SecurityPermission("getPolicy"));
addPermission(new SecurityPermission("setPolicy"));
addPermission(new RuntimePermission("setSecurityManager"));
These are safe for the current code base. So may be add a note to remind
for any future changes.
2. other permissions in current default set are commonly used for the
tests, so I would add more Policy classes like existing
FilePolicy, etc.
//ClassLoaderPolicy, an individual test may only require some of
them, but I would put them in only one class if you agree
addPermission(new RuntimePermission("getClassLoader"));
addPermission(new RuntimePermission("createClassLoader"));
addPermission(new RuntimePermission("setContextClassLoader"));
// PropertyPolicy, there are various properties needed by the
tests, I would not classify them further...
addPermission(new PropertyPermission("*", "read, write"));
//Reflection permissions, jtreg may require them in deed, so I may
add them in default set
addPermission(new ReflectPermission("suppressAccessChecks"));
addPermission(new RuntimePermission("accessDeclaredMembers"));
//other permissions are used in less tests, I may use
tryRunWithTmpPermission instead of Policy class
addPermission(new RuntimePermission("setIO"));
addPermission(new RuntimePermission("createSecurityManager"));
addPermission(new RuntimePermission("modifyThread"));
How about you think?
My understanding is that you intend to grand specific permissions
limited to the test that will extend these policies, e.g. FilePolicy. I
think this is ok and an improvement.
It would definitely improve things - but then all the classes
in the test that runs with this new policy class will inherit
from these permissions as well. This may or may not be an issue.
(I confess I haven't looked at all the webrev ;-()
Yeah, it would be good to make sure a policy is safe with the code. If
both the test code and the code may need a permission, we may have a
conflict that we need to solve. It may be good to start with the basic
permission, and add only if required by the test code, with a note
preferably noting what exactly is needed. "DefaultFeaturesTest" in this
regard, doesn't seem to require FilePolicy, but
CatalogReferCircularityTest requires permission to where the source
files are located, in this case, it would be good to make it specific.
For example, instead of being called "FilePolicy", it may be clearer to
add a parameter so that it's known where the File permission is given
(e.g. the source dir in this case).
Best,
Joe
I had a similar issue when writing logging test, were I wanted
to temporarily disable permission checking in the middle of a test
to perform an infrastructure configuration.
So what I did is use an ThreadLocal to temporarily
disable perm
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 22/07/16 10:15, Frank Yuan wrote:
Hi Daniel
Thank you very much for your review and the comments!
-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Frank,
I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:
107 addPermission(new SecurityPermission("getPolicy"));
108 addPermission(new SecurityPermission("setPolicy"));
109 addPermission(new RuntimePermission("getClassLoader"));
110 addPermission(new RuntimePermission("createClassLoader"));
111 addPermission(new RuntimePermission("setSecurityManager"));
112 addPermission(new RuntimePermission("createSecurityManager"));
113 addPermission(new RuntimePermission("modifyThread"));
114 addPermission(new PropertyPermission("*", "read, write"));
115 addPermission(new ReflectPermission("suppressAccessChecks"));
116 addPermission(new RuntimePermission("setIO"));
117 addPermission(new RuntimePermission("setContextClassLoader"));
118 addPermission(new RuntimePermission("accessDeclaredMembers"));
These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.
Yes, I agree completely. I would control the permission assignment more tightly:
1. only allow the following necessary permissions in default:
addPermission(new SecurityPermission("getPolicy"));
addPermission(new SecurityPermission("setPolicy"));
addPermission(new RuntimePermission("setSecurityManager"));
2. other permissions in current default set are commonly used for the tests, so
I would add more Policy classes like existing
FilePolicy, etc.
//ClassLoaderPolicy, an individual test may only require some of them, but I
would put them in only one class if you agree
addPermission(new RuntimePermission("getClassLoader"));
addPermission(new RuntimePermission("createClassLoader"));
addPermission(new RuntimePermission("setContextClassLoader"));
// PropertyPolicy, there are various properties needed by the tests, I would
not classify them further...
addPermission(new PropertyPermission("*", "read, write"));
//Reflection permissions, jtreg may require them in deed, so I may add them
in default set
addPermission(new ReflectPermission("suppressAccessChecks"));
addPermission(new RuntimePermission("accessDeclaredMembers"));
//other permissions are used in less tests, I may use tryRunWithTmpPermission
instead of Policy class
addPermission(new RuntimePermission("setIO"));
addPermission(new RuntimePermission("createSecurityManager"));
addPermission(new RuntimePermission("modifyThread"));
How about you think?
It would definitely improve things - but then all the classes
in the test that runs with this new policy class will inherit
from these permissions as well. This may or may not be an issue.
(I confess I haven't looked at all the webrev ;-()
I had a similar issue when writing logging test, were I wanted
to temporarily disable permission checking in the middle of a test
to perform an infrastructure configuration.
So what I did is use an ThreadLocal to temporarily
disable permission checking - which allows me in my tests to do things
like:
boolean before = allowAll.get().get();
allowAll.get().set(true);
try {
do something that requires a permission
} finally {
allowAll.get().set(before);
}
JAXPTestUtilities.tryRunWithTmpPermission is similar with this, see the example:
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/test/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6555001.java.sdiff.htm
l
Yes that part looks fine.
cheers,
-- daniel
My implementation of Policy::implies also checks for
if (allowAll.get().get()) return true;
This allows me to control more tightly the set of permissions
I want my test to run under, while still being able to
perform any action I want to set up things without having
to give the same permission to all.
Hope this helps,
-- daniel
On 22/07/16 07:59, Frank Yuan wrote:
According to Amy's suggestion, re-generate a webrev
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix some
issues,
please check.
Thanks
Frank
-Original Message-
From: Amy Lu [mailto:amy...@oracle.com]
Sent: Monday, July 18, 2016 5:42 PM
To: Frank Yuan; 'core-libs-dev'
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 7/18/16 5:32 PM, Frank Yuan wrote:
Btw, I moved internaltest into unittest because it's unnecessary to separate
them.
Maybe you'd like to regenerate the webrev with hg move for those files?
Thanks,
Amy
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel
Thank you very much for your review and the comments!
> -Original Message-
> From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> tests
>
> Hi Frank,
>
> I see that in order to be able to run the tests, you were forced
> to add a few permissions that the test/test infrastructure need
> to setup things:
>
> 107 addPermission(new SecurityPermission("getPolicy"));
> 108 addPermission(new SecurityPermission("setPolicy"));
> 109 addPermission(new RuntimePermission("getClassLoader"));
> 110 addPermission(new RuntimePermission("createClassLoader"));
> 111 addPermission(new RuntimePermission("setSecurityManager"));
> 112 addPermission(new RuntimePermission("createSecurityManager"));
> 113 addPermission(new RuntimePermission("modifyThread"));
> 114 addPermission(new PropertyPermission("*", "read, write"));
> 115 addPermission(new ReflectPermission("suppressAccessChecks"));
> 116 addPermission(new RuntimePermission("setIO"));
> 117 addPermission(new RuntimePermission("setContextClassLoader"));
> 118 addPermission(new RuntimePermission("accessDeclaredMembers"));
>
> These are quite powerful permissions, and adding them by default
> also means that you might miss a bug - if e.g. a doPrivileged is
> missing somewhere in the JAXP code when jaxp tries to e.g. get/create
> a classloader, or read a system property, you might not see
> it.
Yes, I agree completely. I would control the permission assignment more tightly:
1. only allow the following necessary permissions in default:
addPermission(new SecurityPermission("getPolicy"));
addPermission(new SecurityPermission("setPolicy"));
addPermission(new RuntimePermission("setSecurityManager"));
2. other permissions in current default set are commonly used for the tests, so
I would add more Policy classes like existing
FilePolicy, etc.
//ClassLoaderPolicy, an individual test may only require some of them, but I
would put them in only one class if you agree
addPermission(new RuntimePermission("getClassLoader"));
addPermission(new RuntimePermission("createClassLoader"));
addPermission(new RuntimePermission("setContextClassLoader"));
// PropertyPolicy, there are various properties needed by the tests, I would
not classify them further...
addPermission(new PropertyPermission("*", "read, write"));
//Reflection permissions, jtreg may require them in deed, so I may add them
in default set
addPermission(new ReflectPermission("suppressAccessChecks"));
addPermission(new RuntimePermission("accessDeclaredMembers"));
//other permissions are used in less tests, I may use tryRunWithTmpPermission
instead of Policy class
addPermission(new RuntimePermission("setIO"));
addPermission(new RuntimePermission("createSecurityManager"));
addPermission(new RuntimePermission("modifyThread"));
How about you think?
>
> I had a similar issue when writing logging test, were I wanted
> to temporarily disable permission checking in the middle of a test
> to perform an infrastructure configuration.
>
> So what I did is use an ThreadLocal to temporarily
> disable permission checking - which allows me in my tests to do things
> like:
>
> boolean before = allowAll.get().get();
> allowAll.get().set(true);
> try {
> do something that requires a permission
> } finally {
> allowAll.get().set(before);
> }
>
JAXPTestUtilities.tryRunWithTmpPermission is similar with this, see the example:
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/test/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6555001.java.sdiff.htm
l
> My implementation of Policy::implies also checks for
>
> if (allowAll.get().get()) return true;
>
> This allows me to control more tightly the set of permissions
> I want my test to run under, while still being able to
> perform any action I want to set up things without having
> to give the same permission to all.
>
> Hope this helps,
>
> -- daniel
>
>
>
> On 22/07/16 07:59, Frank Yuan wrote:
> > According to Amy's suggestion, re-generate a webrev
> > http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix some
issues,
> > please check.
> >
> > Thanks
> > Frank
> >
> >> -Original Message-
> >> From: Amy Lu [mailto:amy...@oracle.com]
> >> Sent: Monday, July 18, 2016 5:42 PM
> >> To: Frank Yuan; 'core-libs-dev'
> >> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> >> tests
> >>
> >> On 7/18/16 5:32 PM, Frank Yuan wrote:
> >>> Btw, I moved internaltest into unittest because it's unnecessary to
> >>> separate them.
> >>
> >> Maybe you'd like to regenerate the webrev with hg move for those files?
> >>
> >> Thanks,
> >> Amy
> >
> >
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Frank,
I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:
107 addPermission(new SecurityPermission("getPolicy"));
108 addPermission(new SecurityPermission("setPolicy"));
109 addPermission(new RuntimePermission("getClassLoader"));
110 addPermission(new RuntimePermission("createClassLoader"));
111 addPermission(new RuntimePermission("setSecurityManager"));
112 addPermission(new RuntimePermission("createSecurityManager"));
113 addPermission(new RuntimePermission("modifyThread"));
114 addPermission(new PropertyPermission("*", "read, write"));
115 addPermission(new ReflectPermission("suppressAccessChecks"));
116 addPermission(new RuntimePermission("setIO"));
117 addPermission(new RuntimePermission("setContextClassLoader"));
118 addPermission(new RuntimePermission("accessDeclaredMembers"));
These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.
I had a similar issue when writing logging test, were I wanted
to temporarily disable permission checking in the middle of a test
to perform an infrastructure configuration.
So what I did is use an ThreadLocal to temporarily
disable permission checking - which allows me in my tests to do things
like:
boolean before = allowAll.get().get();
allowAll.get().set(true);
try {
do something that requires a permission
} finally {
allowAll.get().set(before);
}
My implementation of Policy::implies also checks for
if (allowAll.get().get()) return true;
This allows me to control more tightly the set of permissions
I want my test to run under, while still being able to
perform any action I want to set up things without having
to give the same permission to all.
Hope this helps,
-- daniel
On 22/07/16 07:59, Frank Yuan wrote:
According to Amy's suggestion, re-generate a webrev
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix some issues,
please check.
Thanks
Frank
-Original Message-----
From: Amy Lu [mailto:amy...@oracle.com]
Sent: Monday, July 18, 2016 5:42 PM
To: Frank Yuan; 'core-libs-dev'
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 7/18/16 5:32 PM, Frank Yuan wrote:
Btw, I moved internaltest into unittest because it's unnecessary to separate
them.
Maybe you'd like to regenerate the webrev with hg move for those files?
Thanks,
Amy
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
According to Amy's suggestion, re-generate a webrev http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix some issues, please check. Thanks Frank > -Original Message- > From: Amy Lu [mailto:amy...@oracle.com] > Sent: Monday, July 18, 2016 5:42 PM > To: Frank Yuan; 'core-libs-dev' > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > On 7/18/16 5:32 PM, Frank Yuan wrote: > > Btw, I moved internaltest into unittest because it's unnecessary to > > separate them. > > Maybe you'd like to regenerate the webrev with hg move for those files? > > Thanks, > Amy
Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 7/18/16 5:32 PM, Frank Yuan wrote: Btw, I moved internaltest into unittest because it's unnecessary to separate them. Maybe you'd like to regenerate the webrev with hg move for those files? Thanks, Amy
RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi
Would you like to review http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/?
Bug: https://bugs.openjdk.java.net/browse/JDK-8067170
In this change, I enabled security manager for JAXP unit tests with improving
the implementation approach and fixing some defects.
Now jaxp tests use TestNG annotation to enable security manager and apply
policies combination, like the following example(this full
example is at:
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/Bug8003147Test.java.html):
@Listeners({ jaxp.library.FilePolicy.class,
jaxp.library.InternalAPIPolicy.class })
public class Bug8003147Test {
There are also 2 additional patterns for some special cases:
1. JAXPTestUtilities.runWithTmpPermission(Runnable, Permission.) is used
for some cases which require their own special
permissions, e.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/transform/CR6551600Test.java.sdiff.html.
2. JAXPTestUtilities.tryRunWithPolicyManager(Runnable, Permission.) is
used for cases where some test methods need to be run
with security manager and some others need to be run without security manager
because they have different behaviors when having sm
or not. E.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/FactoryFindTest.java.sdiff.html.
Such cases
must run in single thread, all other cases don't require it, are thread safe.
Btw, I moved internaltest into unittest because it's unnecessary to separate
them.
Thanks,
Frank
