other than that previously discussed,
in
test/javax/xml/jaxp/functional/javax/xml/parsers/ptests/DocumentBuilderFactoryTest.java
the comments related to the removed code can be removed as well
test/javax/xml/jaxp/functional/javax/xml/parsers/ptests/SAXParserTest.java
@Test(expectedExceptions = SAXException.class, dataProvider =
"parser-provider", enabled = false) //disabled for 8162848
-- just a reminder, as we discussed, the test needs to be enabled,
and 8162848 closed (also one in
test/javax/xml/jaxp/unittest/common/Sources.java)
what are runWithSecurityManager and runWithoutSecurityManager in some of
the tests? I thought the whole test suite will be run with and without
security manager, isn't that the plan?
Once again, great work and nice to clean up the old security-testing code.
Best,
Joe
On 8/2/16, 2:56 PM, Joe Wang wrote:
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
