From 877c58387e1a3d9d3ea976646d69979f1f35027e Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 24 Oct 2024 15:13:10 +0200 Subject: [PATCH] Polishing. Refine Javadoc. Move LdapEncoder lookup into LdapParameters. Eagerly instantiate LdapEncoder. Refine method naming. Add NameEncoder and LikeEncoder for simplified usage. See #509 Original pull request: #518 --- .../data/ldap/repository/LdapEncode.java | 26 ++++++-- .../data/ldap/repository/LdapEncoder.java | 64 +++++++++++++++++-- .../data/ldap/repository/Query.java | 8 ++- .../ldap/repository/query/LdapParameters.java | 32 +++++++++- .../repository/query/LdapQueryCreator.java | 2 +- .../repository/query/StringBasedQuery.java | 58 ++++------------- .../ldap/repository/LdapEncoderUnitTests.java | 44 +++++++++++++ ...AnnotatedLdapRepositoryQueryUnitTests.java | 8 +-- 8 files changed, 177 insertions(+), 65 deletions(-) create mode 100644 src/test/java/org/springframework/data/ldap/repository/LdapEncoderUnitTests.java diff --git a/src/main/java/org/springframework/data/ldap/repository/LdapEncode.java b/src/main/java/org/springframework/data/ldap/repository/LdapEncode.java index b4a58378..5c51af69 100644 --- a/src/main/java/org/springframework/data/ldap/repository/LdapEncode.java +++ b/src/main/java/org/springframework/data/ldap/repository/LdapEncode.java @@ -24,18 +24,34 @@ import org.springframework.core.annotation.AliasFor; /** - * Allows passing of custom {@link LdapEncoder}. + * Annotation which indicates that a method parameter should be encoded using a specific {@link LdapEncoder} for a + * repository query method invocation. + *

+ * If no {@link LdapEncoder} is configured, bound method parameters are encoded using + * {@link org.springframework.ldap.support.LdapEncoder#filterEncode(String)}. The default encoder considers chars such + * as {@code *} (asterisk) to be encoded which might interfere with the intent of running a Like query. Since Spring + * Data LDAP doesn't parse queries it is up to you to decide which encoder to use. + *

+ * {@link LdapEncoder} implementations must declare a no-args constructor so they can be instantiated during repository + * initialization. + *

+ * Note that parameter encoding applies only to parameters that are directly bound to a query. Parameters used in Value + * Expressions (SpEL, Configuration Properties) are not considered for encoding and must be encoded properly by using + * SpEL Method invocations or a SpEL Extension. * * @author Marcin Grzejszczak - * @since 3.5.0 + * @author Mark Paluch + * @since 3.5 + * @see LdapEncoder.LikeEncoder + * @see LdapEncoder.NameEncoder */ -@Target(ElementType.PARAMETER) +@Target({ ElementType.PARAMETER, ElementType.ANNOTATION_TYPE }) @Retention(RetentionPolicy.RUNTIME) @Documented public @interface LdapEncode { /** - * {@link LdapEncoder} to instantiate to encode query parameters. + * {@link LdapEncoder} to encode query parameters. * * @return {@link LdapEncoder} class */ @@ -43,7 +59,7 @@ Class value(); /** - * {@link LdapEncoder} to instantiate to encode query parameters. + * {@link LdapEncoder} to encode query parameters. * * @return {@link LdapEncoder} class */ diff --git a/src/main/java/org/springframework/data/ldap/repository/LdapEncoder.java b/src/main/java/org/springframework/data/ldap/repository/LdapEncoder.java index 6e491975..13d2b817 100644 --- a/src/main/java/org/springframework/data/ldap/repository/LdapEncoder.java +++ b/src/main/java/org/springframework/data/ldap/repository/LdapEncoder.java @@ -15,18 +15,70 @@ */ package org.springframework.data.ldap.repository; +import org.springframework.util.ObjectUtils; + /** - * Allows plugging in custom encoding for {@link LdapEncode}. + * Strategy interface to escape values for use in LDAP filters. + *

+ * Accepts an LDAP filter value to be encoded (escaped) for String-based LDAP query usage as LDAP queries do not feature + * an out-of-band parameter binding mechanism. + *

+ * Make sure that your implementation escapes special characters in the value adequately to prevent injection attacks. * * @author Marcin Grzejszczak - * @since 3.5.0 + * @author Mark Paluch + * @since 3.5 */ public interface LdapEncoder { /** - * Escape a value for use in a filter. - * @param value the value to escape. - * @return a properly escaped representation of the supplied value. + * Encode a value for use in a filter. + * + * @param value the value to encode. + * @return a properly encoded representation of the supplied value. + */ + String encode(String value); + + /** + * {@link LdapEncoder} using {@link org.springframework.ldap.support.LdapEncoder#nameEncode(String)}. Encodes a value + * for use with a DN. Escapes for LDAP, not JNDI! + */ + class NameEncoder implements LdapEncoder { + + @Override + public String encode(String value) { + return org.springframework.ldap.support.LdapEncoder.nameEncode(value); + } + } + + /** + * Escape a value for use in a filter retaining asterisks ({@code *}) for like/contains searches. */ - String filterEncode(String value); + class LikeEncoder implements LdapEncoder { + + @Override + public String encode(String value) { + + if (ObjectUtils.isEmpty(value)) { + return value; + } + + String[] substrings = value.split("\\*", -2); + + if (substrings.length == 1) { + return org.springframework.ldap.support.LdapEncoder.filterEncode(substrings[0]); + } + + StringBuilder buff = new StringBuilder(); + for (int i = 0; i < substrings.length; i++) { + buff.append(org.springframework.ldap.support.LdapEncoder.filterEncode(substrings[i])); + if (i < substrings.length - 1) { + buff.append("*"); + } + } + + return buff.toString(); + } + } + } diff --git a/src/main/java/org/springframework/data/ldap/repository/Query.java b/src/main/java/org/springframework/data/ldap/repository/Query.java index 9a50cf88..06f01904 100644 --- a/src/main/java/org/springframework/data/ldap/repository/Query.java +++ b/src/main/java/org/springframework/data/ldap/repository/Query.java @@ -26,6 +26,10 @@ /** * Annotation for use in {@link org.springframework.data.ldap.repository.LdapRepository} declarations to create * automatic query methods based on statically defined queries. + *

+ * LDAP queries support parameter bindings (by-index using ?0, by-name using :someParam), SpEL expressions, and + * Configuration Properties to create dynamic queries. All non-String arguments are converted to their + * {@link Object#toString() toString} representation to render the query. * * @author Mattias Hellborg Arthursson */ @@ -36,7 +40,7 @@ /** * Search base, to be used as input to {@link org.springframework.ldap.query.LdapQueryBuilder#base(javax.naming.Name)} - * . + * . Supports parameter binding. * * @return the search base, default is {@link org.springframework.ldap.support.LdapUtils#emptyLdapName()} */ @@ -44,7 +48,7 @@ /** * The filter format string, to be used as input to - * {@link org.springframework.ldap.query.LdapQueryBuilder#filter(String, Object...)}. + * {@link org.springframework.ldap.query.LdapQueryBuilder#filter(String, Object...)}. Supports parameter binding. * * @return search filter, must be specified. */ diff --git a/src/main/java/org/springframework/data/ldap/repository/query/LdapParameters.java b/src/main/java/org/springframework/data/ldap/repository/query/LdapParameters.java index c5cedc6d..a33eb501 100644 --- a/src/main/java/org/springframework/data/ldap/repository/query/LdapParameters.java +++ b/src/main/java/org/springframework/data/ldap/repository/query/LdapParameters.java @@ -18,18 +18,22 @@ import java.lang.reflect.Method; import java.util.List; +import org.springframework.beans.BeanUtils; import org.springframework.core.MethodParameter; import org.springframework.data.geo.Distance; +import org.springframework.data.ldap.repository.LdapEncode; +import org.springframework.data.ldap.repository.LdapEncoder; import org.springframework.data.repository.query.Parameter; import org.springframework.data.repository.query.Parameters; import org.springframework.data.repository.query.ParametersSource; import org.springframework.data.util.TypeInformation; +import org.springframework.lang.Nullable; /** * Custom extension of {@link Parameters} discovering additional * * @author Marcin Grzejszczak - * @since 3.5.0 + * @since 3.5 */ public class LdapParameters extends Parameters { @@ -65,9 +69,10 @@ protected LdapParameters createFrom(List parameters) { * * @author Marcin Grzejszczak */ - static class LdapParameter extends Parameter { + protected static class LdapParameter extends Parameter { - final MethodParameter parameter; + private final @Nullable LdapEncoder ldapEncoder; + private final MethodParameter parameter; /** * Creates a new {@link LdapParameter}. @@ -76,8 +81,29 @@ static class LdapParameter extends Parameter { * @param domainType must not be {@literal null}. */ LdapParameter(MethodParameter parameter, TypeInformation domainType) { + super(parameter, domainType); this.parameter = parameter; + + LdapEncode encode = parameter.getParameterAnnotation(LdapEncode.class); + + if (encode != null) { + this.ldapEncoder = BeanUtils.instantiateClass(encode.value()); + } else { + this.ldapEncoder = null; + } + } + + public boolean hasLdapEncoder() { + return ldapEncoder != null; + } + + public LdapEncoder getLdapEncoder() { + + if (ldapEncoder == null) { + throw new IllegalStateException("No LdapEncoder found for parameter " + parameter); + } + return ldapEncoder; } } diff --git a/src/main/java/org/springframework/data/ldap/repository/query/LdapQueryCreator.java b/src/main/java/org/springframework/data/ldap/repository/query/LdapQueryCreator.java index ff4331c9..98f06a2f 100644 --- a/src/main/java/org/springframework/data/ldap/repository/query/LdapQueryCreator.java +++ b/src/main/java/org/springframework/data/ldap/repository/query/LdapQueryCreator.java @@ -52,7 +52,7 @@ class LdapQueryCreator extends AbstractQueryCreator entityType, ObjectDirectoryMapper mapper, diff --git a/src/main/java/org/springframework/data/ldap/repository/query/StringBasedQuery.java b/src/main/java/org/springframework/data/ldap/repository/query/StringBasedQuery.java index 9fb5f1e9..2c96265e 100644 --- a/src/main/java/org/springframework/data/ldap/repository/query/StringBasedQuery.java +++ b/src/main/java/org/springframework/data/ldap/repository/query/StringBasedQuery.java @@ -1,5 +1,5 @@ /* - * Copyright 2016-2024 the original author or authors. + * Copyright 2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,6 +15,8 @@ */ package org.springframework.data.ldap.repository.query; +import static org.springframework.data.ldap.repository.query.StringBasedQuery.BindingContext.*; + import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -25,10 +27,8 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; -import org.springframework.beans.BeanUtils; import org.springframework.data.expression.ValueExpression; import org.springframework.data.expression.ValueExpressionParser; -import org.springframework.data.ldap.repository.LdapEncode; import org.springframework.data.repository.query.Parameter; import org.springframework.data.repository.query.ParameterAccessor; import org.springframework.data.repository.query.Parameters; @@ -39,12 +39,11 @@ import org.springframework.util.Assert; import org.springframework.util.StringUtils; -import static org.springframework.data.ldap.repository.query.StringBasedQuery.BindingContext.ParameterBinding; - /** * String-based Query abstracting a query with parameter bindings. * * @author Marcin Grzejszczak + * @author Mark Paluch * @since 3.5 */ class StringBasedQuery { @@ -79,8 +78,7 @@ private ExpressionDependencies createExpressionDependencies() { for (ParameterBinding binding : queryParameterBindings) { if (binding.isExpression()) { - dependencies - .add(binding.getRequiredExpression().getExpressionDependencies()); + dependencies.add(binding.getRequiredExpression().getExpressionDependencies()); } } @@ -256,7 +254,6 @@ private static Matcher findNextBindingOrExpression(String input, int startPositi */ static class ParameterBinder { - private static final String ARGUMENT_PLACEHOLDER = "?_param_?"; private static final Pattern ARGUMENT_PLACEHOLDER_PATTERN = Pattern.compile(Pattern.quote(ARGUMENT_PLACEHOLDER)); @@ -358,15 +355,20 @@ private Object getParameterValueForBinding(ParameterBinding binding) { if (binding.isExpression()) { return evaluator.apply(binding.getRequiredExpression()); } - Object value = binding.isNamed() - ? parameterAccessor.getBindableValue(getParameterIndex(parameters, binding.getRequiredParameterName())) - : parameterAccessor.getBindableValue(binding.getParameterIndex()); + + int index = binding.isNamed() ? getParameterIndex(parameters, binding.getRequiredParameterName()) + : binding.getParameterIndex(); + Object value = parameterAccessor.getBindableValue(index); if (value == null) { return null; } - return binding.getEncodedValue(parameters, value); + String toString = value.toString(); + LdapParameters.LdapParameter parameter = parameters.getBindableParameter(index); + + return parameter.hasLdapEncoder() ? parameter.getLdapEncoder().encode(toString) + : LdapEncoder.filterEncode(toString); } private int getParameterIndex(Parameters parameters, String parameterName) { @@ -413,38 +415,6 @@ static ParameterBinding named(String name) { return new ParameterBinding(-1, null, name); } - Object getEncodedValue(LdapParameters ldapParameters, Object value) { - org.springframework.data.ldap.repository.LdapEncoder encoder = encoderForParameter(ldapParameters); - if (encoder == null) { - return LdapEncoder.filterEncode(value.toString()); - } - return encoder.filterEncode(value.toString()); - } - - - @Nullable - org.springframework.data.ldap.repository.LdapEncoder encoderForParameter(LdapParameters ldapParameters) { - for (LdapParameters.LdapParameter parameter : ldapParameters) { - if (isByName(parameter) || isByIndex(parameter)) { - LdapEncode ldapEncode = parameter.parameter.getParameterAnnotation(LdapEncode.class); - if (ldapEncode == null) { - return null; - } - Class encoder = ldapEncode.value(); - return BeanUtils.instantiateClass(encoder); - } - } - return null; - } - - private boolean isByIndex(LdapParameters.LdapParameter parameter) { - return parameterIndex != -1 && parameter.getIndex() == parameterIndex; - } - - private boolean isByName(LdapParameters.LdapParameter parameter) { - return parameterName != null && parameterName.equals(parameter.getName().orElse(null)); - } - boolean isNamed() { return (parameterName != null); } diff --git a/src/test/java/org/springframework/data/ldap/repository/LdapEncoderUnitTests.java b/src/test/java/org/springframework/data/ldap/repository/LdapEncoderUnitTests.java new file mode 100644 index 00000000..cb825c13 --- /dev/null +++ b/src/test/java/org/springframework/data/ldap/repository/LdapEncoderUnitTests.java @@ -0,0 +1,44 @@ +/* + * Copyright 2024 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.ldap.repository; + +import static org.assertj.core.api.Assertions.*; + +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link LdapEncoder}. + * + * @author Mark Paluch + */ +class LdapEncoderUnitTests { + + @Test // GH-509 + void shouldEncodeName() { + + String result = new LdapEncoder.NameEncoder().encode("# foo ,+\"\\<>; "); + + assertThat(result).isEqualTo("\\# foo \\,\\+\\\"\\\\\\<\\>\\;\\ "); + } + + @Test // GH-509 + void shouldEncodeLikeFilter() { + + String result = new LdapEncoder.LikeEncoder().encode("*hugo*ern(o)*"); + + assertThat(result).isEqualTo("*hugo*ern\\28o\\29*"); + } +} diff --git a/src/test/java/org/springframework/data/ldap/repository/query/AnnotatedLdapRepositoryQueryUnitTests.java b/src/test/java/org/springframework/data/ldap/repository/query/AnnotatedLdapRepositoryQueryUnitTests.java index 3c1e9d91..64fff307 100644 --- a/src/test/java/org/springframework/data/ldap/repository/query/AnnotatedLdapRepositoryQueryUnitTests.java +++ b/src/test/java/org/springframework/data/ldap/repository/query/AnnotatedLdapRepositoryQueryUnitTests.java @@ -15,14 +15,16 @@ */ package org.springframework.data.ldap.repository.query; +import static org.assertj.core.api.Assertions.*; + import java.util.List; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.springframework.data.ldap.core.mapping.LdapMappingContext; -import org.springframework.data.ldap.repository.LdapEncoder; import org.springframework.data.ldap.repository.LdapEncode; +import org.springframework.data.ldap.repository.LdapEncoder; import org.springframework.data.ldap.repository.LdapRepository; import org.springframework.data.ldap.repository.Query; import org.springframework.data.mapping.model.EntityInstantiators; @@ -32,8 +34,6 @@ import org.springframework.ldap.core.LdapOperations; import org.springframework.ldap.query.LdapQuery; -import static org.assertj.core.api.Assertions.assertThat; - /** * Unit tests for {@link AnnotatedLdapRepositoryQuery} * @@ -122,7 +122,7 @@ interface QueryRepository extends LdapRepository { static class MyEncoder implements LdapEncoder { @Override - public String filterEncode(String value) { + public String encode(String value) { return value + "bar"; } }