Merge pull request #549 from jglick/ProxyWhitelist

Overhauled `ProxyWhitelist`

Author: jglick

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/AclAwareWhitelist.java

@@ -51,9 +51,6 @@ public class AclAwareWhitelist extends Whitelist {
      */
     public AclAwareWhitelist(Whitelist unrestricted, Whitelist restricted) {
         this.unrestricted = unrestricted;
-        if (this.unrestricted instanceof EnumeratingWhitelist) {
-            ((EnumeratingWhitelist) this.unrestricted).precache();
-        }
         this.restricted = restricted;
     }
 

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/EnumeratingWhitelist.java

@@ -24,20 +24,23 @@
 
 package org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
+import edu.umd.cs.findbugs.annotations.CheckForNull;
+import edu.umd.cs.findbugs.annotations.NonNull;
+import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Array;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.Arrays;
 import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-
+import java.util.function.BiPredicate;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
 
-import edu.umd.cs.findbugs.annotations.CheckForNull;
-import edu.umd.cs.findbugs.annotations.NonNull;
-
 /**
  * A whitelist based on listing signatures and searching them. Lists of signatures should not change
  * from invocation to invocation.
@@ -57,127 +60,55 @@ public abstract class EnumeratingWhitelist extends Whitelist {
 
     protected abstract List<FieldSignature> staticFieldSignatures();
 
-    ConcurrentHashMap<String, Boolean> permittedCache = new ConcurrentHashMap<>();  // Not private to facilitate testing
-
-    @SafeVarargs
-    private final void cacheSignatureList(List<Signature> ...sigs) {
-        for (List<Signature> list : sigs) {
-            for (Signature s : list) {
-                if (!s.isWildcard()) { // Cache entries for wildcard signatures will never be accessed and just waste space
-                    permittedCache.put(s.toString(), Boolean.TRUE);
+    private final Predicate<Method> methodCache = cache(this::methodSignatures, MethodSignature::matches);
+    private final Predicate<Constructor<?>> constructorCache = cache(this::newSignatures, NewSignature::matches);
+    private final Predicate<Method> staticMethodCache = cache(this::staticMethodSignatures, MethodSignature::matches);
+    private final Predicate<Field> fieldCache = cache(this::fieldSignatures, FieldSignature::matches);
+    private final Predicate<Field> staticFieldCache = cache(this::staticFieldSignatures, FieldSignature::matches);
+
+    private static <M extends AccessibleObject, S extends Signature> Predicate<M> cache(Supplier<List<S>> signatures, BiPredicate<S, M> matches) {
+        // Unfortunately xxxSignatures() will return empty if called from superclass constructor,
+        // since subclass constructors initialize lists, thus the need for Supplier.
+        // Would be cleaner for EnumeratingWhitelist to take all signatures in its constructor,
+        // and for StaticWhitelist to just be a utility with static constructor methods rather than a subclass.
+        LoadingCache<M, Boolean> cache = Caffeine.newBuilder().weakKeys().build((M m) -> {
+            for (S s : signatures.get()) {
+                if (matches.test(s, m)) {
+                    return true;
                 }
             }
-        }
-    }
-
-    /** Prepopulates the "permitted" cache, resetting if populated already.  Should be called when method signatures change or after initialization. */
-    final void precache() {
-        if (!permittedCache.isEmpty()) {
-            this.permittedCache.clear();  // No sense calling clearCache
-        }
-        cacheSignatureList((List)methodSignatures(), (List)(newSignatures()),
-                           (List)(staticMethodSignatures()), (List)(fieldSignatures()),
-                           (List)(staticFieldSignatures()));
-    }
-
-    /** Frees up nearly all memory used for the cache.  MUST BE CALLED if you change the result of the xxSignatures() methods. */
-    final void clearCache() {
-        this.permittedCache.clear();
-        this.permittedCache = new ConcurrentHashMap<>();
+            return false;
+        });
+        return m -> {
+            if (signatures.get().isEmpty()) {
+                return false; // shortcut
+            }
+            return cache.get(m);
+        };
     }
 
     @Override public final boolean permitsMethod(@NonNull Method method, @NonNull Object receiver, @NonNull Object[] args) {
-        String key = canonicalMethodSig(method);
-        Boolean b = permittedCache.get(key);
-        if (b != null) {
-            return b;
-        }
-
-        boolean output = false;
-        for (MethodSignature s : methodSignatures()) {
-            if (s.matches(method)) {
-                output = true;
-                break;
-            }
-        }
-        permittedCache.put(key, output);
-        return output;
+        return methodCache.test(method);
     }
 
     @Override public final boolean permitsConstructor(@NonNull Constructor<?> constructor, @NonNull Object[] args) {
-        String key = canonicalConstructorSig(constructor);
-        Boolean b = permittedCache.get(key);
-        if (b != null) {
-            return b;
-        }
-
-        boolean output = false;
-        for (NewSignature s : newSignatures()) {
-            if (s.matches(constructor)) {
-                output = true;
-                break;
-            }
-        }
-        permittedCache.put(key, output);
-        return output;
+        return constructorCache.test(constructor);
     }
 
     @Override public final boolean permitsStaticMethod(@NonNull Method method, @NonNull Object[] args) {
-        String key = canonicalStaticMethodSig(method);
-        Boolean b = permittedCache.get(key);
-        if (b != null) {
-            return b;
-        }
-
-        boolean output = false;
-        for (MethodSignature s : staticMethodSignatures()) {
-            if (s.matches(method)) {
-                output = true;
-                break;
-            }
-        }
-        permittedCache.put(key, output);
-        return output;
+        return staticMethodCache.test(method);
     }
 
     @Override public final boolean permitsFieldGet(@NonNull Field field, @NonNull Object receiver) {
-        String key = canonicalFieldSig(field);
-        Boolean b = permittedCache.get(key);
-        if (b != null) {
-            return b;
-        }
-
-        boolean output = false;
-        for (FieldSignature s : fieldSignatures()) {
-            if (s.matches(field)) {
-                output = true;
-                break;
-            }
-        }
-        permittedCache.put(key, output);
-        return output;
+        return fieldCache.test(field);
     }
 
     @Override public final boolean permitsFieldSet(@NonNull Field field, @NonNull Object receiver, Object value) {
         return permitsFieldGet(field, receiver);
     }
 
     @Override public final boolean permitsStaticFieldGet(@NonNull Field field) {
-        String key = canonicalStaticFieldSig(field);
-        Boolean b = permittedCache.get(key);
-        if (b != null) {
-            return b;
-        }
-
-        boolean output = false;
-        for (FieldSignature s : staticFieldSignatures()) {
-            if (s.matches(field)) {
-                output = true;
-                break;
-            }
-        }
-        permittedCache.put(key, output);
-        return output;
+        return staticFieldCache.test(field);
     }
 
     @Override public final boolean permitsStaticFieldSet(@NonNull Field field, Object value) {
@@ -276,6 +207,8 @@ static String[] argumentTypes(Class<?>[] argumentTypes) {
         return s;
     }
 
+    // TODO move all these to StaticWhitelist
+
     /** Canonical name for a field access. */
     static String canonicalFieldString(@NonNull Field field) {
         return getName(field.getDeclaringClass()) + ' ' + field.getName();
@@ -385,7 +318,7 @@ public NewSignature(String type, String[] argumentTypes) {
         public NewSignature(Class<?> type, Class<?>... argumentTypes) {
             this(getName(type), argumentTypes(argumentTypes));
         }
-        boolean matches(Constructor c) {
+        boolean matches(Constructor<?> c) {
             return getName(c.getDeclaringClass()).equals(type) && Arrays.equals(argumentTypes(c.getParameterTypes()), argumentTypes);
         }
         @Override String signaturePart() {

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/GenericWhitelist.java

@@ -24,16 +24,13 @@
 
 package org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists;
 
-import hudson.Extension;
 import java.io.IOException;
-import org.kohsuke.accmod.Restricted;
-import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
- * Includes entries useful for general kinds of scripts.
+ * @deprecated replaced by {@link StaticWhitelist#stockWhitelists}, now used only in tests
  */
-@Restricted(NoExternalUse.class)
-@Extension public final class GenericWhitelist extends ProxyWhitelist {
+@Deprecated
+public final class GenericWhitelist extends ProxyWhitelist {
 
     public GenericWhitelist() throws IOException {
         super(StaticWhitelist.from(GenericWhitelist.class.getResource("generic-whitelist")));