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

[Frank Yuan]

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 +0000
> > +++ 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.
>   * <p>
>   * 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<Integer> 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 JAXPPolicyManager, and delegate the null checking of the global
>     policyManager instance inside those methods.
> 

If policyManager is null, Stream.of(ps) has not any iteration, so it's safe.

> > +            tmpPermissionIndexes.forEach(index -> policyManager.removeTmpPermission(index));
> > +        }
> > +    }
> 
> 
> > +
> > +    /**
> > +     * Run the supplier with all permissions. This won't impact global policy.
> > +     *
> > +     * @param s
> > +     *            Supplier to run
> > +     */
> > +    public static <T> T runWithAllPerm(Supplier<T> s) {
> > +        Optional<JAXPPolicyManager> policyManager = Optional.ofNullable(JAXPPolicyManager
> > +                .getJAXPPolicyManager(false));
> > +        policyManager.ifPresent(manager -> manager.setAllowAll(true));
> > +        try {
> > +            return s.get();
> > +        } finally {
> > +            policyManager.ifPresent(manager -> manager.setAllowAll(false));
> 
> This should restore allowAll to its previous value rather than 'false'.

Same as a previous comment.

> Maybe you should make setAllowAll/switchAllowAll static in
> policyManager. The allowAll ThreadLocal can be static as well, so you
> don't really to have an instance of JAXPPolicyManager or TestPolicy to
> switch it on or off.
> 

Because of ThreadLocal, static is equivalent to instance, I used instance allowAll and setAllowAll because I wanted to emphasize
their logic relationship. I would follow your suggestion if you adhere :)


Thanks

Frank