From c9525ca544b1281e2b7c7347e86e87c86dc1dc6e Mon Sep 17 00:00:00 2001 From: Gunnar Morling Date: Wed, 23 Jul 2014 10:13:18 +0200 Subject: [PATCH] HV-912 Not exposing accessible-made members --- .../internal/engine/ValidatorImpl.java | 107 ++++++++++++++++-- .../resolver/DefaultTraversableResolver.java | 2 +- .../aggregated/ParameterMetaData.java | 5 - .../metadata/aggregated/PropertyMetaData.java | 6 +- .../aggregated/ReturnValueMetaData.java | 5 - .../metadata/core/MetaConstraint.java | 16 --- .../internal/metadata/facets/Cascadable.java | 9 -- .../metadata/raw/ConstrainedExecutable.java | 11 -- .../metadata/raw/ConstrainedField.java | 13 --- 9 files changed, 104 insertions(+), 70 deletions(-) diff --git a/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java b/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java index d66a02a4e8..cc8d5f3092 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java +++ b/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java @@ -17,15 +17,21 @@ package org.hibernate.validator.internal.engine; import java.lang.annotation.ElementType; +import java.lang.reflect.AccessibleObject; import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.lang.reflect.Member; import java.lang.reflect.Method; import java.lang.reflect.Type; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.Arrays; import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentMap; import javax.validation.ConstraintValidatorFactory; import javax.validation.ConstraintViolation; @@ -58,12 +64,17 @@ import org.hibernate.validator.internal.metadata.facets.Cascadable; import org.hibernate.validator.internal.metadata.facets.Validatable; import org.hibernate.validator.internal.metadata.raw.ExecutableElement; +import org.hibernate.validator.internal.util.ConcurrentReferenceHashMap; +import org.hibernate.validator.internal.util.ConcurrentReferenceHashMap.ReferenceType; import org.hibernate.validator.internal.util.Contracts; import org.hibernate.validator.internal.util.ReflectionHelper; import org.hibernate.validator.internal.util.TypeHelper; import org.hibernate.validator.internal.util.TypeResolutionHelper; import org.hibernate.validator.internal.util.logging.Log; import org.hibernate.validator.internal.util.logging.LoggerFactory; +import org.hibernate.validator.internal.util.privilegedactions.GetDeclaredField; +import org.hibernate.validator.internal.util.privilegedactions.GetDeclaredMethod; +import org.hibernate.validator.internal.util.privilegedactions.SetAccessibility; import org.hibernate.validator.spi.valuehandling.ValidatedValueUnwrapper; import com.fasterxml.classmate.ResolvedType; @@ -142,6 +153,11 @@ public class ValidatorImpl implements Validator, ExecutableValidator { */ private final List> validatedValueHandlers; + /** + * Keeps an accessible version for each non-accessible member whose value needs to be accessed during validation. + */ + private final ConcurrentMap accessibleMembers; + public ValidatorImpl(ConstraintValidatorFactory constraintValidatorFactory, MessageInterpolator messageInterpolator, TraversableResolver traversableResolver, @@ -162,6 +178,12 @@ public ValidatorImpl(ConstraintValidatorFactory constraintValidatorFactory, this.failFast = failFast; validationOrderGenerator = new ValidationOrderGenerator(); + + this.accessibleMembers = new ConcurrentReferenceHashMap( + 100, + ReferenceType.SOFT, + ReferenceType.SOFT + ); } @Override @@ -519,7 +541,7 @@ private boolean validateConstraint(ValidationContext validationContext, if ( isValidationRequired( validationContext, valueContext, metaConstraint ) ) { if ( valueContext.getCurrentBean() != null ) { - Object valueToValidate = metaConstraint.getValue( valueContext.getCurrentBean() ); + Object valueToValidate = getValue( metaConstraint.getLocation().getMember(), valueContext.getCurrentBean() ); valueContext.setCurrentValidatedValue( valueToValidate ); } validationSuccessful = metaConstraint.validateConstraint( validationContext, valueContext ); @@ -552,9 +574,8 @@ private void validateCascadedConstraints(ValidationContext validationContext, valueContext.getPropertyPath(), elementType ) ) { - Object value = cascadable.getValue( - valueContext.getCurrentBean() - ); + + Object value = getValue( valueContext.getCurrentBean(), cascadable ); // Value can be wrapped (e.g. Optional
). Try to unwrap it ConstraintMetaData metaData = (ConstraintMetaData) cascadable; @@ -1302,9 +1323,7 @@ else if ( !propertyIter.hasNext() ) { else { if ( property.isCascading() ) { Type type = property.getType(); - newValue = newValue == null ? null : property.getValue( - newValue - ); + newValue = newValue == null ? null : getValue( newValue, property ); if ( elem.isIterable() ) { if ( newValue != null && elem.getIndex() != null ) { newValue = ReflectionHelper.getIndexedValue( newValue, elem.getIndex() ); @@ -1483,4 +1502,78 @@ private void setValidatedValueHandlerToValueContextIfPresent(ValidationConte valueContext.setValidatedValueHandler( handler ); } } + + private Object getValue(Object object, Cascadable cascadable) { + if ( cascadable instanceof PropertyMetaData ) { + Member member = getAccessible( ( (PropertyMetaData) cascadable ).getCascadingMember() ); + return getValue( member, object ); + } + else if ( cascadable instanceof ParameterMetaData ) { + return ( (Object[]) object )[( (ParameterMetaData) cascadable ).getIndex()]; + } + else { + return object; + } + } + + private Object getValue(Member member, Object object) { + if ( member == null ) { + return object; + } + + member = getAccessible( member ); + + if ( member instanceof Method ) { + return ReflectionHelper.getValue( (Method) member, object ); + } + else if ( member instanceof Field ) { + return ReflectionHelper.getValue( (Field) member, object ); + } + return null; + } + + /** + * Returns an accessible version of the given member. Will be the given member itself in case it is accessible, + * otherwise a copy which is set accessible. These copies are maintained in the + * {@link ValidatorImpl#accessibleMembers} cache. + */ + private Member getAccessible(Member original) { + if ( ( (AccessibleObject) original ).isAccessible() ) { + return original; + } + + Member member = accessibleMembers.get( original ); + + if ( member != null ) { + return member; + } + + Class clazz = original.getDeclaringClass(); + + if ( original instanceof Field ) { + member = run( GetDeclaredField.action( clazz, original.getName() ) ); + } + else { + member = run( GetDeclaredMethod.action( clazz, original.getName() ) ); + } + + run( SetAccessibility.action( member ) ); + + Member cached = accessibleMembers.putIfAbsent( original, member ); + if ( cached != null ) { + member = cached; + } + + return member; + } + + /** + * Runs the given privileged action, using a privileged block if required. + *

+ * NOTE: This must never be changed into a publicly available method to avoid execution of arbitrary + * privileged actions within HV's protection domain. + */ + private T run(PrivilegedAction action) { + return System.getSecurityManager() != null ? AccessController.doPrivileged( action ) : action.run(); + } } diff --git a/engine/src/main/java/org/hibernate/validator/internal/engine/resolver/DefaultTraversableResolver.java b/engine/src/main/java/org/hibernate/validator/internal/engine/resolver/DefaultTraversableResolver.java index e8bd592939..f223072950 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/engine/resolver/DefaultTraversableResolver.java +++ b/engine/src/main/java/org/hibernate/validator/internal/engine/resolver/DefaultTraversableResolver.java @@ -99,7 +99,7 @@ private void detectJPA() { // unfortunately there are several incomplete implementations out there (see HV-374) try { Object persistence = run( NewInstance.action( persistenceClass, "persistence provider" ) ); - ReflectionHelper.getValue(persistenceUtilGetter, persistence ); + ReflectionHelper.getValue( persistenceUtilGetter, persistence ); } catch ( Exception e ) { log.debugf( diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ParameterMetaData.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ParameterMetaData.java index abfc611b4f..89d07dd018 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ParameterMetaData.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ParameterMetaData.java @@ -88,11 +88,6 @@ public ElementType getElementType() { return ElementType.PARAMETER; } - @Override - public Object getValue(Object parent) { - return ( (Object[]) parent )[index]; - } - @Override public ParameterDescriptor asDescriptor(boolean defaultGroupSequenceRedefined, List> defaultGroupSequence) { return new ParameterDescriptorImpl( diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/PropertyMetaData.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/PropertyMetaData.java index f226be86e7..74abcaaea4 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/PropertyMetaData.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/PropertyMetaData.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; import java.util.Set; + import javax.validation.ElementKind; import javax.validation.metadata.GroupConversionDescriptor; @@ -94,9 +95,8 @@ private PropertyMetaData(String propertyName, this.groupConversionHelper.validateGroupConversions( isCascading(), this.toString() ); } - @Override - public Object getValue(Object parent) { - return ReflectionHelper.getValue( cascadingMember, parent ); + public Member getCascadingMember() { + return cascadingMember; } @Override diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ReturnValueMetaData.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ReturnValueMetaData.java index 9162b2496e..8c3115da84 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ReturnValueMetaData.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ReturnValueMetaData.java @@ -86,11 +86,6 @@ public ElementType getElementType() { return ElementType.METHOD; } - @Override - public Object getValue(Object parent) { - return parent; - } - @Override public ReturnValueDescriptor asDescriptor(boolean defaultGroupSequenceRedefined, List> defaultGroupSequence) { return new ReturnValueDescriptorImpl( diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraint.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraint.java index b3a33d4e52..e5bbca8494 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraint.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraint.java @@ -25,7 +25,6 @@ import org.hibernate.validator.internal.engine.constraintvalidation.ConstraintTree; import org.hibernate.validator.internal.metadata.descriptor.ConstraintDescriptorImpl; import org.hibernate.validator.internal.metadata.location.ConstraintLocation; -import org.hibernate.validator.internal.util.ReflectionHelper; /** * Instances of this class abstract the constraint type (class, method or field constraint) and give access to @@ -91,21 +90,6 @@ public ConstraintLocation getLocation() { return location; } - /** - * @param o the object from which to retrieve the value. - * - * @return Returns the value for this constraint from the specified object. Depending on the type either the value itself - * is returned of method or field access is used to access the value. - */ - public Object getValue(Object o) { - if ( location.getMember() == null ) { - return o; - } - else { - return ReflectionHelper.getValue( location.getMember(), o ); - } - } - @Override public boolean equals(Object o) { if ( this == o ) { diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/facets/Cascadable.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/facets/Cascadable.java index 3d3ffcbc07..04e2fa8ba4 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/facets/Cascadable.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/facets/Cascadable.java @@ -54,15 +54,6 @@ public interface Cascadable { ElementType getElementType(); - /** - * Retrieves the value of this element from the given object. - * - * @param parent The object to retrieve the value from. - * - * @return This element's value. - */ - Object getValue(Object parent); - /** * Returns the name of this cascadable element. * diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedExecutable.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedExecutable.java index acbfd2407a..41f2691d0b 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedExecutable.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedExecutable.java @@ -18,8 +18,6 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Method; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Collections; import java.util.List; import java.util.Map; @@ -31,7 +29,6 @@ import org.hibernate.validator.internal.metadata.location.ConstraintLocation; import org.hibernate.validator.internal.util.logging.Log; import org.hibernate.validator.internal.util.logging.LoggerFactory; -import org.hibernate.validator.internal.util.privilegedactions.SetAccessibility; import static org.hibernate.validator.internal.util.CollectionHelper.newArrayList; import static org.hibernate.validator.internal.util.CollectionHelper.newHashMap; @@ -143,10 +140,6 @@ public ConstrainedExecutable( this.crossParameterConstraints = crossParameterConstraints; this.parameterMetaData = Collections.unmodifiableList( parameterMetaData ); this.hasParameterConstraints = hasParameterConstraints( parameterMetaData ) || !crossParameterConstraints.isEmpty(); - - if ( isConstrained() ) { - run( SetAccessibility.action( location.getMember() ) ); - } } /** @@ -353,8 +346,4 @@ else if ( !executable.equals( other.executable ) ) { } return true; } - - private T run(PrivilegedAction action) { - return System.getSecurityManager() != null ? AccessController.doPrivileged( action ) : action.run(); - } } diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedField.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedField.java index a43374dda7..6244e8af42 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedField.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedField.java @@ -16,15 +16,11 @@ */ package org.hibernate.validator.internal.metadata.raw; -import java.lang.reflect.Member; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Map; import java.util.Set; import org.hibernate.validator.internal.metadata.core.MetaConstraint; import org.hibernate.validator.internal.metadata.location.ConstraintLocation; -import org.hibernate.validator.internal.util.privilegedactions.SetAccessibility; /** * Represents a field of a Java type and all its associated meta-data relevant @@ -53,11 +49,6 @@ public ConstrainedField(ConfigurationSource source, boolean requiresUnwrapping) { super( source, ConstrainedElementKind.FIELD, location, constraints, groupConversions, isCascading, requiresUnwrapping ); - - Member member = location.getMember(); - if ( member != null && isConstrained() ) { - run( SetAccessibility.action( member ) ); - } } @Override @@ -90,8 +81,4 @@ else if ( !getLocation().getMember().equals( other.getLocation().getMember() ) ) } return true; } - - private T run(PrivilegedAction action) { - return System.getSecurityManager() != null ? AccessController.doPrivileged( action ) : action.run(); - } }