This is an automated email from the ASF dual-hosted git repository. jlmonteiro pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tomee.git
commit a6ece224c5c779d32a3466600d84b0598938dc96 Author: Jean-Louis Monteiro <[email protected]> AuthorDate: Thu Apr 22 21:46:27 2021 +0200 Improve the way we evalate EL --- .../tomee/security/TomEEELInvocationHandler.java | 87 ++++++++++++++++------ .../security/TomEEELInvocationHandlerTest.java | 61 ++++++++++++--- 2 files changed, 114 insertions(+), 34 deletions(-) diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java index 3393788..c02ecb9 100644 --- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java +++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java @@ -24,9 +24,13 @@ import java.lang.reflect.InvocationHandler; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Proxy; +import java.util.regex.Matcher; +import java.util.regex.Pattern; public class TomEEELInvocationHandler implements InvocationHandler { + private static final Pattern EL_EXPRESSION_PATTERN = Pattern.compile("^[#$]\\{(.+)}$"); + private final Annotation annotation; private final ELProcessor processor; @@ -41,37 +45,75 @@ public class TomEEELInvocationHandler implements InvocationHandler { // todo optimize and cache methods // avoid stack overflow because of infinite loop (See bellow) - // if method is already the expression one, just invoke it + // if method is already the expression one with a String return type, then invoke it and return the result + // so we can evaluate the EL and return the evaluated value if (method.getName().endsWith("Expression") && method.getReturnType().equals(String.class)) { return method.invoke(annotation, args); } - try { - final Method expressionMethod = annotation.getClass().getDeclaredMethod(method.getName() + "Expression", method.getParameterTypes()); - final String expression = (String) expressionMethod.invoke(proxy, args); + // If return value is not a String or an array of string, there is another method with "Expression" at the end and a return type String + if (!method.getReturnType().equals(String.class)) { + try { + // try to find the equivalent expression method + final Method expressionMethod = + annotation.getClass() + .getDeclaredMethod(method.getName() + "Expression", method.getParameterTypes()); + + // great, let's get the EL value + // we could check the return value to make sure it's a string, + // but let's assume people writing annotation apis aren't stupid + final String expression = (String) expressionMethod.invoke(proxy, args); + + // if there is an expression, it takes precedence over the static one + if (!expression.trim().isEmpty()) { + // make sure to pass in the return type of the initial method, otherwith it would be String all the time + return eval(processor, sanitizeExpression(expression), method.getReturnType()); + + } else { + return method.invoke(annotation, args); + } + + } catch (final NoSuchMethodException e) { + // from spec it is required for a new String to have an equivalent + // if not, keep going with the initial method invocation + return method.invoke(annotation, args); - // if there is an expression, it takes precedence over the static one - if (!expression.isEmpty()) { - return eval(expression, method.getReturnType()); // use the return type of the static method instead + } catch (final InvocationTargetException e) { // unwrap the invocation target exception so we get the actual error + throw e.getTargetException(); + } + } - } else { - return method.invoke(annotation, args); + // if the return type is a String, we may always get an expression to evaluate. + // check if it's something we can evaluate + final String value = (String) method.invoke(annotation, args); + if (value != null && value.length() > 3) { + final String sanitizedExpression = sanitizeExpression(value); + if (!value.equals(sanitizedExpression)) { + return eval(processor, sanitizedExpression, method.getReturnType()); } + } - } catch (final NoSuchMethodException e) { // expression equivalent not found for the given method - return method.invoke(annotation, args); + return value; + } - } catch (final InvocationTargetException e) { // unwrap the invocation target exception so we get the actual error - throw e.getTargetException(); - } + // following should be abstracted into a wrapper of the ELProcessor utility class + public static boolean isExpression(final String rawExpression) { + final Matcher matcher = EL_EXPRESSION_PATTERN.matcher(rawExpression); + return matcher.matches(); } - private Object eval(final String expression, final Class<?> expectedType) { - // expression maybe #{expression} instead of ${expression} - // the ELProcessor anyways wraps it with ${} - final String sanitizedExpression = expression.replaceAll("^[#$]\\{(.+)}$", "$1"); + public static String sanitizeExpression(final String rawExpression) { + final Matcher matcher = EL_EXPRESSION_PATTERN.matcher(rawExpression); + + if (!matcher.matches()) { + return rawExpression; + } + + return matcher.replaceAll("$1"); + } + public static Object eval(final ELProcessor processor, final String sanitizedExpression, final Class<?> expectedType) { // ELProcessor does not do a good job with enums, so try to be a bit better (not sure) // otherwise, let the EL processor do its best to convert into the expected value if (!isEnumOrArrayOfEnums(expectedType)) { @@ -99,13 +141,12 @@ public class TomEEELInvocationHandler implements InvocationHandler { } // else not sure what else we can do but let the Object go - } return value; } - private boolean isEnumOrArrayOfEnums(final Class type) { + private static boolean isEnumOrArrayOfEnums(final Class type) { if (type.isEnum()) { return true; } @@ -118,7 +159,7 @@ public class TomEEELInvocationHandler implements InvocationHandler { return false; } - private <T /*extends Enum<T>*/> T of(final Class<T> type, final Object name) { + private static <T /*extends Enum<T>*/> T of(final Class<T> type, final Object name) { try { return (T) type.getDeclaredMethod("valueOf", String.class).invoke(null, name); @@ -132,9 +173,7 @@ public class TomEEELInvocationHandler implements InvocationHandler { public static <T extends Annotation> T of(final Class<T> annotationClass, final T annotation, final BeanManager beanManager) { final ELProcessor elProcessor = new ELProcessor(); elProcessor.getELManager().addELResolver(beanManager.getELResolver()); - return (T) Proxy.newProxyInstance(annotation.getClass().getClassLoader(), - new Class[]{annotationClass}, - new TomEEELInvocationHandler(annotation, elProcessor)); + return of(annotationClass, annotation, elProcessor); } public static <T extends Annotation> T of(final Class<T> annotationClass, final T annotation, final ELProcessor elProcessor) { diff --git a/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java b/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java index 80db047..4e23efe 100644 --- a/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java +++ b/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java @@ -16,32 +16,44 @@ */ package org.apache.tomee.security; +import org.apache.tomee.security.identitystore.TomEEDatabaseIdentityStore; import org.junit.Assert; import org.junit.Test; import javax.el.ELProcessor; -import javax.enterprise.inject.Alternative; +import javax.el.ELResolver; import javax.enterprise.inject.Vetoed; import javax.enterprise.inject.spi.BeanManager; import javax.enterprise.inject.spi.CDI; +import javax.inject.Named; import javax.security.enterprise.identitystore.DatabaseIdentityStoreDefinition; import javax.security.enterprise.identitystore.IdentityStore; import javax.security.enterprise.identitystore.PasswordHash; -import java.lang.reflect.InvocationHandler; import java.util.Map; +import java.util.Set; +import java.util.stream.Stream; + +import static java.util.Arrays.stream; +import static java.util.stream.Collectors.toMap; +import static java.util.stream.Collectors.toSet; +import static org.apache.tomee.security.identitystore.TomEEDatabaseIdentityStore.eval; +import static org.apache.tomee.security.identitystore.TomEEDatabaseIdentityStore.toStream; public class TomEEELInvocationHandlerTest extends AbstractTomEESecurityTest { @Test public void canCreateInvocationHandler() { - final DatabaseIdentityStoreDefinition annotation = - Color.class.getAnnotation(DatabaseIdentityStoreDefinition.class); + final DatabaseIdentityStoreDefinition annotation = Color.class.getAnnotation(DatabaseIdentityStoreDefinition.class); final ELProcessor elProcessor = new ELProcessor(); - elProcessor.getELManager().addELResolver(bm().getELResolver()); + final ELResolver elResolver = bm().getELResolver(); + elProcessor.getELManager().addELResolver(elResolver); + + // small trick because of the @Vetoed bellow - OWB won't pick it up + // so we will register one ourselves into the processor so it is resolved + elProcessor.defineBean("color", new Color()); - final InvocationHandler handler = new TomEEELInvocationHandler(annotation, elProcessor); - final DatabaseIdentityStoreDefinition proxiedAnnotation = TomEEELInvocationHandler.of(DatabaseIdentityStoreDefinition.class, annotation, bm()); + final DatabaseIdentityStoreDefinition proxiedAnnotation = TomEEELInvocationHandler.of(DatabaseIdentityStoreDefinition.class, annotation, elProcessor); Assert.assertEquals("select password from caller where name = ?", proxiedAnnotation.callerQuery()); Assert.assertEquals(90, proxiedAnnotation.priority()); @@ -49,21 +61,50 @@ public class TomEEELInvocationHandlerTest extends AbstractTomEESecurityTest { Assert.assertEquals("90", proxiedAnnotation.priorityExpression()); Assert.assertArrayEquals(new IdentityStore.ValidationType[] {IdentityStore.ValidationType.VALIDATE}, proxiedAnnotation.useFor()); + Assert.assertEquals("select group_name from caller_groups where caller_name = ?", proxiedAnnotation.groupsQuery()); + final String[] hashAlgorithmParameters = proxiedAnnotation.hashAlgorithmParameters(); + Assert.assertArrayEquals(new String[]{ + "Pbkdf2PasswordHash.Iterations=3072", + "${color.dyna}" + }, hashAlgorithmParameters); + + final Set<String> evaluatedHashParameters = stream(hashAlgorithmParameters) + .flatMap(s -> toStream(eval(elProcessor, s, Object.class))).collect(toSet()); + + System.out.println(evaluatedHashParameters); + + final Map<String, String> parametersMap = evaluatedHashParameters.stream() + .collect(toMap(s -> (String) s.substring(0, s.indexOf('=')), + s -> (String) eval(elProcessor, s.substring(s.indexOf('=') + 1), String.class))); + + System.out.println(parametersMap); } private BeanManager bm() { return CDI.current().getBeanManager(); } - @Vetoed // so we don't break the other tests with this + @Named // see expression language @DatabaseIdentityStoreDefinition(dataSourceLookup = "jdbc/securityAPIDB", callerQuery = "select password from caller where name = ?", - groupsQuery = "select group_name from caller_groups where caller_name = ?", + groupsQuery = "${color.groupsQuery}", hashAlgorithm = CleartextPasswordHash.class, priority = 30, priorityExpression = "90", - useForExpression = "#{'VALIDATE'}") + useForExpression = "#{'VALIDATE'}", + hashAlgorithmParameters = { + "Pbkdf2PasswordHash.Iterations=3072", + "${color.dyna}" + }) public static class Color { + + public String getGroupsQuery() { + return "select group_name from caller_groups where caller_name = ?"; + } + + public String[] getDyna() { + return new String[]{"Pbkdf2PasswordHash.Algorithm=PBKDF2WithHmacSHA512", "Pbkdf2PasswordHash.SaltSizeBytes=64"}; + } } public static class CleartextPasswordHash implements PasswordHash {
