RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Frank Yuan]

Hi Daniel

Thank you very much for your review and the comments!

> -----Original Message-----
> From: Daniel Fuchs [mailto:daniel.fuchs at 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.lu at 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
> >
> >