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<AtomicBoolean> 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 > > > >