From 771ef0dadcbaf0e9887a36d884167c80025654e5 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 30 Jul 2020 21:09:15 -0700 Subject: [PATCH] Polish spring-security-core main code Manually polish `spring-security-core` following the formatting and checkstyle fixes. Issue gh-8945 --- .../security/access/SecurityConfig.java | 4 - .../Jsr250MethodSecurityMetadataSource.java | 11 +- .../access/annotation/Jsr250Voter.java | 4 - ...curedAnnotationSecurityMetadataSource.java | 10 +- ...uthenticationCredentialsNotFoundEvent.java | 12 +- .../event/AuthorizationFailureEvent.java | 14 +- .../access/event/AuthorizedEvent.java | 11 +- .../security/access/event/LoggerListener.java | 58 +++--- .../AbstractSecurityExpressionHandler.java | 9 +- .../DenyAllPermissionEvaluator.java | 9 +- .../expression/SecurityExpressionRoot.java | 13 +- ...efaultMethodSecurityExpressionHandler.java | 170 ++++++++---------- .../ExpressionBasedPostInvocationAdvice.java | 18 +- .../ExpressionBasedPreInvocationAdvice.java | 39 ++-- .../MethodSecurityEvaluationContext.java | 4 - .../PreInvocationExpressionAttribute.java | 2 - .../hierarchicalroles/RoleHierarchyImpl.java | 42 ++--- .../hierarchicalroles/RoleHierarchyUtils.java | 21 +-- .../AbstractSecurityInterceptor.java | 158 ++++++---------- .../AfterInvocationProviderManager.java | 23 +-- .../MethodInvocationPrivilegeEvaluator.java | 30 ++-- .../RunAsImplAuthenticationProvider.java | 7 +- .../access/intercept/RunAsManagerImpl.java | 4 - .../access/intercept/RunAsUserToken.java | 1 - .../MethodSecurityInterceptor.java | 1 - .../MethodSecurityMetadataSourceAdvisor.java | 11 +- .../AspectJMethodSecurityInterceptor.java | 2 - .../aspectj/MethodInvocationAdapter.java | 11 +- ...tFallbackMethodSecurityMetadataSource.java | 2 - .../AbstractMethodSecurityMetadataSource.java | 2 - ...elegatingMethodSecurityMetadataSource.java | 15 +- .../MapBasedMethodSecurityMetadataSource.java | 69 +++---- .../prepost/PostInvocationAdviceProvider.java | 11 +- ...PreInvocationAuthorizationAdviceVoter.java | 12 +- ...rePostAdviceReactiveMethodInterceptor.java | 16 +- ...ePostAnnotationSecurityMetadataSource.java | 28 +-- .../vote/AbstractAccessDecisionManager.java | 6 +- .../access/vote/AbstractAclVoter.java | 9 +- .../access/vote/AffirmativeBased.java | 14 +- .../access/vote/AuthenticatedVoter.java | 5 - .../security/access/vote/ConsensusBased.java | 23 +-- .../security/access/vote/RoleVoter.java | 3 - .../security/access/vote/UnanimousBased.java | 17 +- .../AbstractAuthenticationToken.java | 31 +--- ...rDetailsReactiveAuthenticationManager.java | 51 +++--- .../AccountStatusUserDetailsChecker.java | 3 - .../AnonymousAuthenticationProvider.java | 2 - .../AnonymousAuthenticationToken.java | 15 +- .../AuthenticationTrustResolverImpl.java | 2 - .../CachingUserDetailsService.java | 4 - .../DefaultAuthenticationEventPublisher.java | 3 - .../authentication/ProviderManager.java | 28 +-- .../ReactiveAuthenticationManagerAdapter.java | 19 +- .../RememberMeAuthenticationProvider.java | 2 - .../RememberMeAuthenticationToken.java | 11 +- .../UsernamePasswordAuthenticationToken.java | 8 +- ...ractUserDetailsAuthenticationProvider.java | 58 ++---- .../dao/DaoAuthenticationProvider.java | 4 - .../authentication/event/LoggerListener.java | 30 ++-- .../AbstractJaasAuthenticationProvider.java | 85 ++++----- .../jaas/JaasAuthenticationProvider.java | 13 +- .../jaas/JaasNameCallbackHandler.java | 22 +-- .../jaas/JaasPasswordCallbackHandler.java | 3 +- .../jaas/SecurityContextLoginModule.java | 29 +-- .../rcp/RemoteAuthenticationManagerImpl.java | 5 +- .../rcp/RemoteAuthenticationProvider.java | 1 - ...enticatedReactiveAuthorizationManager.java | 6 +- ...AuthorityReactiveAuthorizationManager.java | 8 +- .../ReactiveAuthorizationManager.java | 4 +- .../DelegatingSecurityContextCallable.java | 1 - .../DelegatingSecurityContextExecutor.java | 3 +- ...egatingSecurityContextExecutorService.java | 9 +- .../DelegatingSecurityContextRunnable.java | 1 - ...curityContextScheduledExecutorService.java | 12 +- .../security/converter/RsaKeyConverters.java | 6 +- .../core/SpringSecurityCoreVersion.java | 15 +- .../core/authority/AuthorityUtils.java | 9 +- .../authority/SimpleGrantedAuthority.java | 2 - ...edAttributes2GrantedAuthoritiesMapper.java | 25 ++- .../mapping/SimpleAuthorityMapper.java | 4 - .../GlobalSecurityContextHolderStrategy.java | 1 - ...eadLocalSecurityContextHolderStrategy.java | 2 - .../ReactiveSecurityContextHolder.java | 12 +- .../core/context/SecurityContextHolder.java | 58 +++--- .../core/context/SecurityContextImpl.java | 21 +-- ...eadLocalSecurityContextHolderStrategy.java | 2 - .../AnnotationParameterNameDiscoverer.java | 12 +- ...efaultSecurityParameterNameDiscoverer.java | 7 - .../core/session/SessionRegistryImpl.java | 45 +---- .../KeyBasedPersistenceTokenService.java | 17 +- .../core/token/SecureRandomFactoryBean.java | 11 +- .../MapReactiveUserDetailsService.java | 13 +- .../security/core/userdetails/User.java | 30 +--- .../cache/EhCacheBasedUserCache.java | 25 +-- .../cache/SpringCacheBasedUserCache.java | 23 +-- .../core/userdetails/jdbc/JdbcDaoImpl.java | 15 -- .../memory/UserAttributeEditor.java | 54 +++--- .../jackson2/SecurityJackson2Modules.java | 37 ++-- .../security/jackson2/UserDeserializer.java | 23 ++- ...sswordAuthenticationTokenDeserializer.java | 55 +++--- .../InMemoryUserDetailsManager.java | 30 +--- .../provisioning/JdbcUserDetailsManager.java | 108 ++++------- ...atingSecurityContextAsyncTaskExecutor.java | 9 +- .../security/util/FieldUtils.java | 10 +- .../security/util/MethodInvocationUtils.java | 39 ++-- 105 files changed, 753 insertions(+), 1371 deletions(-) diff --git a/core/src/main/java/org/springframework/security/access/SecurityConfig.java b/core/src/main/java/org/springframework/security/access/SecurityConfig.java index c6888dff8a..4be11c2dfc 100644 --- a/core/src/main/java/org/springframework/security/access/SecurityConfig.java +++ b/core/src/main/java/org/springframework/security/access/SecurityConfig.java @@ -40,10 +40,8 @@ public class SecurityConfig implements ConfigAttribute { public boolean equals(Object obj) { if (obj instanceof ConfigAttribute) { ConfigAttribute attr = (ConfigAttribute) obj; - return this.attrib.equals(attr.getAttribute()); } - return false; } @@ -69,11 +67,9 @@ public class SecurityConfig implements ConfigAttribute { public static List createList(String... attributeNames) { Assert.notNull(attributeNames, "You must supply an array of attribute names"); List attributes = new ArrayList<>(attributeNames.length); - for (String attribute : attributeNames) { attributes.add(new SecurityConfig(attribute.trim())); } - return attributes; } diff --git a/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java index b2c9efa2f6..2cc9700280 100644 --- a/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java @@ -76,18 +76,17 @@ public class Jsr250MethodSecurityMetadataSource extends AbstractFallbackMethodSe return null; } List attributes = new ArrayList<>(); - - for (Annotation a : annotations) { - if (a instanceof DenyAll) { + for (Annotation annotation : annotations) { + if (annotation instanceof DenyAll) { attributes.add(Jsr250SecurityConfig.DENY_ALL_ATTRIBUTE); return attributes; } - if (a instanceof PermitAll) { + if (annotation instanceof PermitAll) { attributes.add(Jsr250SecurityConfig.PERMIT_ALL_ATTRIBUTE); return attributes; } - if (a instanceof RolesAllowed) { - RolesAllowed ra = (RolesAllowed) a; + if (annotation instanceof RolesAllowed) { + RolesAllowed ra = (RolesAllowed) annotation; for (String allowed : ra.value()) { String defaultedAllowed = getRoleWithDefaultPrefix(allowed); diff --git a/core/src/main/java/org/springframework/security/access/annotation/Jsr250Voter.java b/core/src/main/java/org/springframework/security/access/annotation/Jsr250Voter.java index 78e6778bbb..d94ae1d3da 100644 --- a/core/src/main/java/org/springframework/security/access/annotation/Jsr250Voter.java +++ b/core/src/main/java/org/springframework/security/access/annotation/Jsr250Voter.java @@ -65,16 +65,13 @@ public class Jsr250Voter implements AccessDecisionVoter { @Override public int vote(Authentication authentication, Object object, Collection definition) { boolean jsr250AttributeFound = false; - for (ConfigAttribute attribute : definition) { if (Jsr250SecurityConfig.PERMIT_ALL_ATTRIBUTE.equals(attribute)) { return ACCESS_GRANTED; } - if (Jsr250SecurityConfig.DENY_ALL_ATTRIBUTE.equals(attribute)) { return ACCESS_DENIED; } - if (supports(attribute)) { jsr250AttributeFound = true; // Attempt to find a matching granted authority @@ -85,7 +82,6 @@ public class Jsr250Voter implements AccessDecisionVoter { } } } - return jsr250AttributeFound ? ACCESS_DENIED : ACCESS_ABSTAIN; } diff --git a/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java index 903e91b4a2..152b52ec43 100644 --- a/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java @@ -74,12 +74,8 @@ public class SecuredAnnotationSecurityMetadataSource extends AbstractFallbackMet return null; } - private Collection processAnnotation(Annotation a) { - if (a == null) { - return null; - } - - return this.annotationExtractor.extractAttributes(a); + private Collection processAnnotation(Annotation annotation) { + return (annotation != null) ? this.annotationExtractor.extractAttributes(annotation) : null; } static class SecuredAnnotationMetadataExtractor implements AnnotationMetadataExtractor { @@ -88,11 +84,9 @@ public class SecuredAnnotationSecurityMetadataSource extends AbstractFallbackMet public Collection extractAttributes(Secured secured) { String[] attributeTokens = secured.value(); List attributes = new ArrayList<>(attributeTokens.length); - for (String token : attributeTokens) { attributes.add(new SecurityConfig(token)); } - return attributes; } diff --git a/core/src/main/java/org/springframework/security/access/event/AuthenticationCredentialsNotFoundEvent.java b/core/src/main/java/org/springframework/security/access/event/AuthenticationCredentialsNotFoundEvent.java index 2a380cdb73..2474a81059 100644 --- a/core/src/main/java/org/springframework/security/access/event/AuthenticationCredentialsNotFoundEvent.java +++ b/core/src/main/java/org/springframework/security/access/event/AuthenticationCredentialsNotFoundEvent.java @@ -20,6 +20,7 @@ import java.util.Collection; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException; +import org.springframework.util.Assert; /** * Indicates a secure object invocation failed because the Authentication @@ -29,9 +30,9 @@ import org.springframework.security.authentication.AuthenticationCredentialsNotF */ public class AuthenticationCredentialsNotFoundEvent extends AbstractAuthorizationEvent { - private AuthenticationCredentialsNotFoundException credentialsNotFoundException; + private final AuthenticationCredentialsNotFoundException credentialsNotFoundException; - private Collection configAttribs; + private final Collection configAttribs; /** * Construct the event. @@ -44,11 +45,8 @@ public class AuthenticationCredentialsNotFoundEvent extends AbstractAuthorizatio public AuthenticationCredentialsNotFoundEvent(Object secureObject, Collection attributes, AuthenticationCredentialsNotFoundException credentialsNotFoundException) { super(secureObject); - - if ((attributes == null) || (credentialsNotFoundException == null)) { - throw new IllegalArgumentException("All parameters are required and cannot be null"); - } - + Assert.isTrue(attributes != null && credentialsNotFoundException != null, + "All parameters are required and cannot be null"); this.configAttribs = attributes; this.credentialsNotFoundException = credentialsNotFoundException; } diff --git a/core/src/main/java/org/springframework/security/access/event/AuthorizationFailureEvent.java b/core/src/main/java/org/springframework/security/access/event/AuthorizationFailureEvent.java index a3a39198e3..de6a301c6a 100644 --- a/core/src/main/java/org/springframework/security/access/event/AuthorizationFailureEvent.java +++ b/core/src/main/java/org/springframework/security/access/event/AuthorizationFailureEvent.java @@ -21,6 +21,7 @@ import java.util.Collection; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.core.Authentication; +import org.springframework.util.Assert; /** * Indicates a secure object invocation failed because the principal could not be @@ -36,11 +37,11 @@ import org.springframework.security.core.Authentication; */ public class AuthorizationFailureEvent extends AbstractAuthorizationEvent { - private AccessDeniedException accessDeniedException; + private final AccessDeniedException accessDeniedException; - private Authentication authentication; + private final Authentication authentication; - private Collection configAttributes; + private final Collection configAttributes; /** * Construct the event. @@ -54,11 +55,8 @@ public class AuthorizationFailureEvent extends AbstractAuthorizationEvent { public AuthorizationFailureEvent(Object secureObject, Collection attributes, Authentication authentication, AccessDeniedException accessDeniedException) { super(secureObject); - - if ((attributes == null) || (authentication == null) || (accessDeniedException == null)) { - throw new IllegalArgumentException("All parameters are required and cannot be null"); - } - + Assert.isTrue(attributes != null && authentication != null && accessDeniedException != null, + "All parameters are required and cannot be null"); this.configAttributes = attributes; this.authentication = authentication; this.accessDeniedException = accessDeniedException; diff --git a/core/src/main/java/org/springframework/security/access/event/AuthorizedEvent.java b/core/src/main/java/org/springframework/security/access/event/AuthorizedEvent.java index dbe2e8d434..f3b05d655b 100644 --- a/core/src/main/java/org/springframework/security/access/event/AuthorizedEvent.java +++ b/core/src/main/java/org/springframework/security/access/event/AuthorizedEvent.java @@ -20,6 +20,7 @@ import java.util.Collection; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.core.Authentication; +import org.springframework.util.Assert; /** * Event indicating a secure object was invoked successfully. @@ -31,9 +32,9 @@ import org.springframework.security.core.Authentication; */ public class AuthorizedEvent extends AbstractAuthorizationEvent { - private Authentication authentication; + private final Authentication authentication; - private Collection configAttributes; + private final Collection configAttributes; /** * Construct the event. @@ -44,11 +45,7 @@ public class AuthorizedEvent extends AbstractAuthorizationEvent { */ public AuthorizedEvent(Object secureObject, Collection attributes, Authentication authentication) { super(secureObject); - - if ((attributes == null) || (authentication == null)) { - throw new IllegalArgumentException("All parameters are required and cannot be null"); - } - + Assert.isTrue(attributes != null && authentication != null, "All parameters are required and cannot be null"); this.configAttributes = attributes; this.authentication = authentication; } diff --git a/core/src/main/java/org/springframework/security/access/event/LoggerListener.java b/core/src/main/java/org/springframework/security/access/event/LoggerListener.java index 57e6d52d17..02247ceb15 100644 --- a/core/src/main/java/org/springframework/security/access/event/LoggerListener.java +++ b/core/src/main/java/org/springframework/security/access/event/LoggerListener.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.context.ApplicationListener; +import org.springframework.core.log.LogMessage; /** * Outputs interceptor-related application events to Commons Logging. @@ -37,42 +38,41 @@ public class LoggerListener implements ApplicationListener private ExpressionParser expressionParser = new SpelExpressionParser(); - private BeanResolver br; + private BeanResolver beanResolver; private RoleHierarchy roleHierarchy; @@ -70,9 +70,8 @@ public abstract class AbstractSecurityExpressionHandler public final EvaluationContext createEvaluationContext(Authentication authentication, T invocation) { SecurityExpressionOperations root = createSecurityExpressionRoot(authentication, invocation); StandardEvaluationContext ctx = createEvaluationContextInternal(authentication, invocation); - ctx.setBeanResolver(this.br); + ctx.setBeanResolver(this.beanResolver); ctx.setRootObject(root); - return ctx; } @@ -96,7 +95,7 @@ public abstract class AbstractSecurityExpressionHandler * invocation type. * @param authentication the current authentication object * @param invocation the invocation (filter, method, channel) - * @return the object wh + * @return the object */ protected abstract SecurityExpressionOperations createSecurityExpressionRoot(Authentication authentication, T invocation); @@ -119,7 +118,7 @@ public abstract class AbstractSecurityExpressionHandler @Override public void setApplicationContext(ApplicationContext applicationContext) { - this.br = new BeanFactoryResolver(applicationContext); + this.beanResolver = new BeanFactoryResolver(applicationContext); } } diff --git a/core/src/main/java/org/springframework/security/access/expression/DenyAllPermissionEvaluator.java b/core/src/main/java/org/springframework/security/access/expression/DenyAllPermissionEvaluator.java index 760f8059de..2aae6b7a24 100644 --- a/core/src/main/java/org/springframework/security/access/expression/DenyAllPermissionEvaluator.java +++ b/core/src/main/java/org/springframework/security/access/expression/DenyAllPermissionEvaluator.java @@ -21,6 +21,7 @@ import java.io.Serializable; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.PermissionEvaluator; import org.springframework.security.core.Authentication; @@ -40,8 +41,8 @@ public class DenyAllPermissionEvaluator implements PermissionEvaluator { */ @Override public boolean hasPermission(Authentication authentication, Object target, Object permission) { - this.logger.warn( - "Denying user " + authentication.getName() + " permission '" + permission + "' on object " + target); + this.logger.warn(LogMessage.format("Denying user %s permission '%s' on object %s", authentication.getName(), + permission, target)); return false; } @@ -51,8 +52,8 @@ public class DenyAllPermissionEvaluator implements PermissionEvaluator { @Override public boolean hasPermission(Authentication authentication, Serializable targetId, String targetType, Object permission) { - this.logger.warn("Denying user " + authentication.getName() + " permission '" + permission - + "' on object with Id '" + targetId); + this.logger.warn(LogMessage.format("Denying user %s permission '%s' on object with Id %s", + authentication.getName(), permission, targetId)); return false; } diff --git a/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java b/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java index b665d23c45..155b317638 100644 --- a/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java +++ b/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java @@ -45,10 +45,14 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat private String defaultRolePrefix = "ROLE_"; - /** Allows "permitAll" expression */ + /** + * Allows "permitAll" expression + */ public final boolean permitAll = true; - /** Allows "denyAll" expression */ + /** + * Allows "denyAll" expression + */ public final boolean denyAll = false; private PermissionEvaluator permissionEvaluator; @@ -96,14 +100,12 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat private boolean hasAnyAuthorityName(String prefix, String... roles) { Set roleSet = getAuthoritySet(); - for (String role : roles) { String defaultedRole = getRoleWithDefaultPrefix(prefix, role); if (roleSet.contains(defaultedRole)) { return true; } } - return false; } @@ -180,14 +182,11 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat private Set getAuthoritySet() { if (this.roles == null) { Collection userAuthorities = this.authentication.getAuthorities(); - if (this.roleHierarchy != null) { userAuthorities = this.roleHierarchy.getReachableGrantedAuthorities(userAuthorities); } - this.roles = AuthorityUtils.authorityListToSet(userAuthorities); } - return this.roles; } diff --git a/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java b/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java index 95b5bced1b..6254ee087d 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.core.ParameterNameDiscoverer; +import org.springframework.core.log.LogMessage; import org.springframework.expression.EvaluationContext; import org.springframework.expression.Expression; import org.springframework.expression.spel.support.StandardEvaluationContext; @@ -88,7 +89,6 @@ public class DefaultMethodSecurityExpressionHandler extends AbstractSecurityExpr root.setTrustResolver(getTrustResolver()); root.setRoleHierarchy(getRoleHierarchy()); root.setDefaultRolePrefix(getDefaultRolePrefix()); - return root; } @@ -101,119 +101,91 @@ public class DefaultMethodSecurityExpressionHandler extends AbstractSecurityExpr * {@code true}. For an array, a new array instance will be returned. */ @Override - @SuppressWarnings("unchecked") public Object filter(Object filterTarget, Expression filterExpression, EvaluationContext ctx) { MethodSecurityExpressionOperations rootObject = (MethodSecurityExpressionOperations) ctx.getRootObject() .getValue(); - final boolean debug = this.logger.isDebugEnabled(); - List retainList; - - if (debug) { - this.logger.debug("Filtering with expression: " + filterExpression.getExpressionString()); - } - + this.logger.debug(LogMessage.format("Filtering with expression: %s", filterExpression.getExpressionString())); if (filterTarget instanceof Collection) { - Collection collection = (Collection) filterTarget; - retainList = new ArrayList(collection.size()); - - if (debug) { - this.logger.debug("Filtering collection with " + collection.size() + " elements"); - } - - if (this.permissionCacheOptimizer != null) { - this.permissionCacheOptimizer.cachePermissionsFor(rootObject.getAuthentication(), collection); - } - - for (Object filterObject : (Collection) filterTarget) { - rootObject.setFilterObject(filterObject); - - if (ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) { - retainList.add(filterObject); - } - } - - if (debug) { - this.logger.debug("Retaining elements: " + retainList); - } - - collection.clear(); - collection.addAll(retainList); - - return filterTarget; + return filterCollection((Collection) filterTarget, filterExpression, ctx, rootObject); } - if (filterTarget.getClass().isArray()) { - Object[] array = (Object[]) filterTarget; - retainList = new ArrayList(array.length); - - if (debug) { - this.logger.debug("Filtering array with " + array.length + " elements"); - } - - if (this.permissionCacheOptimizer != null) { - this.permissionCacheOptimizer.cachePermissionsFor(rootObject.getAuthentication(), Arrays.asList(array)); - } - - for (Object o : array) { - rootObject.setFilterObject(o); - - if (ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) { - retainList.add(o); - } - } - - if (debug) { - this.logger.debug("Retaining elements: " + retainList); - } - - Object[] filtered = (Object[]) Array.newInstance(filterTarget.getClass().getComponentType(), - retainList.size()); - for (int i = 0; i < retainList.size(); i++) { - filtered[i] = retainList.get(i); - } - - return filtered; + return filterArray((Object[]) filterTarget, filterExpression, ctx, rootObject); } - if (filterTarget instanceof Map) { - final Map map = (Map) filterTarget; - final Map retainMap = new LinkedHashMap(map.size()); - - if (debug) { - this.logger.debug("Filtering map with " + map.size() + " elements"); - } - - for (Map.Entry filterObject : map.entrySet()) { - rootObject.setFilterObject(filterObject); - - if (ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) { - retainMap.put(filterObject.getKey(), filterObject.getValue()); - } - } - - if (debug) { - this.logger.debug("Retaining elements: " + retainMap); - } - - map.clear(); - map.putAll(retainMap); - - return filterTarget; + return filterMap((Map) filterTarget, filterExpression, ctx, rootObject); } - if (filterTarget instanceof Stream) { - final Stream original = (Stream) filterTarget; - - return original.filter((filterObject) -> { - rootObject.setFilterObject(filterObject); - return ExpressionUtils.evaluateAsBoolean(filterExpression, ctx); - }).onClose(original::close); + return filterStream((Stream) filterTarget, filterExpression, ctx, rootObject); } - throw new IllegalArgumentException( "Filter target must be a collection, array, map or stream type, but was " + filterTarget); } + private Object filterCollection(Collection filterTarget, Expression filterExpression, EvaluationContext ctx, + MethodSecurityExpressionOperations rootObject) { + this.logger.debug(LogMessage.format("Filtering collection with %s elements", filterTarget.size())); + List retain = new ArrayList<>(filterTarget.size()); + if (this.permissionCacheOptimizer != null) { + this.permissionCacheOptimizer.cachePermissionsFor(rootObject.getAuthentication(), filterTarget); + } + for (T filterObject : filterTarget) { + rootObject.setFilterObject(filterObject); + if (ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) { + retain.add(filterObject); + } + } + this.logger.debug(LogMessage.format("Retaining elements: %s", retain)); + filterTarget.clear(); + filterTarget.addAll(retain); + return filterTarget; + } + + private Object filterArray(Object[] filterTarget, Expression filterExpression, EvaluationContext ctx, + MethodSecurityExpressionOperations rootObject) { + List retain = new ArrayList<>(filterTarget.length); + this.logger.debug(LogMessage.format("Filtering array with %s elements", filterTarget.length)); + if (this.permissionCacheOptimizer != null) { + this.permissionCacheOptimizer.cachePermissionsFor(rootObject.getAuthentication(), + Arrays.asList(filterTarget)); + } + for (Object filterObject : filterTarget) { + rootObject.setFilterObject(filterObject); + if (ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) { + retain.add(filterObject); + } + } + this.logger.debug(LogMessage.format("Retaining elements: %s", retain)); + Object[] filtered = (Object[]) Array.newInstance(filterTarget.getClass().getComponentType(), retain.size()); + for (int i = 0; i < retain.size(); i++) { + filtered[i] = retain.get(i); + } + return filtered; + } + + private Object filterMap(final Map filterTarget, Expression filterExpression, EvaluationContext ctx, + MethodSecurityExpressionOperations rootObject) { + Map retain = new LinkedHashMap<>(filterTarget.size()); + this.logger.debug(LogMessage.format("Filtering map with %s elements", filterTarget.size())); + for (Map.Entry filterObject : filterTarget.entrySet()) { + rootObject.setFilterObject(filterObject); + if (ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) { + retain.put(filterObject.getKey(), filterObject.getValue()); + } + } + this.logger.debug(LogMessage.format("Retaining elements: %s", retain)); + filterTarget.clear(); + filterTarget.putAll(retain); + return filterTarget; + } + + private Object filterStream(final Stream filterTarget, Expression filterExpression, EvaluationContext ctx, + MethodSecurityExpressionOperations rootObject) { + return filterTarget.filter((filterObject) -> { + rootObject.setFilterObject(filterObject); + return ExpressionUtils.evaluateAsBoolean(filterExpression, ctx); + }).onClose(filterTarget::close); + } + /** * Sets the {@link AuthenticationTrustResolver} to be used. The default is * {@link AuthenticationTrustResolverImpl}. diff --git a/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPostInvocationAdvice.java b/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPostInvocationAdvice.java index 100172d57e..664f97102e 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPostInvocationAdvice.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPostInvocationAdvice.java @@ -20,6 +20,7 @@ import org.aopalliance.intercept.MethodInvocation; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.expression.EvaluationContext; import org.springframework.expression.Expression; import org.springframework.security.access.AccessDeniedException; @@ -49,31 +50,20 @@ public class ExpressionBasedPostInvocationAdvice implements PostInvocationAuthor EvaluationContext ctx = this.expressionHandler.createEvaluationContext(authentication, mi); Expression postFilter = pia.getFilterExpression(); Expression postAuthorize = pia.getAuthorizeExpression(); - if (postFilter != null) { - if (this.logger.isDebugEnabled()) { - this.logger.debug("Applying PostFilter expression " + postFilter); - } - + this.logger.debug(LogMessage.format("Applying PostFilter expression %s", postFilter)); if (returnedObject != null) { returnedObject = this.expressionHandler.filter(returnedObject, postFilter, ctx); } else { - if (this.logger.isDebugEnabled()) { - this.logger.debug("Return object is null, filtering will be skipped"); - } + this.logger.debug("Return object is null, filtering will be skipped"); } } - this.expressionHandler.setReturnObject(returnedObject, ctx); - if (postAuthorize != null && !ExpressionUtils.evaluateAsBoolean(postAuthorize, ctx)) { - if (this.logger.isDebugEnabled()) { - this.logger.debug("PostAuthorize expression rejected access"); - } + this.logger.debug("PostAuthorize expression rejected access"); throw new AccessDeniedException("Access is denied"); } - return returnedObject; } diff --git a/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java b/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java index da6cfdacba..19c7659407 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java @@ -26,6 +26,7 @@ import org.springframework.security.access.expression.ExpressionUtils; import org.springframework.security.access.prepost.PreInvocationAttribute; import org.springframework.security.access.prepost.PreInvocationAuthorizationAdvice; import org.springframework.security.core.Authentication; +import org.springframework.util.Assert; /** * Method pre-invocation handling based on expressions. @@ -43,50 +44,34 @@ public class ExpressionBasedPreInvocationAdvice implements PreInvocationAuthoriz EvaluationContext ctx = this.expressionHandler.createEvaluationContext(authentication, mi); Expression preFilter = preAttr.getFilterExpression(); Expression preAuthorize = preAttr.getAuthorizeExpression(); - if (preFilter != null) { Object filterTarget = findFilterTarget(preAttr.getFilterTarget(), ctx, mi); - this.expressionHandler.filter(filterTarget, preFilter, ctx); } - - if (preAuthorize == null) { - return true; - } - - return ExpressionUtils.evaluateAsBoolean(preAuthorize, ctx); + return (preAuthorize != null) ? ExpressionUtils.evaluateAsBoolean(preAuthorize, ctx) : true; } - private Object findFilterTarget(String filterTargetName, EvaluationContext ctx, MethodInvocation mi) { + private Object findFilterTarget(String filterTargetName, EvaluationContext ctx, MethodInvocation invocation) { Object filterTarget = null; - if (filterTargetName.length() > 0) { filterTarget = ctx.lookupVariable(filterTargetName); - if (filterTarget == null) { - throw new IllegalArgumentException( - "Filter target was null, or no argument with name " + filterTargetName + " found in method"); - } + Assert.notNull(filterTarget, + () -> "Filter target was null, or no argument with name " + filterTargetName + " found in method"); } - else if (mi.getArguments().length == 1) { - Object arg = mi.getArguments()[0]; + else if (invocation.getArguments().length == 1) { + Object arg = invocation.getArguments()[0]; if (arg.getClass().isArray() || arg instanceof Collection) { filterTarget = arg; } - if (filterTarget == null) { - throw new IllegalArgumentException("A PreFilter expression was set but the method argument type" - + arg.getClass() + " is not filterable"); - } + Assert.notNull(filterTarget, () -> "A PreFilter expression was set but the method argument type" + + arg.getClass() + " is not filterable"); } - else if (mi.getArguments().length > 1) { + else if (invocation.getArguments().length > 1) { throw new IllegalArgumentException( "Unable to determine the method argument for filtering. Specify the filter target."); } - - if (filterTarget.getClass().isArray()) { - throw new IllegalArgumentException( - "Pre-filtering on array types is not supported. " + "Using a Collection will solve this problem"); - } - + Assert.isTrue(!filterTarget.getClass().isArray(), + "Pre-filtering on array types is not supported. Using a Collection will solve this problem"); return filterTarget; } diff --git a/core/src/main/java/org/springframework/security/access/expression/method/MethodSecurityEvaluationContext.java b/core/src/main/java/org/springframework/security/access/expression/method/MethodSecurityEvaluationContext.java index 1d19fa908a..5ba0a376b6 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/MethodSecurityEvaluationContext.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/MethodSecurityEvaluationContext.java @@ -19,8 +19,6 @@ package org.springframework.security.access.expression.method; import java.lang.reflect.Method; import org.aopalliance.intercept.MethodInvocation; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.springframework.aop.framework.AopProxyUtils; import org.springframework.aop.support.AopUtils; @@ -40,8 +38,6 @@ import org.springframework.security.core.parameters.DefaultSecurityParameterName */ class MethodSecurityEvaluationContext extends MethodBasedEvaluationContext { - private static final Log logger = LogFactory.getLog(MethodSecurityEvaluationContext.class); - /** * Intended for testing. Don't use in practice as it creates a new parameter resolver * for each instance. Use the constructor which takes the resolver, as an argument diff --git a/core/src/main/java/org/springframework/security/access/expression/method/PreInvocationExpressionAttribute.java b/core/src/main/java/org/springframework/security/access/expression/method/PreInvocationExpressionAttribute.java index 5f5ffc56cd..f8c3bae329 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/PreInvocationExpressionAttribute.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/PreInvocationExpressionAttribute.java @@ -32,14 +32,12 @@ class PreInvocationExpressionAttribute extends AbstractExpressionBasedMethodConf PreInvocationExpressionAttribute(String filterExpression, String filterTarget, String authorizeExpression) throws ParseException { super(filterExpression, authorizeExpression); - this.filterTarget = filterTarget; } PreInvocationExpressionAttribute(Expression filterExpression, String filterTarget, Expression authorizeExpression) throws ParseException { super(filterExpression, authorizeExpression); - this.filterTarget = filterTarget; } diff --git a/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java b/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java index 054472191b..e0988445fe 100755 --- a/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java +++ b/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java @@ -20,13 +20,13 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; -import java.util.List; import java.util.Map; import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.authority.SimpleGrantedAuthority; @@ -109,11 +109,8 @@ public class RoleHierarchyImpl implements RoleHierarchy { */ public void setHierarchy(String roleHierarchyStringRepresentation) { this.roleHierarchyStringRepresentation = roleHierarchyStringRepresentation; - - if (logger.isDebugEnabled()) { - logger.debug("setHierarchy() - The following role hierarchy was set: " + roleHierarchyStringRepresentation); - } - + logger.debug(LogMessage.format("setHierarchy() - The following role hierarchy was set: %s", + roleHierarchyStringRepresentation)); buildRolesReachableInOneStepMap(); buildRolesReachableInOneOrMoreStepsMap(); } @@ -124,10 +121,8 @@ public class RoleHierarchyImpl implements RoleHierarchy { if (authorities == null || authorities.isEmpty()) { return AuthorityUtils.NO_AUTHORITIES; } - Set reachableRoles = new HashSet<>(); Set processedNames = new HashSet<>(); - for (GrantedAuthority authority : authorities) { // Do not process authorities without string representation if (authority.getAuthority() == null) { @@ -151,16 +146,10 @@ public class RoleHierarchyImpl implements RoleHierarchy { } } } - - if (logger.isDebugEnabled()) { - logger.debug("getReachableGrantedAuthorities() - From the roles " + authorities + " one can reach " - + reachableRoles + " in zero or more steps."); - } - - List reachableRoleList = new ArrayList<>(reachableRoles.size()); - reachableRoleList.addAll(reachableRoles); - - return reachableRoleList; + logger.debug(LogMessage.format( + "getReachableGrantedAuthorities() - From the roles %s one can reach %s in zero or more steps.", + authorities, reachableRoles)); + return new ArrayList<>(reachableRoles); } /** @@ -172,11 +161,9 @@ public class RoleHierarchyImpl implements RoleHierarchy { for (String line : this.roleHierarchyStringRepresentation.split("\n")) { // Split on > and trim excessive whitespace String[] roles = line.trim().split("\\s+>\\s+"); - for (int i = 1; i < roles.length; i++) { String higherRole = roles[i - 1]; GrantedAuthority lowerRole = new SimpleGrantedAuthority(roles[i]); - Set rolesReachableInOneStepSet; if (!this.rolesReachableInOneStepMap.containsKey(higherRole)) { rolesReachableInOneStepSet = new HashSet<>(); @@ -186,11 +173,9 @@ public class RoleHierarchyImpl implements RoleHierarchy { rolesReachableInOneStepSet = this.rolesReachableInOneStepMap.get(higherRole); } rolesReachableInOneStepSet.add(lowerRole); - - if (logger.isDebugEnabled()) { - logger.debug("buildRolesReachableInOneStepMap() - From role " + higherRole + " one can reach role " - + lowerRole + " in one step."); - } + logger.debug(LogMessage.format( + "buildRolesReachableInOneStepMap() - From role %s one can reach role %s in one step.", + higherRole, lowerRole)); } } } @@ -207,7 +192,6 @@ public class RoleHierarchyImpl implements RoleHierarchy { for (String roleName : this.rolesReachableInOneStepMap.keySet()) { Set rolesToVisitSet = new HashSet<>(this.rolesReachableInOneStepMap.get(roleName)); Set visitedRolesSet = new HashSet<>(); - while (!rolesToVisitSet.isEmpty()) { // take a role from the rolesToVisit set GrantedAuthority lowerRole = rolesToVisitSet.iterator().next(); @@ -222,9 +206,9 @@ public class RoleHierarchyImpl implements RoleHierarchy { rolesToVisitSet.addAll(this.rolesReachableInOneStepMap.get(lowerRole.getAuthority())); } this.rolesReachableInOneOrMoreStepsMap.put(roleName, visitedRolesSet); - - logger.debug("buildRolesReachableInOneOrMoreStepsMap() - From role " + roleName + " one can reach " - + visitedRolesSet + " in one or more steps."); + logger.debug(LogMessage.format( + "buildRolesReachableInOneOrMoreStepsMap() - From role %s one can reach %s in one or more steps.", + roleName, visitedRolesSet)); } } diff --git a/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyUtils.java b/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyUtils.java index fb4a5562f5..2143632d14 100644 --- a/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyUtils.java +++ b/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyUtils.java @@ -43,28 +43,19 @@ public final class RoleHierarchyUtils { * @return a string representation of a role hierarchy * @throws IllegalArgumentException if roleHierarchyMap is null or empty or if a role * name is null or empty or if an implied role name(s) is null or empty - * */ public static String roleHierarchyFromMap(Map> roleHierarchyMap) { Assert.notEmpty(roleHierarchyMap, "roleHierarchyMap cannot be empty"); - - StringWriter roleHierarchyBuffer = new StringWriter(); - PrintWriter roleHierarchyWriter = new PrintWriter(roleHierarchyBuffer); - - for (Map.Entry> roleHierarchyEntry : roleHierarchyMap.entrySet()) { - String role = roleHierarchyEntry.getKey(); - List impliedRoles = roleHierarchyEntry.getValue(); - + StringWriter result = new StringWriter(); + PrintWriter writer = new PrintWriter(result); + roleHierarchyMap.forEach((role, impliedRoles) -> { Assert.hasLength(role, "role name must be supplied"); Assert.notEmpty(impliedRoles, "implied role name(s) cannot be empty"); - for (String impliedRole : impliedRoles) { - String roleMapping = role + " > " + impliedRole; - roleHierarchyWriter.println(roleMapping); + writer.println(role + " > " + impliedRole); } - } - - return roleHierarchyBuffer.toString(); + }); + return result.toString(); } } diff --git a/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java index 9dc828e31c..d92a5ed500 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java +++ b/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java @@ -30,6 +30,7 @@ import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.context.MessageSource; import org.springframework.context.MessageSourceAware; import org.springframework.context.support.MessageSourceAccessor; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDecisionManager; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; @@ -47,6 +48,7 @@ import org.springframework.security.core.SpringSecurityMessageSource; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; /** * Abstract class that implements security interception for secure objects. @@ -141,120 +143,91 @@ public abstract class AbstractSecurityInterceptor () -> "RunAsManager does not support secure object class: " + getSecureObjectClass()); Assert.isTrue(this.accessDecisionManager.supports(getSecureObjectClass()), () -> "AccessDecisionManager does not support secure object class: " + getSecureObjectClass()); - if (this.afterInvocationManager != null) { Assert.isTrue(this.afterInvocationManager.supports(getSecureObjectClass()), () -> "AfterInvocationManager does not support secure object class: " + getSecureObjectClass()); } - if (this.validateConfigAttributes) { Collection attributeDefs = this.obtainSecurityMetadataSource().getAllConfigAttributes(); - if (attributeDefs == null) { - this.logger.warn( - "Could not validate configuration attributes as the SecurityMetadataSource did not return " - + "any attributes from getAllConfigAttributes()"); + this.logger.warn("Could not validate configuration attributes as the " + + "SecurityMetadataSource did not return any attributes from getAllConfigAttributes()"); return; } - - Set unsupportedAttrs = new HashSet<>(); - - for (ConfigAttribute attr : attributeDefs) { - if (!this.runAsManager.supports(attr) && !this.accessDecisionManager.supports(attr) - && ((this.afterInvocationManager == null) || !this.afterInvocationManager.supports(attr))) { - unsupportedAttrs.add(attr); - } - } - - if (unsupportedAttrs.size() != 0) { - throw new IllegalArgumentException("Unsupported configuration attributes: " + unsupportedAttrs); - } - - this.logger.debug("Validated configuration attributes"); + validateAttributeDefs(attributeDefs); } } + private void validateAttributeDefs(Collection attributeDefs) { + Set unsupportedAttrs = new HashSet<>(); + for (ConfigAttribute attr : attributeDefs) { + if (!this.runAsManager.supports(attr) && !this.accessDecisionManager.supports(attr) + && ((this.afterInvocationManager == null) || !this.afterInvocationManager.supports(attr))) { + unsupportedAttrs.add(attr); + } + } + if (unsupportedAttrs.size() != 0) { + throw new IllegalArgumentException("Unsupported configuration attributes: " + unsupportedAttrs); + } + this.logger.debug("Validated configuration attributes"); + } + protected InterceptorStatusToken beforeInvocation(Object object) { Assert.notNull(object, "Object was null"); - final boolean debug = this.logger.isDebugEnabled(); - if (!getSecureObjectClass().isAssignableFrom(object.getClass())) { throw new IllegalArgumentException("Security invocation attempted for object " + object.getClass().getName() + " but AbstractSecurityInterceptor only configured to support secure objects of type: " + getSecureObjectClass()); } - Collection attributes = this.obtainSecurityMetadataSource().getAttributes(object); - - if (attributes == null || attributes.isEmpty()) { - if (this.rejectPublicInvocations) { - throw new IllegalArgumentException("Secure object invocation " + object - + " was denied as public invocations are not allowed via this interceptor. " - + "This indicates a configuration error because the " - + "rejectPublicInvocations property is set to 'true'"); - } - - if (debug) { - this.logger.debug("Public object - authentication not attempted"); - } - + if (CollectionUtils.isEmpty(attributes)) { + Assert.isTrue(!this.rejectPublicInvocations, + () -> "Secure object invocation " + object + + " was denied as public invocations are not allowed via this interceptor. " + + "This indicates a configuration error because the " + + "rejectPublicInvocations property is set to 'true'"); + this.logger.debug("Public object - authentication not attempted"); publishEvent(new PublicInvocationEvent(object)); - return null; // no further work post-invocation } - - if (debug) { - this.logger.debug("Secure object: " + object + "; Attributes: " + attributes); - } - + this.logger.debug(LogMessage.format("Secure object: %s; Attributes: %s", object, attributes)); if (SecurityContextHolder.getContext().getAuthentication() == null) { credentialsNotFound(this.messages.getMessage("AbstractSecurityInterceptor.authenticationNotFound", "An Authentication object was not found in the SecurityContext"), object, attributes); } - Authentication authenticated = authenticateIfRequired(); - // Attempt authorization - try { - this.accessDecisionManager.decide(authenticated, object, attributes); - } - catch (AccessDeniedException accessDeniedException) { - publishEvent(new AuthorizationFailureEvent(object, attributes, authenticated, accessDeniedException)); - - throw accessDeniedException; - } - - if (debug) { - this.logger.debug("Authorization successful"); - } - + attemptAuthorization(object, attributes, authenticated); + this.logger.debug("Authorization successful"); if (this.publishAuthorizationSuccess) { publishEvent(new AuthorizedEvent(object, attributes, authenticated)); } // Attempt to run as a different user Authentication runAs = this.runAsManager.buildRunAs(authenticated, object, attributes); - - if (runAs == null) { - if (debug) { - this.logger.debug("RunAsManager did not change Authentication object"); - } - - // no further work post-invocation - return new InterceptorStatusToken(SecurityContextHolder.getContext(), false, attributes, object); - } - else { - if (debug) { - this.logger.debug("Switching to RunAs Authentication: " + runAs); - } - + if (runAs != null) { + this.logger.debug(LogMessage.format("Switching to RunAs Authentication: %s", runAs)); SecurityContext origCtx = SecurityContextHolder.getContext(); SecurityContextHolder.setContext(SecurityContextHolder.createEmptyContext()); SecurityContextHolder.getContext().setAuthentication(runAs); - // need to revert to token.Authenticated post-invocation return new InterceptorStatusToken(origCtx, true, attributes, object); } + this.logger.debug("RunAsManager did not change Authentication object"); + // no further work post-invocation + return new InterceptorStatusToken(SecurityContextHolder.getContext(), false, attributes, object); + + } + + private void attemptAuthorization(Object object, Collection attributes, + Authentication authenticated) { + try { + this.accessDecisionManager.decide(authenticated, object, attributes); + } + catch (AccessDeniedException ex) { + publishEvent(new AuthorizationFailureEvent(object, attributes, authenticated, ex)); + throw ex; + } } /** @@ -266,11 +239,8 @@ public abstract class AbstractSecurityInterceptor */ protected void finallyInvocation(InterceptorStatusToken token) { if (token != null && token.isContextHolderRefreshRequired()) { - if (this.logger.isDebugEnabled()) { - this.logger.debug( - "Reverting to original Authentication: " + token.getSecurityContext().getAuthentication()); - } - + this.logger.debug(LogMessage.of( + () -> "Reverting to original Authentication: " + token.getSecurityContext().getAuthentication())); SecurityContextHolder.setContext(token.getSecurityContext()); } } @@ -289,24 +259,19 @@ public abstract class AbstractSecurityInterceptor // public object return returnedObject; } - finallyInvocation(token); // continue to clean in this method for passivity - if (this.afterInvocationManager != null) { // Attempt after invocation handling try { returnedObject = this.afterInvocationManager.decide(token.getSecurityContext().getAuthentication(), token.getSecureObject(), token.getAttributes(), returnedObject); } - catch (AccessDeniedException accessDeniedException) { - AuthorizationFailureEvent event = new AuthorizationFailureEvent(token.getSecureObject(), - token.getAttributes(), token.getSecurityContext().getAuthentication(), accessDeniedException); - publishEvent(event); - - throw accessDeniedException; + catch (AccessDeniedException ex) { + publishEvent(new AuthorizationFailureEvent(token.getSecureObject(), token.getAttributes(), + token.getSecurityContext().getAuthentication(), ex)); + throw ex; } } - return returnedObject; } @@ -318,25 +283,14 @@ public abstract class AbstractSecurityInterceptor */ private Authentication authenticateIfRequired() { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); - if (authentication.isAuthenticated() && !this.alwaysReauthenticate) { - if (this.logger.isDebugEnabled()) { - this.logger.debug("Previously Authenticated: " + authentication); - } - + this.logger.debug(LogMessage.format("Previously Authenticated: %s", authentication)); return authentication; } - authentication = this.authenticationManager.authenticate(authentication); - - // We don't authenticated.setAuthentication(true), because each provider should do - // that - if (this.logger.isDebugEnabled()) { - this.logger.debug("Successfully Authenticated: " + authentication); - } - + // Don't authenticated.setAuthentication(true) because each provider does that + this.logger.debug(LogMessage.format("Successfully Authenticated: %s", authentication)); SecurityContextHolder.getContext().setAuthentication(authentication); - return authentication; } @@ -351,11 +305,9 @@ public abstract class AbstractSecurityInterceptor */ private void credentialsNotFound(String reason, Object secureObject, Collection configAttribs) { AuthenticationCredentialsNotFoundException exception = new AuthenticationCredentialsNotFoundException(reason); - AuthenticationCredentialsNotFoundEvent event = new AuthenticationCredentialsNotFoundEvent(secureObject, configAttribs, exception); publishEvent(event); - throw exception; } diff --git a/core/src/main/java/org/springframework/security/access/intercept/AfterInvocationProviderManager.java b/core/src/main/java/org/springframework/security/access/intercept/AfterInvocationProviderManager.java index 26626f006b..44d42d850a 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/AfterInvocationProviderManager.java +++ b/core/src/main/java/org/springframework/security/access/intercept/AfterInvocationProviderManager.java @@ -24,11 +24,13 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.InitializingBean; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.AfterInvocationProvider; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.core.Authentication; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; /** * Provider-based implementation of {@link AfterInvocationManager}. @@ -57,22 +59,13 @@ public class AfterInvocationProviderManager implements AfterInvocationManager, I checkIfValidList(this.providers); } - private void checkIfValidList(List listToCheck) { - if ((listToCheck == null) || (listToCheck.size() == 0)) { - throw new IllegalArgumentException("A list of AfterInvocationProviders is required"); - } - } - @Override public Object decide(Authentication authentication, Object object, Collection config, Object returnedObject) throws AccessDeniedException { - Object result = returnedObject; - for (AfterInvocationProvider provider : this.providers) { result = provider.decide(authentication, object, config, result); } - return result; } @@ -83,7 +76,6 @@ public class AfterInvocationProviderManager implements AfterInvocationManager, I public void setProviders(List newList) { checkIfValidList(newList); this.providers = new ArrayList<>(newList.size()); - for (Object currentObject : newList) { Assert.isInstanceOf(AfterInvocationProvider.class, currentObject, () -> "AfterInvocationProvider " + currentObject.getClass().getName() + " must implement AfterInvocationProvider"); @@ -91,18 +83,18 @@ public class AfterInvocationProviderManager implements AfterInvocationManager, I } } + private void checkIfValidList(List listToCheck) { + Assert.isTrue(!CollectionUtils.isEmpty(listToCheck), "A list of AfterInvocationProviders is required"); + } + @Override public boolean supports(ConfigAttribute attribute) { for (AfterInvocationProvider provider : this.providers) { - if (logger.isDebugEnabled()) { - logger.debug("Evaluating " + attribute + " against " + provider); - } - + logger.debug(LogMessage.format("Evaluating %s against %s", attribute, provider)); if (provider.supports(attribute)) { return true; } } - return false; } @@ -124,7 +116,6 @@ public class AfterInvocationProviderManager implements AfterInvocationManager, I return false; } } - return true; } diff --git a/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java b/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java index 131149429d..95b98e2622 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java +++ b/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java @@ -23,6 +23,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.InitializingBean; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.core.Authentication; @@ -54,36 +55,25 @@ public class MethodInvocationPrivilegeEvaluator implements InitializingBean { Assert.notNull(this.securityInterceptor, "SecurityInterceptor required"); } - public boolean isAllowed(MethodInvocation mi, Authentication authentication) { - Assert.notNull(mi, "MethodInvocation required"); - Assert.notNull(mi.getMethod(), "MethodInvocation must provide a non-null getMethod()"); - - Collection attrs = this.securityInterceptor.obtainSecurityMetadataSource().getAttributes(mi); - + public boolean isAllowed(MethodInvocation invocation, Authentication authentication) { + Assert.notNull(invocation, "MethodInvocation required"); + Assert.notNull(invocation.getMethod(), "MethodInvocation must provide a non-null getMethod()"); + Collection attrs = this.securityInterceptor.obtainSecurityMetadataSource() + .getAttributes(invocation); if (attrs == null) { - if (this.securityInterceptor.isRejectPublicInvocations()) { - return false; - } - - return true; + return !this.securityInterceptor.isRejectPublicInvocations(); } - if (authentication == null || authentication.getAuthorities().isEmpty()) { return false; } - try { - this.securityInterceptor.getAccessDecisionManager().decide(authentication, mi, attrs); + this.securityInterceptor.getAccessDecisionManager().decide(authentication, invocation, attrs); + return true; } catch (AccessDeniedException unauthorized) { - if (logger.isDebugEnabled()) { - logger.debug(mi.toString() + " denied for " + authentication.toString(), unauthorized); - } - + logger.debug(LogMessage.format("%s denied for %s", invocation, authentication), unauthorized); return false; } - - return true; } public void setSecurityInterceptor(AbstractSecurityInterceptor securityInterceptor) { diff --git a/core/src/main/java/org/springframework/security/access/intercept/RunAsImplAuthenticationProvider.java b/core/src/main/java/org/springframework/security/access/intercept/RunAsImplAuthenticationProvider.java index 274d20b2c3..eb0fa743aa 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/RunAsImplAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/access/intercept/RunAsImplAuthenticationProvider.java @@ -54,14 +54,11 @@ public class RunAsImplAuthenticationProvider implements InitializingBean, Authen @Override public Authentication authenticate(Authentication authentication) throws AuthenticationException { RunAsUserToken token = (RunAsUserToken) authentication; - - if (token.getKeyHash() == this.key.hashCode()) { - return authentication; - } - else { + if (token.getKeyHash() != this.key.hashCode()) { throw new BadCredentialsException(this.messages.getMessage("RunAsImplAuthenticationProvider.incorrectKey", "The presented RunAsUserToken does not contain the expected key")); } + return authentication; } public String getKey() { diff --git a/core/src/main/java/org/springframework/security/access/intercept/RunAsManagerImpl.java b/core/src/main/java/org/springframework/security/access/intercept/RunAsManagerImpl.java index 144af06954..352ff66f9f 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/RunAsManagerImpl.java +++ b/core/src/main/java/org/springframework/security/access/intercept/RunAsManagerImpl.java @@ -69,7 +69,6 @@ public class RunAsManagerImpl implements RunAsManager, InitializingBean { public Authentication buildRunAs(Authentication authentication, Object object, Collection attributes) { List newAuthorities = new ArrayList<>(); - for (ConfigAttribute attribute : attributes) { if (this.supports(attribute)) { GrantedAuthority extraAuthority = new SimpleGrantedAuthority( @@ -77,14 +76,11 @@ public class RunAsManagerImpl implements RunAsManager, InitializingBean { newAuthorities.add(extraAuthority); } } - if (newAuthorities.size() == 0) { return null; } - // Add existing authorities newAuthorities.addAll(authentication.getAuthorities()); - return new RunAsUserToken(this.key, authentication.getPrincipal(), authentication.getCredentials(), newAuthorities, authentication.getClass()); } diff --git a/core/src/main/java/org/springframework/security/access/intercept/RunAsUserToken.java b/core/src/main/java/org/springframework/security/access/intercept/RunAsUserToken.java index 31b145f84c..1019947440 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/RunAsUserToken.java +++ b/core/src/main/java/org/springframework/security/access/intercept/RunAsUserToken.java @@ -75,7 +75,6 @@ public class RunAsUserToken extends AbstractAuthenticationToken { StringBuilder sb = new StringBuilder(super.toString()); String className = (this.originalAuthentication != null) ? this.originalAuthentication.getName() : null; sb.append("; Original Class: ").append(className); - return sb.toString(); } diff --git a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java index 42e1af08d1..fadf5baf42 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java +++ b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java @@ -56,7 +56,6 @@ public class MethodSecurityInterceptor extends AbstractSecurityInterceptor imple @Override public Object invoke(MethodInvocation mi) throws Throwable { InterceptorStatusToken token = super.beforeInvocation(mi); - Object result; try { result = mi.proceed(); diff --git a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java index 89d04e208a..1d8902d82c 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java +++ b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.io.ObjectInputStream; import java.io.Serializable; import java.lang.reflect.Method; -import java.util.Collection; import org.aopalliance.aop.Advice; import org.aopalliance.intercept.MethodInterceptor; @@ -33,6 +32,7 @@ import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.BeanFactoryAware; import org.springframework.security.access.method.MethodSecurityMetadataSource; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; /** * Advisor driven by a {@link MethodSecurityMetadataSource}, used to exclude a @@ -85,7 +85,6 @@ public class MethodSecurityMetadataSourceAdvisor extends AbstractPointcutAdvisor Assert.notNull(adviceBeanName, "The adviceBeanName cannot be null"); Assert.notNull(attributeSource, "The attributeSource cannot be null"); Assert.notNull(attributeSourceBeanName, "The attributeSourceBeanName cannot be null"); - this.adviceBeanName = adviceBeanName; this.attributeSource = attributeSource; this.metadataSourceBeanName = attributeSourceBeanName; @@ -123,11 +122,9 @@ public class MethodSecurityMetadataSourceAdvisor extends AbstractPointcutAdvisor class MethodSecurityMetadataSourcePointcut extends StaticMethodMatcherPointcut implements Serializable { @Override - @SuppressWarnings("unchecked") - public boolean matches(Method m, Class targetClass) { - Collection attributes = MethodSecurityMetadataSourceAdvisor.this.attributeSource.getAttributes(m, - targetClass); - return attributes != null && !attributes.isEmpty(); + public boolean matches(Method m, Class targetClass) { + MethodSecurityMetadataSource source = MethodSecurityMetadataSourceAdvisor.this.attributeSource; + return !CollectionUtils.isEmpty(source.getAttributes(m, targetClass)); } } diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java index 99c63eb8f4..78e450c4d1 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java +++ b/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java @@ -55,7 +55,6 @@ public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterc */ public Object invoke(JoinPoint jp, AspectJCallback advisorProceed) { InterceptorStatusToken token = super.beforeInvocation(new MethodInvocationAdapter(jp)); - Object result; try { result = advisorProceed.proceedWithObject(); @@ -63,7 +62,6 @@ public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterc finally { super.finallyInvocation(token); } - return super.afterInvocation(token, result); } diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java index 041efdc311..7030f99ee4 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java +++ b/core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java @@ -24,6 +24,8 @@ import org.aspectj.lang.JoinPoint; import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.reflect.CodeSignature; +import org.springframework.util.Assert; + /** * Decorates a JoinPoint to allow it to be used with method-security infrastructure * classes which support {@code MethodInvocation} instances. @@ -51,23 +53,17 @@ public final class MethodInvocationAdapter implements MethodInvocation { String targetMethodName = jp.getStaticPart().getSignature().getName(); Class[] types = ((CodeSignature) jp.getStaticPart().getSignature()).getParameterTypes(); Class declaringType = jp.getStaticPart().getSignature().getDeclaringType(); - this.method = findMethod(targetMethodName, declaringType, types); - - if (this.method == null) { - throw new IllegalArgumentException("Could not obtain target method from JoinPoint: '" + jp + "'"); - } + Assert.notNull(this.method, () -> "Could not obtain target method from JoinPoint: '" + jp + "'"); } private Method findMethod(String name, Class declaringType, Class[] params) { Method method = null; - try { method = declaringType.getMethod(name, params); } catch (NoSuchMethodException ignored) { } - if (method == null) { try { method = declaringType.getDeclaredMethod(name, params); @@ -75,7 +71,6 @@ public final class MethodInvocationAdapter implements MethodInvocation { catch (NoSuchMethodException ignored) { } } - return method; } diff --git a/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java index 2b5a9366c8..3b3be3effb 100644 --- a/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java @@ -57,13 +57,11 @@ public abstract class AbstractFallbackMethodSecurityMetadataSource extends Abstr if (attr != null) { return attr; } - // Second try is the config attribute on the target class. attr = findAttributes(specificMethod.getDeclaringClass()); if (attr != null) { return attr; } - if (specificMethod != method || targetClass == null) { // Fallback is to look at the original method. attr = findAttributes(method, method.getDeclaringClass()); diff --git a/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java index 9fc0775825..3ed17acc70 100644 --- a/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java @@ -42,7 +42,6 @@ public abstract class AbstractMethodSecurityMetadataSource implements MethodSecu MethodInvocation mi = (MethodInvocation) object; Object target = mi.getThis(); Class targetClass = null; - if (target != null) { targetClass = (target instanceof Class) ? (Class) target : AopProxyUtils.ultimateTargetClass(target); @@ -56,7 +55,6 @@ public abstract class AbstractMethodSecurityMetadataSource implements MethodSecu } return attrs; } - throw new IllegalArgumentException("Object must be a non-null MethodInvocation"); } diff --git a/core/src/main/java/org/springframework/security/access/method/DelegatingMethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/method/DelegatingMethodSecurityMetadataSource.java index 583d140b31..2591d099fc 100644 --- a/core/src/main/java/org/springframework/security/access/method/DelegatingMethodSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/method/DelegatingMethodSecurityMetadataSource.java @@ -25,6 +25,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.ConfigAttribute; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; @@ -56,11 +57,9 @@ public final class DelegatingMethodSecurityMetadataSource extends AbstractMethod synchronized (this.attributeCache) { Collection cached = this.attributeCache.get(cacheKey); // Check for canonical value indicating there is no config attribute, - if (cached != null) { return cached; } - // No cached value, so query the sources to find a result Collection attributes = null; for (MethodSecurityMetadataSource s : this.methodSecurityMetadataSources) { @@ -69,19 +68,13 @@ public final class DelegatingMethodSecurityMetadataSource extends AbstractMethod break; } } - // Put it in the cache. if (attributes == null || attributes.isEmpty()) { this.attributeCache.put(cacheKey, NULL_CONFIG_ATTRIBUTE); return NULL_CONFIG_ATTRIBUTE; } - - if (this.logger.isDebugEnabled()) { - this.logger.debug("Caching method [" + cacheKey + "] with attributes " + attributes); - } - + this.logger.debug(LogMessage.format("Caching method [%s] with attributes %s", cacheKey, attributes)); this.attributeCache.put(cacheKey, attributes); - return attributes; } } @@ -127,8 +120,8 @@ public final class DelegatingMethodSecurityMetadataSource extends AbstractMethod @Override public String toString() { - return "CacheKey[" + ((this.targetClass != null) ? this.targetClass.getName() : "-") + "; " + this.method - + "]"; + String targetClassName = (this.targetClass != null) ? this.targetClass.getName() : "-"; + return "CacheKey[" + targetClassName + "; " + this.method + "]"; } } diff --git a/core/src/main/java/org/springframework/security/access/method/MapBasedMethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/method/MapBasedMethodSecurityMetadataSource.java index eb7a41816a..4fb00cc78e 100644 --- a/core/src/main/java/org/springframework/security/access/method/MapBasedMethodSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/method/MapBasedMethodSecurityMetadataSource.java @@ -26,6 +26,7 @@ import java.util.Map; import java.util.Set; import org.springframework.beans.factory.BeanClassLoaderAware; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.ConfigAttribute; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -47,10 +48,14 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod private ClassLoader beanClassLoader = ClassUtils.getDefaultClassLoader(); - /** Map from RegisteredMethod to ConfigAttribute list */ + /** + * Map from RegisteredMethod to ConfigAttribute list + */ protected final Map> methodMap = new HashMap<>(); - /** Map from RegisteredMethod to name pattern used for registration */ + /** + * Map from RegisteredMethod to name pattern used for registration + */ private final Map nameMap = new HashMap<>(); public MapBasedMethodSecurityMetadataSource() { @@ -83,7 +88,6 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod if (targetClass == null) { return null; } - return findAttributesSpecifiedAgainst(method, targetClass); } @@ -107,17 +111,11 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod */ private void addSecureMethod(String name, List attr) { int lastDotIndex = name.lastIndexOf("."); - - if (lastDotIndex == -1) { - throw new IllegalArgumentException("'" + name + "' is not a valid method name: format is FQN.methodName"); - } - + Assert.isTrue(lastDotIndex != -1, () -> "'" + name + "' is not a valid method name: format is FQN.methodName"); String methodName = name.substring(lastDotIndex + 1); Assert.hasText(methodName, () -> "Method not found for '" + name + "'"); - String typeName = name.substring(0, lastDotIndex); Class type = ClassUtils.resolveClassName(typeName, this.beanClassLoader); - addSecureMethod(type, methodName, attr); } @@ -131,43 +129,38 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod */ public void addSecureMethod(Class javaType, String mappedName, List attr) { String name = javaType.getName() + '.' + mappedName; - - if (this.logger.isDebugEnabled()) { - this.logger.debug("Request to add secure method [" + name + "] with attributes [" + attr + "]"); - } - + this.logger.debug(LogMessage.format("Request to add secure method [%s] with attributes [%s]", name, attr)); Method[] methods = javaType.getMethods(); List matchingMethods = new ArrayList<>(); - - for (Method m : methods) { - if (m.getName().equals(mappedName) || isMatch(m.getName(), mappedName)) { - matchingMethods.add(m); + for (Method method : methods) { + if (method.getName().equals(mappedName) || isMatch(method.getName(), mappedName)) { + matchingMethods.add(method); } } + Assert.notEmpty(matchingMethods, () -> "Couldn't find method '" + mappedName + "' on '" + javaType + "'"); + registerAllMatchingMethods(javaType, attr, name, matchingMethods); + } - if (matchingMethods.isEmpty()) { - throw new IllegalArgumentException("Couldn't find method '" + mappedName + "' on '" + javaType + "'"); - } - - // register all matching methods + private void registerAllMatchingMethods(Class javaType, List attr, String name, + List matchingMethods) { for (Method method : matchingMethods) { RegisteredMethod registeredMethod = new RegisteredMethod(method, javaType); String regMethodName = this.nameMap.get(registeredMethod); - if ((regMethodName == null) || (!regMethodName.equals(name) && (regMethodName.length() <= name.length()))) { // no already registered method name, or more specific // method name specification (now) -> (re-)register method if (regMethodName != null) { - this.logger.debug("Replacing attributes for secure method [" + method + "]: current name [" + name - + "] is more specific than [" + regMethodName + "]"); + this.logger.debug(LogMessage.format( + "Replacing attributes for secure method [%s]: current name [%s] is more specific than [%s]", + method, name, regMethodName)); } - this.nameMap.put(registeredMethod, name); addSecureMethod(registeredMethod, attr); } else { - this.logger.debug("Keeping attributes for secure method [" + method + "]: current name [" + name - + "] is not more specific than [" + regMethodName + "]"); + this.logger.debug(LogMessage.format( + "Keeping attributes for secure method [%s]: current name [%s] is not more specific than [%s]", + method, name, regMethodName)); } } } @@ -183,13 +176,11 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod */ public void addSecureMethod(Class javaType, Method method, List attr) { RegisteredMethod key = new RegisteredMethod(method, javaType); - if (this.methodMap.containsKey(key)) { - this.logger.debug( - "Method [" + method + "] is already registered with attributes [" + this.methodMap.get(key) + "]"); + this.logger.debug(LogMessage.format("Method [%s] is already registered with attributes [%s]", method, + this.methodMap.get(key))); return; } - this.methodMap.put(key, attr); } @@ -201,9 +192,7 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod private void addSecureMethod(RegisteredMethod method, List attr) { Assert.notNull(method, "RegisteredMethod required"); Assert.notNull(attr, "Configuration attribute required"); - if (this.logger.isInfoEnabled()) { - this.logger.info("Adding secure method [" + method + "] with attributes [" + attr + "]"); - } + this.logger.info(LogMessage.format("Adding secure method [%s] with attributes [%s]", method, attr)); this.methodMap.put(method, attr); } @@ -214,11 +203,7 @@ public class MapBasedMethodSecurityMetadataSource extends AbstractFallbackMethod @Override public Collection getAllConfigAttributes() { Set allAttributes = new HashSet<>(); - - for (List attributeList : this.methodMap.values()) { - allAttributes.addAll(attributeList); - } - + this.methodMap.values().forEach(allAttributes::addAll); return allAttributes; } diff --git a/core/src/main/java/org/springframework/security/access/prepost/PostInvocationAdviceProvider.java b/core/src/main/java/org/springframework/security/access/prepost/PostInvocationAdviceProvider.java index ff13589249..b63a39e8c7 100644 --- a/core/src/main/java/org/springframework/security/access/prepost/PostInvocationAdviceProvider.java +++ b/core/src/main/java/org/springframework/security/access/prepost/PostInvocationAdviceProvider.java @@ -49,14 +49,12 @@ public class PostInvocationAdviceProvider implements AfterInvocationProvider { @Override public Object decide(Authentication authentication, Object object, Collection config, Object returnedObject) throws AccessDeniedException { - - PostInvocationAttribute pia = findPostInvocationAttribute(config); - - if (pia == null) { + PostInvocationAttribute postInvocationAttribute = findPostInvocationAttribute(config); + if (postInvocationAttribute == null) { return returnedObject; } - - return this.postAdvice.after(authentication, (MethodInvocation) object, pia, returnedObject); + return this.postAdvice.after(authentication, (MethodInvocation) object, postInvocationAttribute, + returnedObject); } private PostInvocationAttribute findPostInvocationAttribute(Collection config) { @@ -65,7 +63,6 @@ public class PostInvocationAdviceProvider implements AfterInvocationProvider { return (PostInvocationAttribute) attribute; } } - return null; } diff --git a/core/src/main/java/org/springframework/security/access/prepost/PreInvocationAuthorizationAdviceVoter.java b/core/src/main/java/org/springframework/security/access/prepost/PreInvocationAuthorizationAdviceVoter.java index 5d1a4594a8..2b9979290a 100644 --- a/core/src/main/java/org/springframework/security/access/prepost/PreInvocationAuthorizationAdviceVoter.java +++ b/core/src/main/java/org/springframework/security/access/prepost/PreInvocationAuthorizationAdviceVoter.java @@ -61,21 +61,14 @@ public class PreInvocationAuthorizationAdviceVoter implements AccessDecisionVote @Override public int vote(Authentication authentication, MethodInvocation method, Collection attributes) { - // Find prefilter and preauth (or combined) attributes - // if both null, abstain - // else call advice with them - + // if both null, abstain else call advice with them PreInvocationAttribute preAttr = findPreInvocationAttribute(attributes); - if (preAttr == null) { // No expression based metadata, so abstain return ACCESS_ABSTAIN; } - - boolean allowed = this.preAdvice.before(authentication, method, preAttr); - - return allowed ? ACCESS_GRANTED : ACCESS_DENIED; + return this.preAdvice.before(authentication, method, preAttr) ? ACCESS_GRANTED : ACCESS_DENIED; } private PreInvocationAttribute findPreInvocationAttribute(Collection config) { @@ -84,7 +77,6 @@ public class PreInvocationAuthorizationAdviceVoter implements AccessDecisionVote return (PreInvocationAttribute) attribute; } } - return null; } diff --git a/core/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java b/core/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java index 0fc6e502a2..b56a755edb 100644 --- a/core/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java @@ -66,7 +66,6 @@ public class PrePostAdviceReactiveMethodInterceptor implements MethodInterceptor Assert.notNull(attributeSource, "attributeSource cannot be null"); Assert.notNull(preInvocationAdvice, "preInvocationAdvice cannot be null"); Assert.notNull(postInvocationAdvice, "postInvocationAdvice cannot be null"); - this.attributeSource = attributeSource; this.preInvocationAdvice = preInvocationAdvice; this.postAdvice = postInvocationAdvice; @@ -76,31 +75,26 @@ public class PrePostAdviceReactiveMethodInterceptor implements MethodInterceptor public Object invoke(final MethodInvocation invocation) { Method method = invocation.getMethod(); Class returnType = method.getReturnType(); - if (!Publisher.class.isAssignableFrom(returnType)) { - throw new IllegalStateException("The returnType " + returnType + " on " + method - + " must return an instance of org.reactivestreams.Publisher (i.e. Mono / Flux) in order to support Reactor Context"); - } + Assert.state(Publisher.class.isAssignableFrom(returnType), + () -> "The returnType " + returnType + " on " + method + + " must return an instance of org.reactivestreams.Publisher " + + "(i.e. Mono / Flux) in order to support Reactor Context"); Class targetClass = invocation.getThis().getClass(); Collection attributes = this.attributeSource.getAttributes(method, targetClass); - PreInvocationAttribute preAttr = findPreInvocationAttribute(attributes); Mono toInvoke = ReactiveSecurityContextHolder.getContext() .map(SecurityContext::getAuthentication).defaultIfEmpty(this.anonymous) .filter((auth) -> this.preInvocationAdvice.before(auth, invocation, preAttr)) .switchIfEmpty(Mono.defer(() -> Mono.error(new AccessDeniedException("Denied")))); - PostInvocationAttribute attr = findPostInvocationAttribute(attributes); - if (Mono.class.isAssignableFrom(returnType)) { return toInvoke.flatMap((auth) -> PrePostAdviceReactiveMethodInterceptor.>proceed(invocation) .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); } - if (Flux.class.isAssignableFrom(returnType)) { return toInvoke.flatMapMany((auth) -> PrePostAdviceReactiveMethodInterceptor.>proceed(invocation) .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); } - return toInvoke.flatMapMany( (auth) -> Flux.from(PrePostAdviceReactiveMethodInterceptor.>proceed(invocation)) .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); @@ -121,7 +115,6 @@ public class PrePostAdviceReactiveMethodInterceptor implements MethodInterceptor return (PostInvocationAttribute) attribute; } } - return null; } @@ -131,7 +124,6 @@ public class PrePostAdviceReactiveMethodInterceptor implements MethodInterceptor return (PreInvocationAttribute) attribute; } } - return null; } diff --git a/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java index ff12befce0..8f85e8ad78 100644 --- a/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java @@ -23,6 +23,7 @@ import java.util.Collection; import java.util.Collections; import org.springframework.core.annotation.AnnotationUtils; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.access.method.AbstractMethodSecurityMetadataSource; import org.springframework.util.ClassUtils; @@ -61,45 +62,35 @@ public class PrePostAnnotationSecurityMetadataSource extends AbstractMethodSecur if (method.getDeclaringClass() == Object.class) { return Collections.emptyList(); } - - this.logger.trace("Looking for Pre/Post annotations for method '" + method.getName() + "' on target class '" - + targetClass + "'"); + this.logger.trace(LogMessage.format("Looking for Pre/Post annotations for method '%s' on target class '%s'", + method.getName(), targetClass)); PreFilter preFilter = findAnnotation(method, targetClass, PreFilter.class); PreAuthorize preAuthorize = findAnnotation(method, targetClass, PreAuthorize.class); PostFilter postFilter = findAnnotation(method, targetClass, PostFilter.class); // TODO: Can we check for void methods and throw an exception here? PostAuthorize postAuthorize = findAnnotation(method, targetClass, PostAuthorize.class); - if (preFilter == null && preAuthorize == null && postFilter == null && postAuthorize == null) { // There is no meta-data so return this.logger.trace("No expression annotations found"); return Collections.emptyList(); } - String preFilterAttribute = (preFilter != null) ? preFilter.value() : null; String filterObject = (preFilter != null) ? preFilter.filterTarget() : null; String preAuthorizeAttribute = (preAuthorize != null) ? preAuthorize.value() : null; String postFilterAttribute = (postFilter != null) ? postFilter.value() : null; String postAuthorizeAttribute = (postAuthorize != null) ? postAuthorize.value() : null; - ArrayList attrs = new ArrayList<>(2); - PreInvocationAttribute pre = this.attributeFactory.createPreInvocationAttribute(preFilterAttribute, filterObject, preAuthorizeAttribute); - if (pre != null) { attrs.add(pre); } - PostInvocationAttribute post = this.attributeFactory.createPostInvocationAttribute(postFilterAttribute, postAuthorizeAttribute); - if (post != null) { attrs.add(post); } - attrs.trimToSize(); - return attrs; } @@ -120,31 +111,26 @@ public class PrePostAnnotationSecurityMetadataSource extends AbstractMethodSecur // If the target class is null, the method will be unchanged. Method specificMethod = ClassUtils.getMostSpecificMethod(method, targetClass); A annotation = AnnotationUtils.findAnnotation(specificMethod, annotationClass); - if (annotation != null) { - this.logger.debug(annotation + " found on specific method: " + specificMethod); + this.logger.debug(LogMessage.format("%s found on specific method: %s", annotation, specificMethod)); return annotation; } - // Check the original (e.g. interface) method if (specificMethod != method) { annotation = AnnotationUtils.findAnnotation(method, annotationClass); - if (annotation != null) { - this.logger.debug(annotation + " found on: " + method); + this.logger.debug(LogMessage.format("%s found on: %s", annotation, method)); return annotation; } } - // Check the class-level (note declaringClass, not targetClass, which may not // actually implement the method) annotation = AnnotationUtils.findAnnotation(specificMethod.getDeclaringClass(), annotationClass); - if (annotation != null) { - this.logger.debug(annotation + " found on: " + specificMethod.getDeclaringClass().getName()); + this.logger.debug( + LogMessage.format("%s found on: %s", annotation, specificMethod.getDeclaringClass().getName())); return annotation; } - return null; } diff --git a/core/src/main/java/org/springframework/security/access/vote/AbstractAccessDecisionManager.java b/core/src/main/java/org/springframework/security/access/vote/AbstractAccessDecisionManager.java index d603c6183e..50d835df37 100644 --- a/core/src/main/java/org/springframework/security/access/vote/AbstractAccessDecisionManager.java +++ b/core/src/main/java/org/springframework/security/access/vote/AbstractAccessDecisionManager.java @@ -88,12 +88,11 @@ public abstract class AbstractAccessDecisionManager @Override public boolean supports(ConfigAttribute attribute) { - for (AccessDecisionVoter voter : this.decisionVoters) { + for (AccessDecisionVoter voter : this.decisionVoters) { if (voter.supports(attribute)) { return true; } } - return false; } @@ -108,12 +107,11 @@ public abstract class AbstractAccessDecisionManager */ @Override public boolean supports(Class clazz) { - for (AccessDecisionVoter voter : this.decisionVoters) { + for (AccessDecisionVoter voter : this.decisionVoters) { if (!voter.supports(clazz)) { return false; } } - return true; } diff --git a/core/src/main/java/org/springframework/security/access/vote/AbstractAclVoter.java b/core/src/main/java/org/springframework/security/access/vote/AbstractAclVoter.java index 54257496a0..5e1fd32180 100644 --- a/core/src/main/java/org/springframework/security/access/vote/AbstractAclVoter.java +++ b/core/src/main/java/org/springframework/security/access/vote/AbstractAclVoter.java @@ -33,18 +33,13 @@ public abstract class AbstractAclVoter implements AccessDecisionVoter processDomainObjectClass; protected Object getDomainObjectInstance(MethodInvocation invocation) { - Object[] args; - Class[] params; - - params = invocation.getMethod().getParameterTypes(); - args = invocation.getArguments(); - + Object[] args = invocation.getArguments(); + Class[] params = invocation.getMethod().getParameterTypes(); for (int i = 0; i < params.length; i++) { if (this.processDomainObjectClass.isAssignableFrom(params[i])) { return args[i]; } } - throw new AuthorizationServiceException("MethodInvocation: " + invocation + " did not provide any argument of type: " + this.processDomainObjectClass); } diff --git a/core/src/main/java/org/springframework/security/access/vote/AffirmativeBased.java b/core/src/main/java/org/springframework/security/access/vote/AffirmativeBased.java index 1ddc9157c8..718ed9f15f 100644 --- a/core/src/main/java/org/springframework/security/access/vote/AffirmativeBased.java +++ b/core/src/main/java/org/springframework/security/access/vote/AffirmativeBased.java @@ -19,6 +19,7 @@ package org.springframework.security.access.vote; import java.util.Collection; import java.util.List; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDecisionVoter; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; @@ -52,36 +53,27 @@ public class AffirmativeBased extends AbstractAccessDecisionManager { * @throws AccessDeniedException if access is denied */ @Override + @SuppressWarnings({ "rawtypes", "unchecked" }) public void decide(Authentication authentication, Object object, Collection configAttributes) throws AccessDeniedException { int deny = 0; - for (AccessDecisionVoter voter : getDecisionVoters()) { int result = voter.vote(authentication, object, configAttributes); - - if (this.logger.isDebugEnabled()) { - this.logger.debug("Voter: " + voter + ", returned: " + result); - } - + this.logger.debug(LogMessage.format("Voter: %s, returned: %s", voter, result)); switch (result) { case AccessDecisionVoter.ACCESS_GRANTED: return; - case AccessDecisionVoter.ACCESS_DENIED: deny++; - break; - default: break; } } - if (deny > 0) { throw new AccessDeniedException( this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access is denied")); } - // To get this far, every AccessDecisionVoter abstained checkAllowIfAllAbstainDecisions(); } diff --git a/core/src/main/java/org/springframework/security/access/vote/AuthenticatedVoter.java b/core/src/main/java/org/springframework/security/access/vote/AuthenticatedVoter.java index 364485b8ec..eec33f2d53 100644 --- a/core/src/main/java/org/springframework/security/access/vote/AuthenticatedVoter.java +++ b/core/src/main/java/org/springframework/security/access/vote/AuthenticatedVoter.java @@ -87,24 +87,20 @@ public class AuthenticatedVoter implements AccessDecisionVoter { @Override public int vote(Authentication authentication, Object object, Collection attributes) { int result = ACCESS_ABSTAIN; - for (ConfigAttribute attribute : attributes) { if (this.supports(attribute)) { result = ACCESS_DENIED; - if (IS_AUTHENTICATED_FULLY.equals(attribute.getAttribute())) { if (isFullyAuthenticated(authentication)) { return ACCESS_GRANTED; } } - if (IS_AUTHENTICATED_REMEMBERED.equals(attribute.getAttribute())) { if (this.authenticationTrustResolver.isRememberMe(authentication) || isFullyAuthenticated(authentication)) { return ACCESS_GRANTED; } } - if (IS_AUTHENTICATED_ANONYMOUSLY.equals(attribute.getAttribute())) { if (this.authenticationTrustResolver.isAnonymous(authentication) || isFullyAuthenticated(authentication) @@ -114,7 +110,6 @@ public class AuthenticatedVoter implements AccessDecisionVoter { } } } - return result; } diff --git a/core/src/main/java/org/springframework/security/access/vote/ConsensusBased.java b/core/src/main/java/org/springframework/security/access/vote/ConsensusBased.java index 2bd16b2930..3a98a61a49 100644 --- a/core/src/main/java/org/springframework/security/access/vote/ConsensusBased.java +++ b/core/src/main/java/org/springframework/security/access/vote/ConsensusBased.java @@ -19,6 +19,7 @@ package org.springframework.security.access.vote; import java.util.Collection; import java.util.List; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDecisionVoter; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; @@ -59,53 +60,39 @@ public class ConsensusBased extends AbstractAccessDecisionManager { * @throws AccessDeniedException if access is denied */ @Override + @SuppressWarnings({ "rawtypes", "unchecked" }) public void decide(Authentication authentication, Object object, Collection configAttributes) throws AccessDeniedException { int grant = 0; int deny = 0; - for (AccessDecisionVoter voter : getDecisionVoters()) { int result = voter.vote(authentication, object, configAttributes); - - if (this.logger.isDebugEnabled()) { - this.logger.debug("Voter: " + voter + ", returned: " + result); - } - + this.logger.debug(LogMessage.format("Voter: %s, returned: %s", voter, result)); switch (result) { case AccessDecisionVoter.ACCESS_GRANTED: grant++; - break; - case AccessDecisionVoter.ACCESS_DENIED: deny++; - break; - default: break; } } - if (grant > deny) { return; } - if (deny > grant) { throw new AccessDeniedException( this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access is denied")); } - if ((grant == deny) && (grant != 0)) { if (this.allowIfEqualGrantedDeniedDecisions) { return; } - else { - throw new AccessDeniedException( - this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access is denied")); - } + throw new AccessDeniedException( + this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access is denied")); } - // To get this far, every AccessDecisionVoter abstained checkAllowIfAllAbstainDecisions(); } diff --git a/core/src/main/java/org/springframework/security/access/vote/RoleVoter.java b/core/src/main/java/org/springframework/security/access/vote/RoleVoter.java index e20808eb73..546507df12 100644 --- a/core/src/main/java/org/springframework/security/access/vote/RoleVoter.java +++ b/core/src/main/java/org/springframework/security/access/vote/RoleVoter.java @@ -89,11 +89,9 @@ public class RoleVoter implements AccessDecisionVoter { } int result = ACCESS_ABSTAIN; Collection authorities = extractAuthorities(authentication); - for (ConfigAttribute attribute : attributes) { if (this.supports(attribute)) { result = ACCESS_DENIED; - // Attempt to find a matching granted authority for (GrantedAuthority authority : authorities) { if (attribute.getAttribute().equals(authority.getAuthority())) { @@ -102,7 +100,6 @@ public class RoleVoter implements AccessDecisionVoter { } } } - return result; } diff --git a/core/src/main/java/org/springframework/security/access/vote/UnanimousBased.java b/core/src/main/java/org/springframework/security/access/vote/UnanimousBased.java index 0dae111017..edadbb9f49 100644 --- a/core/src/main/java/org/springframework/security/access/vote/UnanimousBased.java +++ b/core/src/main/java/org/springframework/security/access/vote/UnanimousBased.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDecisionVoter; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; @@ -56,45 +57,33 @@ public class UnanimousBased extends AbstractAccessDecisionManager { * @throws AccessDeniedException if access is denied */ @Override + @SuppressWarnings({ "rawtypes", "unchecked" }) public void decide(Authentication authentication, Object object, Collection attributes) throws AccessDeniedException { - int grant = 0; - List singleAttributeList = new ArrayList<>(1); singleAttributeList.add(null); - for (ConfigAttribute attribute : attributes) { singleAttributeList.set(0, attribute); - for (AccessDecisionVoter voter : getDecisionVoters()) { int result = voter.vote(authentication, object, singleAttributeList); - - if (this.logger.isDebugEnabled()) { - this.logger.debug("Voter: " + voter + ", returned: " + result); - } - + this.logger.debug(LogMessage.format("Voter: %s, returned: %s", voter, result)); switch (result) { case AccessDecisionVoter.ACCESS_GRANTED: grant++; - break; - case AccessDecisionVoter.ACCESS_DENIED: throw new AccessDeniedException( this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access is denied")); - default: break; } } } - // To get this far, there were no deny votes if (grant > 0) { return; } - // To get this far, every AccessDecisionVoter abstained checkAllowIfAllAbstainDecisions(); } diff --git a/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java b/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java index dc01274d1f..422ce529d3 100644 --- a/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java +++ b/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java @@ -27,6 +27,7 @@ import org.springframework.security.core.CredentialsContainer; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.userdetails.UserDetails; +import org.springframework.util.Assert; /** * Base class for Authentication objects. @@ -54,15 +55,10 @@ public abstract class AbstractAuthenticationToken implements Authentication, Cre this.authorities = AuthorityUtils.NO_AUTHORITIES; return; } - for (GrantedAuthority a : authorities) { - if (a == null) { - throw new IllegalArgumentException("Authorities collection cannot contain any null elements"); - } + Assert.notNull(a, "Authorities collection cannot contain any null elements"); } - ArrayList temp = new ArrayList<>(authorities.size()); - temp.addAll(authorities); - this.authorities = Collections.unmodifiableList(temp); + this.authorities = Collections.unmodifiableList(new ArrayList<>(authorities)); } @Override @@ -81,7 +77,6 @@ public abstract class AbstractAuthenticationToken implements Authentication, Cre if (this.getPrincipal() instanceof Principal) { return ((Principal) this.getPrincipal()).getName(); } - return (this.getPrincipal() == null) ? "" : this.getPrincipal().toString(); } @@ -127,68 +122,52 @@ public abstract class AbstractAuthenticationToken implements Authentication, Cre if (!(obj instanceof AbstractAuthenticationToken)) { return false; } - AbstractAuthenticationToken test = (AbstractAuthenticationToken) obj; - if (!this.authorities.equals(test.authorities)) { return false; } - if ((this.details == null) && (test.getDetails() != null)) { return false; } - if ((this.details != null) && (test.getDetails() == null)) { return false; } - if ((this.details != null) && (!this.details.equals(test.getDetails()))) { return false; } - if ((this.getCredentials() == null) && (test.getCredentials() != null)) { return false; } - if ((this.getCredentials() != null) && !this.getCredentials().equals(test.getCredentials())) { return false; } - if (this.getPrincipal() == null && test.getPrincipal() != null) { return false; } - if (this.getPrincipal() != null && !this.getPrincipal().equals(test.getPrincipal())) { return false; } - return this.isAuthenticated() == test.isAuthenticated(); } @Override public int hashCode() { int code = 31; - for (GrantedAuthority authority : this.authorities) { code ^= authority.hashCode(); } - if (this.getPrincipal() != null) { code ^= this.getPrincipal().hashCode(); } - if (this.getCredentials() != null) { code ^= this.getCredentials().hashCode(); } - if (this.getDetails() != null) { code ^= this.getDetails().hashCode(); } - if (this.isAuthenticated()) { code ^= -37; } - return code; } @@ -200,23 +179,19 @@ public abstract class AbstractAuthenticationToken implements Authentication, Cre sb.append("Credentials: [PROTECTED]; "); sb.append("Authenticated: ").append(this.isAuthenticated()).append("; "); sb.append("Details: ").append(this.getDetails()).append("; "); - if (!this.authorities.isEmpty()) { sb.append("Granted Authorities: "); - int i = 0; for (GrantedAuthority authority : this.authorities) { if (i++ > 0) { sb.append(", "); } - sb.append(authority); } } else { sb.append("Not granted any authorities"); } - return sb.toString(); } diff --git a/core/src/main/java/org/springframework/security/authentication/AbstractUserDetailsReactiveAuthenticationManager.java b/core/src/main/java/org/springframework/security/authentication/AbstractUserDetailsReactiveAuthenticationManager.java index 08b86ca969..0c1b6daf5d 100644 --- a/core/src/main/java/org/springframework/security/authentication/AbstractUserDetailsReactiveAuthenticationManager.java +++ b/core/src/main/java/org/springframework/security/authentication/AbstractUserDetailsReactiveAuthenticationManager.java @@ -57,55 +57,60 @@ public abstract class AbstractUserDetailsReactiveAuthenticationManager implement private Scheduler scheduler = Schedulers.boundedElastic(); - private UserDetailsChecker preAuthenticationChecks = (user) -> { + private UserDetailsChecker preAuthenticationChecks = this::defaultPreAuthenticationChecks; + + private UserDetailsChecker postAuthenticationChecks = this::defaultPostAuthenticationChecks; + + private void defaultPreAuthenticationChecks(UserDetails user) { if (!user.isAccountNonLocked()) { this.logger.debug("User account is locked"); - throw new LockedException(this.messages.getMessage("AbstractUserDetailsAuthenticationProvider.locked", "User account is locked")); } - if (!user.isEnabled()) { this.logger.debug("User account is disabled"); - throw new DisabledException( this.messages.getMessage("AbstractUserDetailsAuthenticationProvider.disabled", "User is disabled")); } - if (!user.isAccountNonExpired()) { this.logger.debug("User account is expired"); - throw new AccountExpiredException(this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.expired", "User account has expired")); } - }; + } - private UserDetailsChecker postAuthenticationChecks = (user) -> { + private void defaultPostAuthenticationChecks(UserDetails user) { if (!user.isCredentialsNonExpired()) { this.logger.debug("User account credentials have expired"); - throw new CredentialsExpiredException(this.messages.getMessage( "AbstractUserDetailsAuthenticationProvider.credentialsExpired", "User credentials have expired")); } - }; + } @Override public Mono authenticate(Authentication authentication) { - final String username = authentication.getName(); - final String presentedPassword = (String) authentication.getCredentials(); + String username = authentication.getName(); + String presentedPassword = (String) authentication.getCredentials(); return retrieveUser(username).doOnNext(this.preAuthenticationChecks::check).publishOn(this.scheduler) - .filter((u) -> this.passwordEncoder.matches(presentedPassword, u.getPassword())) + .filter((userDetails) -> this.passwordEncoder.matches(presentedPassword, userDetails.getPassword())) .switchIfEmpty(Mono.defer(() -> Mono.error(new BadCredentialsException("Invalid Credentials")))) - .flatMap((u) -> { - boolean upgradeEncoding = this.userDetailsPasswordService != null - && this.passwordEncoder.upgradeEncoding(u.getPassword()); - if (upgradeEncoding) { - String newPassword = this.passwordEncoder.encode(presentedPassword); - return this.userDetailsPasswordService.updatePassword(u, newPassword); - } - return Mono.just(u); - }).doOnNext(this.postAuthenticationChecks::check) - .map((u) -> new UsernamePasswordAuthenticationToken(u, u.getPassword(), u.getAuthorities())); + .flatMap((userDetails) -> upgradeEncodingIfNecessary(userDetails, presentedPassword)) + .doOnNext(this.postAuthenticationChecks::check).map(this::createUsernamePasswordAuthenticationToken); + } + + private Mono upgradeEncodingIfNecessary(UserDetails userDetails, String presentedPassword) { + boolean upgradeEncoding = this.userDetailsPasswordService != null + && this.passwordEncoder.upgradeEncoding(userDetails.getPassword()); + if (upgradeEncoding) { + String newPassword = this.passwordEncoder.encode(presentedPassword); + return this.userDetailsPasswordService.updatePassword(userDetails, newPassword); + } + return Mono.just(userDetails); + } + + private UsernamePasswordAuthenticationToken createUsernamePasswordAuthenticationToken(UserDetails userDetails) { + return new UsernamePasswordAuthenticationToken(userDetails, userDetails.getPassword(), + userDetails.getAuthorities()); } /** diff --git a/core/src/main/java/org/springframework/security/authentication/AccountStatusUserDetailsChecker.java b/core/src/main/java/org/springframework/security/authentication/AccountStatusUserDetailsChecker.java index 2431294b01..9770a62111 100644 --- a/core/src/main/java/org/springframework/security/authentication/AccountStatusUserDetailsChecker.java +++ b/core/src/main/java/org/springframework/security/authentication/AccountStatusUserDetailsChecker.java @@ -37,17 +37,14 @@ public class AccountStatusUserDetailsChecker implements UserDetailsChecker, Mess throw new LockedException( this.messages.getMessage("AccountStatusUserDetailsChecker.locked", "User account is locked")); } - if (!user.isEnabled()) { throw new DisabledException( this.messages.getMessage("AccountStatusUserDetailsChecker.disabled", "User is disabled")); } - if (!user.isAccountNonExpired()) { throw new AccountExpiredException( this.messages.getMessage("AccountStatusUserDetailsChecker.expired", "User account has expired")); } - if (!user.isCredentialsNonExpired()) { throw new CredentialsExpiredException(this.messages .getMessage("AccountStatusUserDetailsChecker.credentialsExpired", "User credentials have expired")); diff --git a/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationProvider.java index 74b428b30d..dbff14cb4c 100644 --- a/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationProvider.java @@ -49,12 +49,10 @@ public class AnonymousAuthenticationProvider implements AuthenticationProvider, if (!supports(authentication.getClass())) { return null; } - if (this.key.hashCode() != ((AnonymousAuthenticationToken) authentication).getKeyHash()) { throw new BadCredentialsException(this.messages.getMessage("AnonymousAuthenticationProvider.incorrectKey", "The presented AnonymousAuthenticationToken does not contain the expected key")); } - return authentication; } diff --git a/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationToken.java b/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationToken.java index ede6e269b2..2e92d105f7 100644 --- a/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationToken.java +++ b/core/src/main/java/org/springframework/security/authentication/AnonymousAuthenticationToken.java @@ -57,12 +57,8 @@ public class AnonymousAuthenticationToken extends AbstractAuthenticationToken im private AnonymousAuthenticationToken(Integer keyHash, Object principal, Collection authorities) { super(authorities); - - if (principal == null || "".equals(principal)) { - throw new IllegalArgumentException("principal cannot be null or empty"); - } + Assert.isTrue(principal != null && !"".equals(principal), "principal cannot be null or empty"); Assert.notEmpty(authorities, "authorities cannot be null or empty"); - this.keyHash = keyHash; this.principal = principal; setAuthenticated(true); @@ -78,17 +74,10 @@ public class AnonymousAuthenticationToken extends AbstractAuthenticationToken im if (!super.equals(obj)) { return false; } - if (obj instanceof AnonymousAuthenticationToken) { AnonymousAuthenticationToken test = (AnonymousAuthenticationToken) obj; - - if (this.getKeyHash() != test.getKeyHash()) { - return false; - } - - return true; + return (this.getKeyHash() == test.getKeyHash()); } - return false; } diff --git a/core/src/main/java/org/springframework/security/authentication/AuthenticationTrustResolverImpl.java b/core/src/main/java/org/springframework/security/authentication/AuthenticationTrustResolverImpl.java index f9b16aea86..a34645cde0 100644 --- a/core/src/main/java/org/springframework/security/authentication/AuthenticationTrustResolverImpl.java +++ b/core/src/main/java/org/springframework/security/authentication/AuthenticationTrustResolverImpl.java @@ -48,7 +48,6 @@ public class AuthenticationTrustResolverImpl implements AuthenticationTrustResol if ((this.anonymousClass == null) || (authentication == null)) { return false; } - return this.anonymousClass.isAssignableFrom(authentication.getClass()); } @@ -57,7 +56,6 @@ public class AuthenticationTrustResolverImpl implements AuthenticationTrustResol if ((this.rememberMeClass == null) || (authentication == null)) { return false; } - return this.rememberMeClass.isAssignableFrom(authentication.getClass()); } diff --git a/core/src/main/java/org/springframework/security/authentication/CachingUserDetailsService.java b/core/src/main/java/org/springframework/security/authentication/CachingUserDetailsService.java index d33eda5e0a..39dc1f6487 100644 --- a/core/src/main/java/org/springframework/security/authentication/CachingUserDetailsService.java +++ b/core/src/main/java/org/springframework/security/authentication/CachingUserDetailsService.java @@ -47,16 +47,12 @@ public class CachingUserDetailsService implements UserDetailsService { @Override public UserDetails loadUserByUsername(String username) { UserDetails user = this.userCache.getUserFromCache(username); - if (user == null) { user = this.delegate.loadUserByUsername(username); } - Assert.notNull(user, () -> "UserDetailsService " + this.delegate + " returned null for username " + username + ". " + "This is an interface contract violation"); - this.userCache.putUserInCache(user); - return user; } diff --git a/core/src/main/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisher.java b/core/src/main/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisher.java index 6e9049f7be..a5e9ff8619 100644 --- a/core/src/main/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisher.java +++ b/core/src/main/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisher.java @@ -79,7 +79,6 @@ public class DefaultAuthenticationEventPublisher public DefaultAuthenticationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { this.applicationEventPublisher = applicationEventPublisher; - addMapping(BadCredentialsException.class.getName(), AuthenticationFailureBadCredentialsEvent.class); addMapping(UsernameNotFoundException.class.getName(), AuthenticationFailureBadCredentialsEvent.class); addMapping(AccountExpiredException.class.getName(), AuthenticationFailureExpiredEvent.class); @@ -105,7 +104,6 @@ public class DefaultAuthenticationEventPublisher public void publishAuthenticationFailure(AuthenticationException exception, Authentication authentication) { Constructor constructor = getEventConstructor(exception); AbstractAuthenticationEvent event = null; - if (constructor != null) { try { event = constructor.newInstance(authentication, exception); @@ -113,7 +111,6 @@ public class DefaultAuthenticationEventPublisher catch (IllegalAccessException | InvocationTargetException | InstantiationException ignored) { } } - if (event != null) { if (this.applicationEventPublisher != null) { this.applicationEventPublisher.publishEvent(event); diff --git a/core/src/main/java/org/springframework/security/authentication/ProviderManager.java b/core/src/main/java/org/springframework/security/authentication/ProviderManager.java index 1b9a6a8ca6..622c0284b1 100644 --- a/core/src/main/java/org/springframework/security/authentication/ProviderManager.java +++ b/core/src/main/java/org/springframework/security/authentication/ProviderManager.java @@ -27,6 +27,7 @@ import org.springframework.beans.factory.InitializingBean; import org.springframework.context.MessageSource; import org.springframework.context.MessageSourceAware; import org.springframework.context.support.MessageSourceAccessor; +import org.springframework.core.log.LogMessage; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; import org.springframework.security.core.CredentialsContainer; @@ -134,13 +135,10 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar } private void checkState() { - if (this.parent == null && this.providers.isEmpty()) { - throw new IllegalArgumentException( - "A parent AuthenticationManager or a list " + "of AuthenticationProviders is required"); - } - else if (CollectionUtils.contains(this.providers.iterator(), null)) { - throw new IllegalArgumentException("providers list cannot contain null values"); - } + Assert.isTrue(this.parent != null || !this.providers.isEmpty(), + "A parent AuthenticationManager or a list of AuthenticationProviders is required"); + Assert.isTrue(!CollectionUtils.contains(this.providers.iterator(), null), + "providers list cannot contain null values"); } /** @@ -170,20 +168,13 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar AuthenticationException parentException = null; Authentication result = null; Authentication parentResult = null; - boolean debug = logger.isDebugEnabled(); - for (AuthenticationProvider provider : getProviders()) { if (!provider.supports(toTest)) { continue; } - - if (debug) { - logger.debug("Authentication attempt using " + provider.getClass().getName()); - } - + logger.debug(LogMessage.format("Authentication attempt using %s", provider.getClass().getName())); try { result = provider.authenticate(authentication); - if (result != null) { copyDetails(authentication, result); break; @@ -199,7 +190,6 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar lastException = ex; } } - if (result == null && this.parent != null) { // Allow the parent to try. try { @@ -217,14 +207,12 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar lastException = ex; } } - if (result != null) { if (this.eraseCredentialsAfterAuthentication && (result instanceof CredentialsContainer)) { // Authentication is complete. Remove credentials and other secret data // from authentication ((CredentialsContainer) result).eraseCredentials(); } - // If the parent AuthenticationManager was attempted and successful then it // will publish an AuthenticationSuccessEvent // This check prevents a duplicate AuthenticationSuccessEvent if the parent @@ -236,12 +224,10 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar } // Parent was null, or didn't authenticate (or throw an exception). - if (lastException == null) { lastException = new ProviderNotFoundException(this.messages.getMessage("ProviderManager.providerNotFound", new Object[] { toTest.getName() }, "No AuthenticationProvider found for {0}")); } - // If the parent AuthenticationManager was attempted and failed then it will // publish an AbstractAuthenticationFailureEvent // This check prevents a duplicate AbstractAuthenticationFailureEvent if the @@ -249,7 +235,6 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar if (parentException == null) { prepareException(lastException, authentication); } - throw lastException; } @@ -267,7 +252,6 @@ public class ProviderManager implements AuthenticationManager, MessageSourceAwar private void copyDetails(Authentication source, Authentication dest) { if ((dest instanceof AbstractAuthenticationToken) && (dest.getDetails() == null)) { AbstractAuthenticationToken token = (AbstractAuthenticationToken) dest; - token.setDetails(source.getDetails()); } } diff --git a/core/src/main/java/org/springframework/security/authentication/ReactiveAuthenticationManagerAdapter.java b/core/src/main/java/org/springframework/security/authentication/ReactiveAuthenticationManagerAdapter.java index bb5d6dc17e..01612df39c 100644 --- a/core/src/main/java/org/springframework/security/authentication/ReactiveAuthenticationManagerAdapter.java +++ b/core/src/main/java/org/springframework/security/authentication/ReactiveAuthenticationManagerAdapter.java @@ -47,14 +47,17 @@ public class ReactiveAuthenticationManagerAdapter implements ReactiveAuthenticat @Override public Mono authenticate(Authentication token) { - return Mono.just(token).publishOn(this.scheduler).flatMap((t) -> { - try { - return Mono.just(this.authenticationManager.authenticate(t)); - } - catch (Throwable error) { - return Mono.error(error); - } - }).filter((a) -> a.isAuthenticated()); + return Mono.just(token).publishOn(this.scheduler).flatMap(this::doAuthenticate) + .filter(Authentication::isAuthenticated); + } + + private Mono doAuthenticate(Authentication authentication) { + try { + return Mono.just(this.authenticationManager.authenticate(authentication)); + } + catch (Throwable ex) { + return Mono.error(ex); + } } /** diff --git a/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationProvider.java index f15311ab6a..ed9df33a28 100644 --- a/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationProvider.java @@ -53,12 +53,10 @@ public class RememberMeAuthenticationProvider implements AuthenticationProvider, if (!supports(authentication.getClass())) { return null; } - if (this.key.hashCode() != ((RememberMeAuthenticationToken) authentication).getKeyHash()) { throw new BadCredentialsException(this.messages.getMessage("RememberMeAuthenticationProvider.incorrectKey", "The presented RememberMeAuthenticationToken does not contain the expected key")); } - return authentication; } diff --git a/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationToken.java b/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationToken.java index cf26e8851b..8a62c9ca2a 100644 --- a/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationToken.java +++ b/core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationToken.java @@ -48,11 +48,9 @@ public class RememberMeAuthenticationToken extends AbstractAuthenticationToken { public RememberMeAuthenticationToken(String key, Object principal, Collection authorities) { super(authorities); - if ((key == null) || ("".equals(key)) || (principal == null) || "".equals(principal)) { throw new IllegalArgumentException("Cannot pass null or empty values to constructor"); } - this.keyHash = key.hashCode(); this.principal = principal; setAuthenticated(true); @@ -68,7 +66,6 @@ public class RememberMeAuthenticationToken extends AbstractAuthenticationToken { private RememberMeAuthenticationToken(Integer keyHash, Object principal, Collection authorities) { super(authorities); - this.keyHash = keyHash; this.principal = principal; setAuthenticated(true); @@ -97,17 +94,13 @@ public class RememberMeAuthenticationToken extends AbstractAuthenticationToken { if (!super.equals(obj)) { return false; } - if (obj instanceof RememberMeAuthenticationToken) { - RememberMeAuthenticationToken test = (RememberMeAuthenticationToken) obj; - - if (this.getKeyHash() != test.getKeyHash()) { + RememberMeAuthenticationToken other = (RememberMeAuthenticationToken) obj; + if (this.getKeyHash() != other.getKeyHash()) { return false; } - return true; } - return false; } diff --git a/core/src/main/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java b/core/src/main/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java index ca28be6eb5..55963150a6 100644 --- a/core/src/main/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java +++ b/core/src/main/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java @@ -20,6 +20,7 @@ import java.util.Collection; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.SpringSecurityCoreVersion; +import org.springframework.util.Assert; /** * An {@link org.springframework.security.core.Authentication} implementation that is @@ -82,11 +83,8 @@ public class UsernamePasswordAuthenticationToken extends AbstractAuthenticationT @Override public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { - if (isAuthenticated) { - throw new IllegalArgumentException( - "Cannot set this token to trusted - use constructor which takes a GrantedAuthority list instead"); - } - + Assert.isTrue(!isAuthenticated, + "Cannot set this token to trusted - use constructor which takes a GrantedAuthority list instead"); super.setAuthenticated(false); } diff --git a/core/src/main/java/org/springframework/security/authentication/dao/AbstractUserDetailsAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/dao/AbstractUserDetailsAuthenticationProvider.java index b9f93d4a7d..953e99455c 100644 --- a/core/src/main/java/org/springframework/security/authentication/dao/AbstractUserDetailsAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/dao/AbstractUserDetailsAuthenticationProvider.java @@ -124,67 +124,54 @@ public abstract class AbstractUserDetailsAuthenticationProvider Assert.isInstanceOf(UsernamePasswordAuthenticationToken.class, authentication, () -> this.messages.getMessage("AbstractUserDetailsAuthenticationProvider.onlySupports", "Only UsernamePasswordAuthenticationToken is supported")); - - // Determine username - String username = (authentication.getPrincipal() == null) ? "NONE_PROVIDED" : authentication.getName(); - + String username = determineUsername(authentication); boolean cacheWasUsed = true; UserDetails user = this.userCache.getUserFromCache(username); - if (user == null) { cacheWasUsed = false; - try { user = retrieveUser(username, (UsernamePasswordAuthenticationToken) authentication); } - catch (UsernameNotFoundException notFound) { + catch (UsernameNotFoundException ex) { this.logger.debug("User '" + username + "' not found"); - - if (this.hideUserNotFoundExceptions) { - throw new BadCredentialsException(this.messages - .getMessage("AbstractUserDetailsAuthenticationProvider.badCredentials", "Bad credentials")); - } - else { - throw notFound; + if (!this.hideUserNotFoundExceptions) { + throw ex; } + throw new BadCredentialsException(this.messages + .getMessage("AbstractUserDetailsAuthenticationProvider.badCredentials", "Bad credentials")); } - Assert.notNull(user, "retrieveUser returned null - a violation of the interface contract"); } - try { this.preAuthenticationChecks.check(user); additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); } - catch (AuthenticationException exception) { - if (cacheWasUsed) { - // There was a problem, so try again after checking - // we're using latest data (i.e. not from the cache) - cacheWasUsed = false; - user = retrieveUser(username, (UsernamePasswordAuthenticationToken) authentication); - this.preAuthenticationChecks.check(user); - additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); - } - else { - throw exception; + catch (AuthenticationException ex) { + if (!cacheWasUsed) { + throw ex; } + // There was a problem, so try again after checking + // we're using latest data (i.e. not from the cache) + cacheWasUsed = false; + user = retrieveUser(username, (UsernamePasswordAuthenticationToken) authentication); + this.preAuthenticationChecks.check(user); + additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); } - this.postAuthenticationChecks.check(user); - if (!cacheWasUsed) { this.userCache.putUserInCache(user); } - Object principalToReturn = user; - if (this.forcePrincipalAsString) { principalToReturn = user.getUsername(); } - return createSuccessAuthentication(principalToReturn, authentication, user); } + private String determineUsername(Authentication authentication) { + return (authentication.getPrincipal() == null) ? "NONE_PROVIDED" : authentication.getName(); + } + /** * Creates a successful {@link Authentication} object. *

@@ -209,7 +196,6 @@ public abstract class AbstractUserDetailsAuthenticationProvider UsernamePasswordAuthenticationToken result = new UsernamePasswordAuthenticationToken(principal, authentication.getCredentials(), this.authoritiesMapper.mapAuthorities(user.getAuthorities())); result.setDetails(authentication.getDetails()); - return result; } @@ -333,21 +319,16 @@ public abstract class AbstractUserDetailsAuthenticationProvider public void check(UserDetails user) { if (!user.isAccountNonLocked()) { AbstractUserDetailsAuthenticationProvider.this.logger.debug("User account is locked"); - throw new LockedException(AbstractUserDetailsAuthenticationProvider.this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.locked", "User account is locked")); } - if (!user.isEnabled()) { AbstractUserDetailsAuthenticationProvider.this.logger.debug("User account is disabled"); - throw new DisabledException(AbstractUserDetailsAuthenticationProvider.this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.disabled", "User is disabled")); } - if (!user.isAccountNonExpired()) { AbstractUserDetailsAuthenticationProvider.this.logger.debug("User account is expired"); - throw new AccountExpiredException(AbstractUserDetailsAuthenticationProvider.this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.expired", "User account has expired")); } @@ -361,7 +342,6 @@ public abstract class AbstractUserDetailsAuthenticationProvider public void check(UserDetails user) { if (!user.isCredentialsNonExpired()) { AbstractUserDetailsAuthenticationProvider.this.logger.debug("User account credentials have expired"); - throw new CredentialsExpiredException(AbstractUserDetailsAuthenticationProvider.this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.credentialsExpired", "User credentials have expired")); diff --git a/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java index ecb852cb54..245101592f 100644 --- a/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java @@ -69,16 +69,12 @@ public class DaoAuthenticationProvider extends AbstractUserDetailsAuthentication UsernamePasswordAuthenticationToken authentication) throws AuthenticationException { if (authentication.getCredentials() == null) { this.logger.debug("Authentication failed: no credentials provided"); - throw new BadCredentialsException(this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.badCredentials", "Bad credentials")); } - String presentedPassword = authentication.getCredentials().toString(); - if (!this.passwordEncoder.matches(presentedPassword, userDetails.getPassword())) { this.logger.debug("Authentication failed: password does not match stored value"); - throw new BadCredentialsException(this.messages .getMessage("AbstractUserDetailsAuthenticationProvider.badCredentials", "Bad credentials")); } diff --git a/core/src/main/java/org/springframework/security/authentication/event/LoggerListener.java b/core/src/main/java/org/springframework/security/authentication/event/LoggerListener.java index 71b153c201..92fda0f52d 100644 --- a/core/src/main/java/org/springframework/security/authentication/event/LoggerListener.java +++ b/core/src/main/java/org/springframework/security/authentication/event/LoggerListener.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.context.ApplicationListener; +import org.springframework.core.log.LogMessage; import org.springframework.util.ClassUtils; /** @@ -44,23 +45,22 @@ public class LoggerListener implements ApplicationListener getLogMessage(event))); + } - if (logger.isWarnEnabled()) { - final StringBuilder builder = new StringBuilder(); - builder.append("Authentication event "); - builder.append(ClassUtils.getShortName(event.getClass())); - builder.append(": "); - builder.append(event.getAuthentication().getName()); - builder.append("; details: "); - builder.append(event.getAuthentication().getDetails()); - - if (event instanceof AbstractAuthenticationFailureEvent) { - builder.append("; exception: "); - builder.append(((AbstractAuthenticationFailureEvent) event).getException().getMessage()); - } - - logger.warn(builder.toString()); + private String getLogMessage(AbstractAuthenticationEvent event) { + StringBuilder builder = new StringBuilder(); + builder.append("Authentication event "); + builder.append(ClassUtils.getShortName(event.getClass())); + builder.append(": "); + builder.append(event.getAuthentication().getName()); + builder.append("; details: "); + builder.append(event.getAuthentication().getDetails()); + if (event instanceof AbstractAuthenticationFailureEvent) { + builder.append("; exception: "); + builder.append(((AbstractAuthenticationFailureEvent) event).getException().getMessage()); } + return builder.toString(); } public boolean isLogInteractiveAuthenticationSuccessEvents() { diff --git a/core/src/main/java/org/springframework/security/authentication/jaas/AbstractJaasAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/jaas/AbstractJaasAuthenticationProvider.java index 93903db4da..d9327ff437 100644 --- a/core/src/main/java/org/springframework/security/authentication/jaas/AbstractJaasAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/jaas/AbstractJaasAuthenticationProvider.java @@ -36,6 +36,7 @@ import org.springframework.beans.factory.InitializingBean; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.context.ApplicationListener; +import org.springframework.core.log.LogMessage; import org.springframework.security.authentication.AuthenticationProvider; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.authentication.jaas.event.JaasAuthenticationFailedEvent; @@ -46,6 +47,7 @@ import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.session.SessionDestroyedEvent; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.util.ObjectUtils; /** @@ -161,58 +163,53 @@ public abstract class AbstractJaasAuthenticationProvider implements Authenticati if (!(auth instanceof UsernamePasswordAuthenticationToken)) { return null; } - UsernamePasswordAuthenticationToken request = (UsernamePasswordAuthenticationToken) auth; Set authorities; - try { // Create the LoginContext object, and pass our InternallCallbackHandler LoginContext loginContext = createLoginContext(new InternalCallbackHandler(auth)); - // Attempt to login the user, the LoginContext will call our // InternalCallbackHandler at this point. loginContext.login(); - - // Create a set to hold the authorities, and add any that have already been - // applied. - authorities = new HashSet<>(); - // Get the subject principals and pass them to each of the AuthorityGranters Set principals = loginContext.getSubject().getPrincipals(); - - for (Principal principal : principals) { - for (AuthorityGranter granter : this.authorityGranters) { - Set roles = granter.grant(principal); - - // If the granter doesn't wish to grant any authorities, it should - // return null. - if ((roles != null) && !roles.isEmpty()) { - for (String role : roles) { - authorities.add(new JaasGrantedAuthority(role, principal)); - } - } - } - } - + // Create a set to hold the authorities, and add any that have already been + // applied. + authorities = getAuthorities(principals); // Convert the authorities set back to an array and apply it to the token. JaasAuthenticationToken result = new JaasAuthenticationToken(request.getPrincipal(), request.getCredentials(), new ArrayList<>(authorities), loginContext); - // Publish the success event publishSuccessEvent(result); - // we're done, return the token. return result; } - catch (LoginException loginException) { - AuthenticationException ase = this.loginExceptionResolver.resolveException(loginException); - - publishFailureEvent(request, ase); - throw ase; + catch (LoginException ex) { + AuthenticationException resolvedException = this.loginExceptionResolver.resolveException(ex); + publishFailureEvent(request, resolvedException); + throw resolvedException; } } + private Set getAuthorities(Set principals) { + Set authorities; + authorities = new HashSet<>(); + for (Principal principal : principals) { + for (AuthorityGranter granter : this.authorityGranters) { + Set roles = granter.grant(principal); + // If the granter doesn't wish to grant any authorities, + // it should return null. + if (!CollectionUtils.isEmpty(roles)) { + for (String role : roles) { + authorities.add(new JaasGrantedAuthority(role, principal)); + } + } + } + } + return authorities; + } + /** * Creates the LoginContext to be used for authentication. * @param handler The CallbackHandler that should be used for the LoginContext (never @@ -230,32 +227,17 @@ public abstract class AbstractJaasAuthenticationProvider implements Authenticati */ protected void handleLogout(SessionDestroyedEvent event) { List contexts = event.getSecurityContexts(); - if (contexts.isEmpty()) { this.log.debug("The destroyed session has no SecurityContexts"); - return; } - for (SecurityContext context : contexts) { Authentication auth = context.getAuthentication(); - if ((auth != null) && (auth instanceof JaasAuthenticationToken)) { JaasAuthenticationToken token = (JaasAuthenticationToken) auth; - try { LoginContext loginContext = token.getLoginContext(); - boolean debug = this.log.isDebugEnabled(); - if (loginContext != null) { - if (debug) { - this.log.debug("Logging principal: [" + token.getPrincipal() + "] out of LoginContext"); - } - loginContext.logout(); - } - else if (debug) { - this.log.debug("Cannot logout principal: [" + token.getPrincipal() + "] from LoginContext. " - + "The LoginContext is unavailable"); - } + logout(token, loginContext); } catch (LoginException ex) { this.log.warn("Error error logging out of LoginContext", ex); @@ -264,6 +246,17 @@ public abstract class AbstractJaasAuthenticationProvider implements Authenticati } } + private void logout(JaasAuthenticationToken token, LoginContext loginContext) throws LoginException { + if (loginContext != null) { + this.log.debug( + LogMessage.of(() -> "Logging principal: [" + token.getPrincipal() + "] out of LoginContext")); + loginContext.logout(); + return; + } + this.log.debug(LogMessage.of(() -> "Cannot logout principal: [" + token.getPrincipal() + + "] from LoginContext. The LoginContext is unavailable")); + } + @Override public void onApplicationEvent(SessionDestroyedEvent event) { handleLogout(event); diff --git a/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java index f5b20e8653..f9e38143c4 100644 --- a/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.core.io.Resource; +import org.springframework.core.log.LogMessage; import org.springframework.security.authentication.AuthenticationProvider; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.authentication.jaas.event.JaasAuthenticationFailedEvent; @@ -153,7 +154,6 @@ public class JaasAuthenticationProvider extends AbstractJaasAuthenticationProvid Assert.hasLength(getLoginContextName(), () -> "loginContextName must be set on " + getClass()); Assert.notNull(this.loginConfig, () -> "loginConfig must be set on " + getClass()); configureJaas(this.loginConfig); - Assert.notNull(Configuration.getConfiguration(), "As per https://java.sun.com/j2se/1.5.0/docs/api/javax/security/auth/login/Configuration.html " + "\"If a Configuration object was set via the Configuration.setConfiguration method, then that object is " @@ -173,7 +173,6 @@ public class JaasAuthenticationProvider extends AbstractJaasAuthenticationProvid */ protected void configureJaas(Resource loginConfig) throws IOException { configureJaasUsingLoop(); - if (this.refreshConfigurationOnStartup) { // Overcome issue in SEC-760 Configuration.getConfiguration().refresh(); @@ -189,38 +188,30 @@ public class JaasAuthenticationProvider extends AbstractJaasAuthenticationProvid private void configureJaasUsingLoop() throws IOException { String loginConfigUrl = convertLoginConfigToUrl(); boolean alreadySet = false; - int n = 1; final String prefix = "login.config.url."; String existing; - while ((existing = Security.getProperty(prefix + n)) != null) { alreadySet = existing.equals(loginConfigUrl); - if (alreadySet) { break; } - n++; } - if (!alreadySet) { String key = prefix + n; - log.debug("Setting security property [" + key + "] to: " + loginConfigUrl); + log.debug(LogMessage.format("Setting security property [%s] to: %s", key, loginConfigUrl)); Security.setProperty(key, loginConfigUrl); } } private String convertLoginConfigToUrl() throws IOException { String loginConfigPath; - try { loginConfigPath = this.loginConfig.getFile().getAbsolutePath().replace(File.separatorChar, '/'); - if (!loginConfigPath.startsWith("/")) { loginConfigPath = "/" + loginConfigPath; } - return new URL("file", "", loginConfigPath).toString(); } catch (IOException ex) { diff --git a/core/src/main/java/org/springframework/security/authentication/jaas/JaasNameCallbackHandler.java b/core/src/main/java/org/springframework/security/authentication/jaas/JaasNameCallbackHandler.java index d2aba3590d..1f7881283f 100644 --- a/core/src/main/java/org/springframework/security/authentication/jaas/JaasNameCallbackHandler.java +++ b/core/src/main/java/org/springframework/security/authentication/jaas/JaasNameCallbackHandler.java @@ -46,20 +46,16 @@ public class JaasNameCallbackHandler implements JaasAuthenticationCallbackHandle @Override public void handle(Callback callback, Authentication authentication) { if (callback instanceof NameCallback) { - NameCallback ncb = (NameCallback) callback; - String username; - - Object principal = authentication.getPrincipal(); - - if (principal instanceof UserDetails) { - username = ((UserDetails) principal).getUsername(); - } - else { - username = principal.toString(); - } - - ncb.setName(username); + ((NameCallback) callback).setName(getUserName(authentication)); } } + private String getUserName(Authentication authentication) { + Object principal = authentication.getPrincipal(); + if (principal instanceof UserDetails) { + return ((UserDetails) principal).getUsername(); + } + return principal.toString(); + } + } diff --git a/core/src/main/java/org/springframework/security/authentication/jaas/JaasPasswordCallbackHandler.java b/core/src/main/java/org/springframework/security/authentication/jaas/JaasPasswordCallbackHandler.java index c558a6ea74..8b43440052 100644 --- a/core/src/main/java/org/springframework/security/authentication/jaas/JaasPasswordCallbackHandler.java +++ b/core/src/main/java/org/springframework/security/authentication/jaas/JaasPasswordCallbackHandler.java @@ -47,8 +47,7 @@ public class JaasPasswordCallbackHandler implements JaasAuthenticationCallbackHa @Override public void handle(Callback callback, Authentication auth) { if (callback instanceof PasswordCallback) { - PasswordCallback pc = (PasswordCallback) callback; - pc.setPassword(auth.getCredentials().toString().toCharArray()); + ((PasswordCallback) callback).setPassword(auth.getCredentials().toString().toCharArray()); } } diff --git a/core/src/main/java/org/springframework/security/authentication/jaas/SecurityContextLoginModule.java b/core/src/main/java/org/springframework/security/authentication/jaas/SecurityContextLoginModule.java index c74bfcd6c1..4cc8c8d402 100644 --- a/core/src/main/java/org/springframework/security/authentication/jaas/SecurityContextLoginModule.java +++ b/core/src/main/java/org/springframework/security/authentication/jaas/SecurityContextLoginModule.java @@ -73,9 +73,7 @@ public class SecurityContextLoginModule implements LoginModule { if (this.authen == null) { return false; } - this.authen = null; - return true; } @@ -91,9 +89,7 @@ public class SecurityContextLoginModule implements LoginModule { if (this.authen == null) { return false; } - this.subject.getPrincipals().add(this.authen); - return true; } @@ -119,7 +115,6 @@ public class SecurityContextLoginModule implements LoginModule { @SuppressWarnings("unchecked") public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) { this.subject = subject; - if (options != null) { this.ignoreMissingAuthentication = "true".equals(options.get("ignoreMissingAuthentication")); } @@ -135,21 +130,15 @@ public class SecurityContextLoginModule implements LoginModule { @Override public boolean login() throws LoginException { this.authen = SecurityContextHolder.getContext().getAuthentication(); - - if (this.authen == null) { - String msg = "Login cannot complete, authentication not found in security context"; - - if (this.ignoreMissingAuthentication) { - log.warn(msg); - - return false; - } - else { - throw new LoginException(msg); - } + if (this.authen != null) { + return true; } - - return true; + String msg = "Login cannot complete, authentication not found in security context"; + if (!this.ignoreMissingAuthentication) { + throw new LoginException(msg); + } + log.warn(msg); + return false; } /** @@ -163,10 +152,8 @@ public class SecurityContextLoginModule implements LoginModule { if (this.authen == null) { return false; } - this.subject.getPrincipals().remove(this.authen); this.authen = null; - return true; } diff --git a/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationManagerImpl.java b/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationManagerImpl.java index 5bf95fad72..2f3063cdd3 100644 --- a/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationManagerImpl.java +++ b/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationManagerImpl.java @@ -46,12 +46,11 @@ public class RemoteAuthenticationManagerImpl implements RemoteAuthenticationMana public Collection attemptAuthentication(String username, String password) throws RemoteAuthenticationException { UsernamePasswordAuthenticationToken request = new UsernamePasswordAuthenticationToken(username, password); - try { return this.authenticationManager.authenticate(request).getAuthorities(); } - catch (AuthenticationException authEx) { - throw new RemoteAuthenticationException(authEx.getMessage()); + catch (AuthenticationException ex) { + throw new RemoteAuthenticationException(ex.getMessage()); } } diff --git a/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationProvider.java index d68867bc7a..3ed938a248 100644 --- a/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/rcp/RemoteAuthenticationProvider.java @@ -66,7 +66,6 @@ public class RemoteAuthenticationProvider implements AuthenticationProvider, Ini String password = (credentials != null) ? credentials.toString() : null; Collection authorities = this.remoteAuthenticationManager .attemptAuthentication(username, password); - return new UsernamePasswordAuthenticationToken(username, password, authorities); } diff --git a/core/src/main/java/org/springframework/security/authorization/AuthenticatedReactiveAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/AuthenticatedReactiveAuthorizationManager.java index b854eac09f..2bba4744f6 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthenticatedReactiveAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthenticatedReactiveAuthorizationManager.java @@ -39,10 +39,14 @@ public class AuthenticatedReactiveAuthorizationManager implements ReactiveAut @Override public Mono check(Mono authentication, T object) { - return authentication.filter(this::isNotAnonymous).map((a) -> new AuthorizationDecision(a.isAuthenticated())) + return authentication.filter(this::isNotAnonymous).map(this::getAuthorizationDecision) .defaultIfEmpty(new AuthorizationDecision(false)); } + private AuthorizationDecision getAuthorizationDecision(Authentication authentication) { + return new AuthorizationDecision(authentication.isAuthenticated()); + } + /** * Verify (via {@link AuthenticationTrustResolver}) that the given authentication is * not anonymous. diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java index e8bb7b8fe8..144904a000 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java @@ -22,6 +22,7 @@ import java.util.List; import reactor.core.publisher.Mono; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; import org.springframework.util.Assert; /** @@ -42,9 +43,8 @@ public class AuthorityReactiveAuthorizationManager implements ReactiveAuthori @Override public Mono check(Mono authentication, T object) { - return authentication.filter((a) -> a.isAuthenticated()).flatMapIterable((a) -> a.getAuthorities()) - .map((g) -> g.getAuthority()).any((a) -> this.authorities.contains(a)) - .map((hasAuthority) -> new AuthorizationDecision(hasAuthority)) + return authentication.filter((a) -> a.isAuthenticated()).flatMapIterable(Authentication::getAuthorities) + .map(GrantedAuthority::getAuthority).any(this.authorities::contains).map(AuthorizationDecision::new) .defaultIfEmpty(new AuthorizationDecision(false)); } @@ -74,7 +74,6 @@ public class AuthorityReactiveAuthorizationManager implements ReactiveAuthori for (String authority : authorities) { Assert.notNull(authority, "authority cannot be null"); } - return new AuthorityReactiveAuthorizationManager<>(authorities); } @@ -104,7 +103,6 @@ public class AuthorityReactiveAuthorizationManager implements ReactiveAuthori for (String role : roles) { Assert.notNull(role, "role cannot be null"); } - return hasAnyAuthority(toNamedRolesArray(roles)); } diff --git a/core/src/main/java/org/springframework/security/authorization/ReactiveAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/ReactiveAuthorizationManager.java index 05b3adc293..ff5c304794 100644 --- a/core/src/main/java/org/springframework/security/authorization/ReactiveAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/ReactiveAuthorizationManager.java @@ -47,9 +47,9 @@ public interface ReactiveAuthorizationManager { * denied */ default Mono verify(Mono authentication, T object) { - return check(authentication, object).filter((d) -> d.isGranted()) + return check(authentication, object).filter(AuthorizationDecision::isGranted) .switchIfEmpty(Mono.defer(() -> Mono.error(new AccessDeniedException("Access Denied")))) - .flatMap((d) -> Mono.empty()); + .flatMap((decision) -> Mono.empty()); } } diff --git a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextCallable.java b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextCallable.java index ea7a1a9849..50d1b89af0 100644 --- a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextCallable.java +++ b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextCallable.java @@ -79,7 +79,6 @@ public final class DelegatingSecurityContextCallable implements Callable { @Override public V call() throws Exception { this.originalSecurityContext = SecurityContextHolder.getContext(); - try { SecurityContextHolder.setContext(this.delegateSecurityContext); return this.delegate.call(); diff --git a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutor.java b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutor.java index 4f035cac2b..c1af6a7546 100644 --- a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutor.java +++ b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutor.java @@ -59,8 +59,7 @@ public class DelegatingSecurityContextExecutor extends AbstractDelegatingSecurit @Override public final void execute(Runnable task) { - task = wrap(task); - this.delegate.execute(task); + this.delegate.execute(wrap(task)); } protected final Executor getDelegateExecutor() { diff --git a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutorService.java b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutorService.java index 34201d5594..289f9bec83 100644 --- a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutorService.java +++ b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextExecutorService.java @@ -90,20 +90,17 @@ public class DelegatingSecurityContextExecutorService extends DelegatingSecurity @Override public final Future submit(Callable task) { - task = wrap(task); - return getDelegate().submit(task); + return getDelegate().submit(wrap(task)); } @Override public final Future submit(Runnable task, T result) { - task = wrap(task); - return getDelegate().submit(task, result); + return getDelegate().submit(wrap(task), result); } @Override public final Future submit(Runnable task) { - task = wrap(task); - return getDelegate().submit(task); + return getDelegate().submit(wrap(task)); } @Override diff --git a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextRunnable.java b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextRunnable.java index 80def0d9cf..24b0746641 100644 --- a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextRunnable.java +++ b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextRunnable.java @@ -77,7 +77,6 @@ public final class DelegatingSecurityContextRunnable implements Runnable { @Override public void run() { this.originalSecurityContext = SecurityContextHolder.getContext(); - try { SecurityContextHolder.setContext(this.delegateSecurityContext); this.delegate.run(); diff --git a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextScheduledExecutorService.java b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextScheduledExecutorService.java index 3c01e1e201..ee8ff98489 100644 --- a/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextScheduledExecutorService.java +++ b/core/src/main/java/org/springframework/security/concurrent/DelegatingSecurityContextScheduledExecutorService.java @@ -61,26 +61,22 @@ public final class DelegatingSecurityContextScheduledExecutorService extends Del @Override public ScheduledFuture schedule(Runnable command, long delay, TimeUnit unit) { - command = wrap(command); - return getDelegate().schedule(command, delay, unit); + return getDelegate().schedule(wrap(command), delay, unit); } @Override public ScheduledFuture schedule(Callable callable, long delay, TimeUnit unit) { - callable = wrap(callable); - return getDelegate().schedule(callable, delay, unit); + return getDelegate().schedule(wrap(callable), delay, unit); } @Override public ScheduledFuture scheduleAtFixedRate(Runnable command, long initialDelay, long period, TimeUnit unit) { - command = wrap(command); - return getDelegate().scheduleAtFixedRate(command, initialDelay, period, unit); + return getDelegate().scheduleAtFixedRate(wrap(command), initialDelay, period, unit); } @Override public ScheduledFuture scheduleWithFixedDelay(Runnable command, long initialDelay, long delay, TimeUnit unit) { - command = wrap(command); - return getDelegate().scheduleWithFixedDelay(command, initialDelay, delay, unit); + return getDelegate().scheduleWithFixedDelay(wrap(command), initialDelay, delay, unit); } private ScheduledExecutorService getDelegate() { diff --git a/core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java b/core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java index b675c35855..c330d4121e 100644 --- a/core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java +++ b/core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java @@ -72,7 +72,7 @@ public final class RsaKeyConverters { return (source) -> { List lines = readAllLines(source); Assert.isTrue(!lines.isEmpty() && lines.get(0).startsWith(PKCS8_PEM_HEADER), - "Key is not in PEM-encoded PKCS#8 format, " + "please check that the header begins with -----" + "Key is not in PEM-encoded PKCS#8 format, please check that the header begins with -----" + PKCS8_PEM_HEADER + "-----"); StringBuilder base64Encoded = new StringBuilder(); for (String line : lines) { @@ -81,7 +81,6 @@ public final class RsaKeyConverters { } } byte[] pkcs8 = Base64.getDecoder().decode(base64Encoded.toString()); - try { return (RSAPrivateKey) keyFactory.generatePrivate(new PKCS8EncodedKeySpec(pkcs8)); } @@ -105,7 +104,7 @@ public final class RsaKeyConverters { return (source) -> { List lines = readAllLines(source); Assert.isTrue(!lines.isEmpty() && lines.get(0).startsWith(X509_PEM_HEADER), - "Key is not in PEM-encoded X.509 format, " + "please check that the header begins with -----" + "Key is not in PEM-encoded X.509 format, please check that the header begins with -----" + X509_PEM_HEADER + "-----"); StringBuilder base64Encoded = new StringBuilder(); for (String line : lines) { @@ -114,7 +113,6 @@ public final class RsaKeyConverters { } } byte[] x509 = Base64.getDecoder().decode(base64Encoded.toString()); - try { return (RSAPublicKey) keyFactory.generatePublic(new X509EncodedKeySpec(x509)); } diff --git a/core/src/main/java/org/springframework/security/core/SpringSecurityCoreVersion.java b/core/src/main/java/org/springframework/security/core/SpringSecurityCoreVersion.java index d8c6fadf92..e67e0cafd3 100644 --- a/core/src/main/java/org/springframework/security/core/SpringSecurityCoreVersion.java +++ b/core/src/main/java/org/springframework/security/core/SpringSecurityCoreVersion.java @@ -53,14 +53,6 @@ public final class SpringSecurityCoreVersion { private SpringSecurityCoreVersion() { } - public static String getVersion() { - Package pkg = SpringSecurityCoreVersion.class.getPackage(); - return (pkg != null) ? pkg.getImplementationVersion() : null; - } - - /** - * Performs version checks - */ private static void performVersionChecks() { performVersionChecks(MIN_SPRING_VERSION); } @@ -76,11 +68,9 @@ public final class SpringSecurityCoreVersion { // Check Spring Compatibility String springVersion = SpringVersion.getVersion(); String version = getVersion(); - if (disableChecks(springVersion, version)) { return; } - logger.info("You are running with Spring Security Core " + version); if (new ComparableVersion(springVersion).compareTo(new ComparableVersion(minSpringVersion)) < 0) { logger.warn("**** You are advised to use Spring " + minSpringVersion @@ -88,6 +78,11 @@ public final class SpringSecurityCoreVersion { } } + public static String getVersion() { + Package pkg = SpringSecurityCoreVersion.class.getPackage(); + return (pkg != null) ? pkg.getImplementationVersion() : null; + } + /** * Disable if springVersion and springSecurityVersion are the same to allow working * with Uber Jars. diff --git a/core/src/main/java/org/springframework/security/core/authority/AuthorityUtils.java b/core/src/main/java/org/springframework/security/core/authority/AuthorityUtils.java index 1c69e4dc9f..babadbca1d 100644 --- a/core/src/main/java/org/springframework/security/core/authority/AuthorityUtils.java +++ b/core/src/main/java/org/springframework/security/core/authority/AuthorityUtils.java @@ -34,10 +34,13 @@ import org.springframework.util.StringUtils; * * @author Luke Taylor */ -public abstract class AuthorityUtils { +public final class AuthorityUtils { public static final List NO_AUTHORITIES = Collections.emptyList(); + private AuthorityUtils() { + } + /** * Creates a array of GrantedAuthority objects from a comma-separated string * representation (e.g. "ROLE_A, ROLE_B, ROLE_C"). @@ -56,11 +59,9 @@ public abstract class AuthorityUtils { public static Set authorityListToSet(Collection userAuthorities) { Assert.notNull(userAuthorities, "userAuthorities cannot be null"); Set set = new HashSet<>(userAuthorities.size()); - for (GrantedAuthority authority : userAuthorities) { set.add(authority.getAuthority()); } - return set; } @@ -71,11 +72,9 @@ public abstract class AuthorityUtils { */ public static List createAuthorityList(String... authorities) { List grantedAuthorities = new ArrayList<>(authorities.length); - for (String authority : authorities) { grantedAuthorities.add(new SimpleGrantedAuthority(authority)); } - return grantedAuthorities; } diff --git a/core/src/main/java/org/springframework/security/core/authority/SimpleGrantedAuthority.java b/core/src/main/java/org/springframework/security/core/authority/SimpleGrantedAuthority.java index d5ae07612e..71719d4801 100644 --- a/core/src/main/java/org/springframework/security/core/authority/SimpleGrantedAuthority.java +++ b/core/src/main/java/org/springframework/security/core/authority/SimpleGrantedAuthority.java @@ -50,11 +50,9 @@ public final class SimpleGrantedAuthority implements GrantedAuthority { if (this == obj) { return true; } - if (obj instanceof SimpleGrantedAuthority) { return this.role.equals(((SimpleGrantedAuthority) obj).role); } - return false; } diff --git a/core/src/main/java/org/springframework/security/core/authority/mapping/MapBasedAttributes2GrantedAuthoritiesMapper.java b/core/src/main/java/org/springframework/security/core/authority/mapping/MapBasedAttributes2GrantedAuthoritiesMapper.java index 733cb2e79a..263973f5d2 100755 --- a/core/src/main/java/org/springframework/security/core/authority/mapping/MapBasedAttributes2GrantedAuthoritiesMapper.java +++ b/core/src/main/java/org/springframework/security/core/authority/mapping/MapBasedAttributes2GrantedAuthoritiesMapper.java @@ -58,16 +58,15 @@ public class MapBasedAttributes2GrantedAuthoritiesMapper */ @Override public List getGrantedAuthorities(Collection attributes) { - ArrayList gaList = new ArrayList<>(); + ArrayList result = new ArrayList<>(); for (String attribute : attributes) { - Collection c = this.attributes2grantedAuthoritiesMap.get(attribute); - if (c != null) { - gaList.addAll(c); + Collection granted = this.attributes2grantedAuthoritiesMap.get(attribute); + if (granted != null) { + result.addAll(granted); } } - gaList.trimToSize(); - - return gaList; + result.trimToSize(); + return result; } /** @@ -85,7 +84,6 @@ public class MapBasedAttributes2GrantedAuthoritiesMapper Assert.notEmpty(attributes2grantedAuthoritiesMap, "A non-empty attributes2grantedAuthoritiesMap must be supplied"); this.attributes2grantedAuthoritiesMap = preProcessMap(attributes2grantedAuthoritiesMap); - this.mappableAttributes = Collections.unmodifiableSet(this.attributes2grantedAuthoritiesMap.keySet()); } @@ -96,7 +94,6 @@ public class MapBasedAttributes2GrantedAuthoritiesMapper */ private Map> preProcessMap(Map orgMap) { Map> result = new HashMap<>(orgMap.size()); - for (Map.Entry entry : orgMap.entrySet()) { Assert.isInstanceOf(String.class, entry.getKey(), "attributes2grantedAuthoritiesMap contains non-String objects as keys"); @@ -156,11 +153,11 @@ public class MapBasedAttributes2GrantedAuthoritiesMapper } private void addGrantedAuthorityCollection(Collection result, String value) { - StringTokenizer st = new StringTokenizer(value, this.stringSeparator, false); - while (st.hasMoreTokens()) { - String nextToken = st.nextToken(); - if (StringUtils.hasText(nextToken)) { - result.add(new SimpleGrantedAuthority(nextToken)); + StringTokenizer tokenizer = new StringTokenizer(value, this.stringSeparator, false); + while (tokenizer.hasMoreTokens()) { + String token = tokenizer.nextToken(); + if (StringUtils.hasText(token)) { + result.add(new SimpleGrantedAuthority(token)); } } } diff --git a/core/src/main/java/org/springframework/security/core/authority/mapping/SimpleAuthorityMapper.java b/core/src/main/java/org/springframework/security/core/authority/mapping/SimpleAuthorityMapper.java index 577dff4990..2bb66a73b6 100644 --- a/core/src/main/java/org/springframework/security/core/authority/mapping/SimpleAuthorityMapper.java +++ b/core/src/main/java/org/springframework/security/core/authority/mapping/SimpleAuthorityMapper.java @@ -63,11 +63,9 @@ public final class SimpleAuthorityMapper implements GrantedAuthoritiesMapper, In for (GrantedAuthority authority : authorities) { mapped.add(mapAuthority(authority.getAuthority())); } - if (this.defaultAuthority != null) { mapped.add(this.defaultAuthority); } - return mapped; } @@ -78,11 +76,9 @@ public final class SimpleAuthorityMapper implements GrantedAuthoritiesMapper, In else if (this.convertToLowerCase) { name = name.toLowerCase(); } - if (this.prefix.length() > 0 && !name.startsWith(this.prefix)) { name = this.prefix + name; } - return new SimpleGrantedAuthority(name); } diff --git a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java index 141c1d6354..d8367c4ebd 100644 --- a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java @@ -41,7 +41,6 @@ final class GlobalSecurityContextHolderStrategy implements SecurityContextHolder if (contextHolder == null) { contextHolder = new SecurityContextImpl(); } - return contextHolder; } diff --git a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java index fac2ce9875..cb415500ca 100644 --- a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java @@ -37,12 +37,10 @@ final class InheritableThreadLocalSecurityContextHolderStrategy implements Secur @Override public SecurityContext getContext() { SecurityContext ctx = contextHolder.get(); - if (ctx == null) { ctx = createEmptyContext(); contextHolder.set(ctx); } - return ctx; } diff --git a/core/src/main/java/org/springframework/security/core/context/ReactiveSecurityContextHolder.java b/core/src/main/java/org/springframework/security/core/context/ReactiveSecurityContextHolder.java index 449d461fd7..279ccfed8c 100644 --- a/core/src/main/java/org/springframework/security/core/context/ReactiveSecurityContextHolder.java +++ b/core/src/main/java/org/springframework/security/core/context/ReactiveSecurityContextHolder.java @@ -41,8 +41,16 @@ public final class ReactiveSecurityContextHolder { * @return the {@code Mono} */ public static Mono getContext() { - return Mono.subscriberContext().filter((c) -> c.hasKey(SECURITY_CONTEXT_KEY)) - .flatMap((c) -> c.>get(SECURITY_CONTEXT_KEY)); + return Mono.subscriberContext().filter(ReactiveSecurityContextHolder::hasSecurityContext) + .flatMap(ReactiveSecurityContextHolder::getSecurityContext); + } + + private static boolean hasSecurityContext(Context context) { + return context.hasKey(SECURITY_CONTEXT_KEY); + } + + private static Mono getSecurityContext(Context context) { + return context.>get(SECURITY_CONTEXT_KEY); } /** diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java index d84fbf9419..810edef7c9 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java @@ -67,6 +67,34 @@ public class SecurityContextHolder { initialize(); } + private static void initialize() { + if (!StringUtils.hasText(strategyName)) { + // Set default + strategyName = MODE_THREADLOCAL; + } + if (strategyName.equals(MODE_THREADLOCAL)) { + strategy = new ThreadLocalSecurityContextHolderStrategy(); + } + else if (strategyName.equals(MODE_INHERITABLETHREADLOCAL)) { + strategy = new InheritableThreadLocalSecurityContextHolderStrategy(); + } + else if (strategyName.equals(MODE_GLOBAL)) { + strategy = new GlobalSecurityContextHolderStrategy(); + } + else { + // Try to load a custom strategy + try { + Class clazz = Class.forName(strategyName); + Constructor customStrategy = clazz.getConstructor(); + strategy = (SecurityContextHolderStrategy) customStrategy.newInstance(); + } + catch (Exception ex) { + ReflectionUtils.handleReflectionException(ex); + } + } + initializeCount++; + } + /** * Explicitly clears the context value from the current thread. */ @@ -92,36 +120,6 @@ public class SecurityContextHolder { return initializeCount; } - private static void initialize() { - if (!StringUtils.hasText(strategyName)) { - // Set default - strategyName = MODE_THREADLOCAL; - } - - if (strategyName.equals(MODE_THREADLOCAL)) { - strategy = new ThreadLocalSecurityContextHolderStrategy(); - } - else if (strategyName.equals(MODE_INHERITABLETHREADLOCAL)) { - strategy = new InheritableThreadLocalSecurityContextHolderStrategy(); - } - else if (strategyName.equals(MODE_GLOBAL)) { - strategy = new GlobalSecurityContextHolderStrategy(); - } - else { - // Try to load a custom strategy - try { - Class clazz = Class.forName(strategyName); - Constructor customStrategy = clazz.getConstructor(); - strategy = (SecurityContextHolderStrategy) customStrategy.newInstance(); - } - catch (Exception ex) { - ReflectionUtils.handleReflectionException(ex); - } - } - - initializeCount++; - } - /** * Associates a new SecurityContext with the current thread of execution. * @param context the new SecurityContext (may not be null) diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextImpl.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextImpl.java index e38de5e3e2..19c18abb24 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextImpl.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextImpl.java @@ -18,6 +18,7 @@ package org.springframework.security.core.context; import org.springframework.security.core.Authentication; import org.springframework.security.core.SpringSecurityCoreVersion; +import org.springframework.util.ObjectUtils; /** * Base implementation of {@link SecurityContext}. @@ -42,18 +43,15 @@ public class SecurityContextImpl implements SecurityContext { @Override public boolean equals(Object obj) { if (obj instanceof SecurityContextImpl) { - SecurityContextImpl test = (SecurityContextImpl) obj; - - if ((this.getAuthentication() == null) && (test.getAuthentication() == null)) { + SecurityContextImpl other = (SecurityContextImpl) obj; + if ((this.getAuthentication() == null) && (other.getAuthentication() == null)) { return true; } - - if ((this.getAuthentication() != null) && (test.getAuthentication() != null) - && this.getAuthentication().equals(test.getAuthentication())) { + if ((this.getAuthentication() != null) && (other.getAuthentication() != null) + && this.getAuthentication().equals(other.getAuthentication())) { return true; } } - return false; } @@ -64,12 +62,7 @@ public class SecurityContextImpl implements SecurityContext { @Override public int hashCode() { - if (this.authentication == null) { - return -1; - } - else { - return this.authentication.hashCode(); - } + return ObjectUtils.nullSafeHashCode(this.authentication); } @Override @@ -81,14 +74,12 @@ public class SecurityContextImpl implements SecurityContext { public String toString() { StringBuilder sb = new StringBuilder(); sb.append(super.toString()); - if (this.authentication == null) { sb.append(": Null authentication"); } else { sb.append(": Authentication: ").append(this.authentication); } - return sb.toString(); } diff --git a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java index 88506f1a18..801f5c8207 100644 --- a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java @@ -38,12 +38,10 @@ final class ThreadLocalSecurityContextHolderStrategy implements SecurityContextH @Override public SecurityContext getContext() { SecurityContext ctx = contextHolder.get(); - if (ctx == null) { ctx = createEmptyContext(); contextHolder.set(ctx); } - return ctx; } diff --git a/core/src/main/java/org/springframework/security/core/parameters/AnnotationParameterNameDiscoverer.java b/core/src/main/java/org/springframework/security/core/parameters/AnnotationParameterNameDiscoverer.java index d2f0f6f0c8..18d8099705 100644 --- a/core/src/main/java/org/springframework/security/core/parameters/AnnotationParameterNameDiscoverer.java +++ b/core/src/main/java/org/springframework/security/core/parameters/AnnotationParameterNameDiscoverer.java @@ -88,6 +88,11 @@ import org.springframework.util.ReflectionUtils; */ public class AnnotationParameterNameDiscoverer implements ParameterNameDiscoverer { + private static final ParameterNameFactory> CONSTRUCTOR_METHODPARAM_FACTORY = ( + constructor) -> constructor.getParameterAnnotations(); + + private static final ParameterNameFactory METHOD_METHODPARAM_FACTORY = Method::getParameterAnnotations; + private final Set annotationClassesToUse; public AnnotationParameterNameDiscoverer(String... annotationClassToUse) { @@ -162,12 +167,6 @@ public class AnnotationParameterNameDiscoverer implements ParameterNameDiscovere return null; } - private static final ParameterNameFactory> CONSTRUCTOR_METHODPARAM_FACTORY = ( - constructor) -> constructor.getParameterAnnotations(); - - private static final ParameterNameFactory METHOD_METHODPARAM_FACTORY = (method) -> method - .getParameterAnnotations(); - /** * Strategy interface for looking up the parameter names. * @@ -175,6 +174,7 @@ public class AnnotationParameterNameDiscoverer implements ParameterNameDiscovere * @author Rob Winch * @since 3.2 */ + @FunctionalInterface private interface ParameterNameFactory { /** diff --git a/core/src/main/java/org/springframework/security/core/parameters/DefaultSecurityParameterNameDiscoverer.java b/core/src/main/java/org/springframework/security/core/parameters/DefaultSecurityParameterNameDiscoverer.java index b708508192..7a7b339917 100644 --- a/core/src/main/java/org/springframework/security/core/parameters/DefaultSecurityParameterNameDiscoverer.java +++ b/core/src/main/java/org/springframework/security/core/parameters/DefaultSecurityParameterNameDiscoverer.java @@ -21,9 +21,6 @@ import java.util.HashSet; import java.util.List; import java.util.Set; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; - import org.springframework.core.DefaultParameterNameDiscoverer; import org.springframework.core.LocalVariableTableParameterNameDiscoverer; import org.springframework.core.ParameterNameDiscoverer; @@ -52,8 +49,6 @@ import org.springframework.util.ClassUtils; */ public class DefaultSecurityParameterNameDiscoverer extends PrioritizedParameterNameDiscoverer { - private final Log logger = LogFactory.getLog(getClass()); - private static final String DATA_PARAM_CLASSNAME = "org.springframework.data.repository.query.Param"; private static final boolean DATA_PARAM_PRESENT = ClassUtils.isPresent(DATA_PARAM_CLASSNAME, @@ -79,14 +74,12 @@ public class DefaultSecurityParameterNameDiscoverer extends PrioritizedParameter for (ParameterNameDiscoverer discover : parameterNameDiscovers) { addDiscoverer(discover); } - Set annotationClassesToUse = new HashSet<>(2); annotationClassesToUse.add("org.springframework.security.access.method.P"); annotationClassesToUse.add(P.class.getName()); if (DATA_PARAM_PRESENT) { annotationClassesToUse.add(DATA_PARAM_CLASSNAME); } - addDiscoverer(new AnnotationParameterNameDiscoverer(annotationClassesToUse)); addDiscoverer(new DefaultParameterNameDiscoverer()); } diff --git a/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java b/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java index 0d648d8c38..73623f1bc4 100644 --- a/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java +++ b/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.context.ApplicationListener; +import org.springframework.core.log.LogMessage; import org.springframework.util.Assert; /** @@ -74,33 +75,26 @@ public class SessionRegistryImpl implements SessionRegistry, ApplicationListener @Override public List getAllSessions(Object principal, boolean includeExpiredSessions) { - final Set sessionsUsedByPrincipal = this.principals.get(principal); - + Set sessionsUsedByPrincipal = this.principals.get(principal); if (sessionsUsedByPrincipal == null) { return Collections.emptyList(); } - List list = new ArrayList<>(sessionsUsedByPrincipal.size()); - for (String sessionId : sessionsUsedByPrincipal) { SessionInformation sessionInformation = getSessionInformation(sessionId); - if (sessionInformation == null) { continue; } - if (includeExpiredSessions || !sessionInformation.isExpired()) { list.add(sessionInformation); } } - return list; } @Override public SessionInformation getSessionInformation(String sessionId) { Assert.hasText(sessionId, "SessionId required as per interface contract"); - return this.sessionIds.get(sessionId); } @@ -123,9 +117,7 @@ public class SessionRegistryImpl implements SessionRegistry, ApplicationListener @Override public void refreshLastRequest(String sessionId) { Assert.hasText(sessionId, "SessionId required as per interface contract"); - SessionInformation info = getSessionInformation(sessionId); - if (info != null) { info.refreshLastRequest(); } @@ -135,26 +127,19 @@ public class SessionRegistryImpl implements SessionRegistry, ApplicationListener public void registerNewSession(String sessionId, Object principal) { Assert.hasText(sessionId, "SessionId required as per interface contract"); Assert.notNull(principal, "Principal required as per interface contract"); - if (getSessionInformation(sessionId) != null) { removeSessionInformation(sessionId); } - if (this.logger.isDebugEnabled()) { - this.logger.debug("Registering session " + sessionId + ", for principal " + principal); + this.logger.debug(LogMessage.format("Registering session %s, for principal %s", sessionId, principal)); } - this.sessionIds.put(sessionId, new SessionInformation(principal, sessionId, new Date())); - this.principals.compute(principal, (key, sessionsUsedByPrincipal) -> { if (sessionsUsedByPrincipal == null) { sessionsUsedByPrincipal = new CopyOnWriteArraySet<>(); } sessionsUsedByPrincipal.add(sessionId); - - if (this.logger.isTraceEnabled()) { - this.logger.trace("Sessions used by '" + principal + "' : " + sessionsUsedByPrincipal); - } + this.logger.trace(LogMessage.format("Sessions used by '%s' : %s", principal, sessionsUsedByPrincipal)); return sessionsUsedByPrincipal; }); } @@ -162,37 +147,25 @@ public class SessionRegistryImpl implements SessionRegistry, ApplicationListener @Override public void removeSessionInformation(String sessionId) { Assert.hasText(sessionId, "SessionId required as per interface contract"); - SessionInformation info = getSessionInformation(sessionId); - if (info == null) { return; } - if (this.logger.isTraceEnabled()) { this.logger.debug("Removing session " + sessionId + " from set of registered sessions"); } - this.sessionIds.remove(sessionId); - this.principals.computeIfPresent(info.getPrincipal(), (key, sessionsUsedByPrincipal) -> { - if (this.logger.isDebugEnabled()) { - this.logger.debug("Removing session " + sessionId + " from principal's set of registered sessions"); - } - + this.logger.debug( + LogMessage.format("Removing session %s from principal's set of registered sessions", sessionId)); sessionsUsedByPrincipal.remove(sessionId); - if (sessionsUsedByPrincipal.isEmpty()) { // No need to keep object in principals Map anymore - if (this.logger.isDebugEnabled()) { - this.logger.debug("Removing principal " + info.getPrincipal() + " from registry"); - } + this.logger.debug(LogMessage.format("Removing principal %s from registry", info.getPrincipal())); sessionsUsedByPrincipal = null; } - - if (this.logger.isTraceEnabled()) { - this.logger.trace("Sessions used by '" + info.getPrincipal() + "' : " + sessionsUsedByPrincipal); - } + this.logger.trace( + LogMessage.format("Sessions used by '%s' : %s", info.getPrincipal(), sessionsUsedByPrincipal)); return sessionsUsedByPrincipal; }); } diff --git a/core/src/main/java/org/springframework/security/core/token/KeyBasedPersistenceTokenService.java b/core/src/main/java/org/springframework/security/core/token/KeyBasedPersistenceTokenService.java index 4e5297ab5d..cf3f0f0206 100644 --- a/core/src/main/java/org/springframework/security/core/token/KeyBasedPersistenceTokenService.java +++ b/core/src/main/java/org/springframework/security/core/token/KeyBasedPersistenceTokenService.java @@ -89,13 +89,14 @@ public class KeyBasedPersistenceTokenService implements TokenService, Initializi String serverSecret = computeServerSecretApplicableAt(creationTime); String pseudoRandomNumber = generatePseudoRandomNumber(); String content = creationTime + ":" + pseudoRandomNumber + ":" + extendedInformation; + String key = computeKey(serverSecret, content); + return new DefaultToken(key, creationTime, extendedInformation); + } - // Compute key + private String computeKey(String serverSecret, String content) { String sha512Hex = Sha512DigestUtils.shaHex(content + ":" + serverSecret); String keyPayload = content + ":" + sha512Hex; - String key = Utf8.decode(Base64.getEncoder().encode(Utf8.encode(keyPayload))); - - return new DefaultToken(key, creationTime, extendedInformation); + return Utf8.decode(Base64.getEncoder().encode(Utf8.encode(keyPayload))); } @Override @@ -106,18 +107,15 @@ public class KeyBasedPersistenceTokenService implements TokenService, Initializi String[] tokens = StringUtils .delimitedListToStringArray(Utf8.decode(Base64.getDecoder().decode(Utf8.encode(key))), ":"); Assert.isTrue(tokens.length >= 4, () -> "Expected 4 or more tokens but found " + tokens.length); - long creationTime; try { creationTime = Long.decode(tokens[0]); } - catch (NumberFormatException nfe) { + catch (NumberFormatException ex) { throw new IllegalArgumentException("Expected number but found " + tokens[0]); } - String serverSecret = computeServerSecretApplicableAt(creationTime); String pseudoRandomNumber = tokens[1]; - // Permit extendedInfo to itself contain ":" characters StringBuilder extendedInfo = new StringBuilder(); for (int i = 2; i < tokens.length - 1; i++) { @@ -126,14 +124,11 @@ public class KeyBasedPersistenceTokenService implements TokenService, Initializi } extendedInfo.append(tokens[i]); } - String sha1Hex = tokens[tokens.length - 1]; - // Verification String content = creationTime + ":" + pseudoRandomNumber + ":" + extendedInfo.toString(); String expectedSha512Hex = Sha512DigestUtils.shaHex(content + ":" + serverSecret); Assert.isTrue(expectedSha512Hex.equals(sha1Hex), "Key verification failure"); - return new DefaultToken(key, creationTime, extendedInfo.toString()); } diff --git a/core/src/main/java/org/springframework/security/core/token/SecureRandomFactoryBean.java b/core/src/main/java/org/springframework/security/core/token/SecureRandomFactoryBean.java index 2231f82006..2e73c5dcf7 100644 --- a/core/src/main/java/org/springframework/security/core/token/SecureRandomFactoryBean.java +++ b/core/src/main/java/org/springframework/security/core/token/SecureRandomFactoryBean.java @@ -38,19 +38,16 @@ public class SecureRandomFactoryBean implements FactoryBean { @Override public SecureRandom getObject() throws Exception { - SecureRandom rnd = SecureRandom.getInstance(this.algorithm); - + SecureRandom random = SecureRandom.getInstance(this.algorithm); // Request the next bytes, thus eagerly incurring the expense of default // seeding and to prevent the see from replacing the entire state - rnd.nextBytes(new byte[1]); - + random.nextBytes(new byte[1]); if (this.seed != null) { // Seed specified, so use it byte[] seedBytes = FileCopyUtils.copyToByteArray(this.seed.getInputStream()); - rnd.setSeed(seedBytes); + random.setSeed(seedBytes); } - - return rnd; + return random; } @Override diff --git a/core/src/main/java/org/springframework/security/core/userdetails/MapReactiveUserDetailsService.java b/core/src/main/java/org/springframework/security/core/userdetails/MapReactiveUserDetailsService.java index 2b465afda5..430bdbe493 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/MapReactiveUserDetailsService.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/MapReactiveUserDetailsService.java @@ -72,10 +72,15 @@ public class MapReactiveUserDetailsService implements ReactiveUserDetailsService @Override public Mono updatePassword(UserDetails user, String newPassword) { - return Mono.just(user).map((u) -> User.withUserDetails(u).password(newPassword).build()).doOnNext((u) -> { - String key = getKey(user.getUsername()); - this.users.put(key, u); - }); + return Mono.just(user).map((userDetails) -> withNewPassword(userDetails, newPassword)) + .doOnNext((userDetails) -> { + String key = getKey(user.getUsername()); + this.users.put(key, userDetails); + }); + } + + private UserDetails withNewPassword(UserDetails userDetails, String newPassword) { + return User.withUserDetails(userDetails).password(newPassword).build(); } private String getKey(String username) { diff --git a/core/src/main/java/org/springframework/security/core/userdetails/User.java b/core/src/main/java/org/springframework/security/core/userdetails/User.java index d2587154b8..1b88028c8f 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/User.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/User.java @@ -107,11 +107,8 @@ public class User implements UserDetails, CredentialsContainer { public User(String username, String password, boolean enabled, boolean accountNonExpired, boolean credentialsNonExpired, boolean accountNonLocked, Collection authorities) { - - if (((username == null) || "".equals(username)) || (password == null)) { - throw new IllegalArgumentException("Cannot pass null or empty values to constructor"); - } - + Assert.isTrue(username != null && !"".equals(username) && password != null, + "Cannot pass null or empty values to constructor"); this.username = username; this.password = password; this.enabled = enabled; @@ -166,12 +163,10 @@ public class User implements UserDetails, CredentialsContainer { // Ensure array iteration order is predictable (as per // UserDetails.getAuthorities() contract and SEC-717) SortedSet sortedAuthorities = new TreeSet<>(new AuthorityComparator()); - for (GrantedAuthority grantedAuthority : authorities) { Assert.notNull(grantedAuthority, "GrantedAuthority list cannot contain any null elements"); sortedAuthorities.add(grantedAuthority); } - return sortedAuthorities; } @@ -183,9 +178,9 @@ public class User implements UserDetails, CredentialsContainer { * the same principal. */ @Override - public boolean equals(Object rhs) { - if (rhs instanceof User) { - return this.username.equals(((User) rhs).username); + public boolean equals(Object obj) { + if (obj instanceof User) { + return this.username.equals(((User) obj).username); } return false; } @@ -208,24 +203,20 @@ public class User implements UserDetails, CredentialsContainer { sb.append("AccountNonExpired: ").append(this.accountNonExpired).append("; "); sb.append("credentialsNonExpired: ").append(this.credentialsNonExpired).append("; "); sb.append("AccountNonLocked: ").append(this.accountNonLocked).append("; "); - if (!this.authorities.isEmpty()) { sb.append("Granted Authorities: "); - boolean first = true; for (GrantedAuthority auth : this.authorities) { if (!first) { sb.append(","); } first = false; - sb.append(auth); } } else { sb.append("Not granted any authorities"); } - return sb.toString(); } @@ -303,8 +294,8 @@ public class User implements UserDetails, CredentialsContainer { */ @Deprecated public static UserBuilder withDefaultPasswordEncoder() { - logger.warn( - "User.withDefaultPasswordEncoder() is considered unsafe for production and is only intended for sample applications."); + logger.warn("User.withDefaultPasswordEncoder() is considered unsafe for production " + + "and is only intended for sample applications."); PasswordEncoder encoder = PasswordEncoderFactories.createDelegatingPasswordEncoder(); return builder().passwordEncoder(encoder::encode); } @@ -323,17 +314,14 @@ public class User implements UserDetails, CredentialsContainer { @Override public int compare(GrantedAuthority g1, GrantedAuthority g2) { // Neither should ever be null as each entry is checked before adding it to - // the set. - // If the authority is null, it is a custom authority and should precede - // others. + // the set. If the authority is null, it is a custom authority and should + // precede others. if (g2.getAuthority() == null) { return -1; } - if (g1.getAuthority() == null) { return 1; } - return g1.getAuthority().compareTo(g2.getAuthority()); } diff --git a/core/src/main/java/org/springframework/security/core/userdetails/cache/EhCacheBasedUserCache.java b/core/src/main/java/org/springframework/security/core/userdetails/cache/EhCacheBasedUserCache.java index b61e3a135e..201199ffeb 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/cache/EhCacheBasedUserCache.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/cache/EhCacheBasedUserCache.java @@ -22,6 +22,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.InitializingBean; +import org.springframework.core.log.LogMessage; import org.springframework.security.core.userdetails.UserCache; import org.springframework.security.core.userdetails.UserDetails; import org.springframework.util.Assert; @@ -50,35 +51,19 @@ public class EhCacheBasedUserCache implements UserCache, InitializingBean { @Override public UserDetails getUserFromCache(String username) { Element element = this.cache.get(username); - - if (logger.isDebugEnabled()) { - logger.debug("Cache hit: " + (element != null) + "; username: " + username); - } - - if (element == null) { - return null; - } - else { - return (UserDetails) element.getValue(); - } + logger.debug(LogMessage.of(() -> "Cache hit: " + (element != null) + "; username: " + username)); + return (element != null) ? (UserDetails) element.getValue() : null; } @Override public void putUserInCache(UserDetails user) { Element element = new Element(user.getUsername(), user); - - if (logger.isDebugEnabled()) { - logger.debug("Cache put: " + element.getKey()); - } - + logger.debug(LogMessage.of(() -> "Cache put: " + element.getKey())); this.cache.put(element); } public void removeUserFromCache(UserDetails user) { - if (logger.isDebugEnabled()) { - logger.debug("Cache remove: " + user.getUsername()); - } - + logger.debug(LogMessage.of(() -> "Cache remove: " + user.getUsername())); this.removeUserFromCache(user.getUsername()); } diff --git a/core/src/main/java/org/springframework/security/core/userdetails/cache/SpringCacheBasedUserCache.java b/core/src/main/java/org/springframework/security/core/userdetails/cache/SpringCacheBasedUserCache.java index 50f4b46464..f0cae216e5 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/cache/SpringCacheBasedUserCache.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/cache/SpringCacheBasedUserCache.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.cache.Cache; +import org.springframework.core.log.LogMessage; import org.springframework.security.core.userdetails.UserCache; import org.springframework.security.core.userdetails.UserDetails; import org.springframework.util.Assert; @@ -44,32 +45,18 @@ public class SpringCacheBasedUserCache implements UserCache { @Override public UserDetails getUserFromCache(String username) { Cache.ValueWrapper element = (username != null) ? this.cache.get(username) : null; - - if (logger.isDebugEnabled()) { - logger.debug("Cache hit: " + (element != null) + "; username: " + username); - } - - if (element == null) { - return null; - } - else { - return (UserDetails) element.get(); - } + logger.debug(LogMessage.of(() -> "Cache hit: " + (element != null) + "; username: " + username)); + return (element != null) ? (UserDetails) element.get() : null; } @Override public void putUserInCache(UserDetails user) { - if (logger.isDebugEnabled()) { - logger.debug("Cache put: " + user.getUsername()); - } + logger.debug(LogMessage.of(() -> "Cache put: " + user.getUsername())); this.cache.put(user.getUsername(), user); } public void removeUserFromCache(UserDetails user) { - if (logger.isDebugEnabled()) { - logger.debug("Cache remove: " + user.getUsername()); - } - + logger.debug(LogMessage.of(() -> "Cache remove: " + user.getUsername())); this.removeUserFromCache(user.getUsername()); } diff --git a/core/src/main/java/org/springframework/security/core/userdetails/jdbc/JdbcDaoImpl.java b/core/src/main/java/org/springframework/security/core/userdetails/jdbc/JdbcDaoImpl.java index 565c28b7a9..de2f058080 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/jdbc/JdbcDaoImpl.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/jdbc/JdbcDaoImpl.java @@ -171,37 +171,26 @@ public class JdbcDaoImpl extends JdbcDaoSupport implements UserDetailsService, M @Override public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { List users = loadUsersByUsername(username); - if (users.size() == 0) { this.logger.debug("Query returned no results for user '" + username + "'"); - throw new UsernameNotFoundException(this.messages.getMessage("JdbcDaoImpl.notFound", new Object[] { username }, "Username {0} not found")); } - UserDetails user = users.get(0); // contains no GrantedAuthority[] - Set dbAuthsSet = new HashSet<>(); - if (this.enableAuthorities) { dbAuthsSet.addAll(loadUserAuthorities(user.getUsername())); } - if (this.enableGroups) { dbAuthsSet.addAll(loadGroupAuthorities(user.getUsername())); } - List dbAuths = new ArrayList<>(dbAuthsSet); - addCustomAuthorities(user.getUsername(), dbAuths); - if (dbAuths.size() == 0) { this.logger.debug("User '" + username + "' has no authorities and will be treated as 'not found'"); - throw new UsernameNotFoundException(this.messages.getMessage("JdbcDaoImpl.noAuthority", new Object[] { username }, "User {0} has no GrantedAuthority")); } - return createUserDetails(username, user, dbAuths); } @@ -225,7 +214,6 @@ public class JdbcDaoImpl extends JdbcDaoSupport implements UserDetailsService, M protected List loadUserAuthorities(String username) { return getJdbcTemplate().query(this.authoritiesByUsernameQuery, new String[] { username }, (rs, rowNum) -> { String roleName = JdbcDaoImpl.this.rolePrefix + rs.getString(2); - return new SimpleGrantedAuthority(roleName); }); } @@ -239,7 +227,6 @@ public class JdbcDaoImpl extends JdbcDaoSupport implements UserDetailsService, M return getJdbcTemplate().query(this.groupAuthoritiesByUsernameQuery, new String[] { username }, (rs, rowNum) -> { String roleName = getRolePrefix() + rs.getString(3); - return new SimpleGrantedAuthority(roleName); }); } @@ -256,11 +243,9 @@ public class JdbcDaoImpl extends JdbcDaoSupport implements UserDetailsService, M protected UserDetails createUserDetails(String username, UserDetails userFromUserQuery, List combinedAuthorities) { String returnUsername = userFromUserQuery.getUsername(); - if (!this.usernameBasedPrimaryKey) { returnUsername = username; } - return new User(returnUsername, userFromUserQuery.getPassword(), userFromUserQuery.isEnabled(), userFromUserQuery.isAccountNonExpired(), userFromUserQuery.isCredentialsNonExpired(), userFromUserQuery.isAccountNonLocked(), combinedAuthorities); diff --git a/core/src/main/java/org/springframework/security/core/userdetails/memory/UserAttributeEditor.java b/core/src/main/java/org/springframework/security/core/userdetails/memory/UserAttributeEditor.java index 163c502bbb..09f10c18d0 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/memory/UserAttributeEditor.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/memory/UserAttributeEditor.java @@ -32,42 +32,32 @@ public class UserAttributeEditor extends PropertyEditorSupport { @Override public void setAsText(String s) throws IllegalArgumentException { - if (StringUtils.hasText(s)) { - String[] tokens = StringUtils.commaDelimitedListToStringArray(s); - UserAttribute userAttrib = new UserAttribute(); - - List authoritiesAsStrings = new ArrayList<>(); - - for (int i = 0; i < tokens.length; i++) { - String currentToken = tokens[i].trim(); - - if (i == 0) { - userAttrib.setPassword(currentToken); - } - else { - if (currentToken.toLowerCase().equals("enabled")) { - userAttrib.setEnabled(true); - } - else if (currentToken.toLowerCase().equals("disabled")) { - userAttrib.setEnabled(false); - } - else { - authoritiesAsStrings.add(currentToken); - } - } - } - userAttrib.setAuthoritiesAsString(authoritiesAsStrings); - - if (userAttrib.isValid()) { - setValue(userAttrib); + if (!StringUtils.hasText(s)) { + setValue(null); + return; + } + String[] tokens = StringUtils.commaDelimitedListToStringArray(s); + UserAttribute userAttrib = new UserAttribute(); + List authoritiesAsStrings = new ArrayList<>(); + for (int i = 0; i < tokens.length; i++) { + String currentToken = tokens[i].trim(); + if (i == 0) { + userAttrib.setPassword(currentToken); } else { - setValue(null); + if (currentToken.toLowerCase().equals("enabled")) { + userAttrib.setEnabled(true); + } + else if (currentToken.toLowerCase().equals("disabled")) { + userAttrib.setEnabled(false); + } + else { + authoritiesAsStrings.add(currentToken); + } } } - else { - setValue(null); - } + userAttrib.setAuthoritiesAsString(authoritiesAsStrings); + setValue(userAttrib.isValid() ? userAttrib : null); } } diff --git a/core/src/main/java/org/springframework/security/jackson2/SecurityJackson2Modules.java b/core/src/main/java/org/springframework/security/jackson2/SecurityJackson2Modules.java index 03e24ae4a7..febd2b755c 100644 --- a/core/src/main/java/org/springframework/security/jackson2/SecurityJackson2Modules.java +++ b/core/src/main/java/org/springframework/security/jackson2/SecurityJackson2Modules.java @@ -42,6 +42,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.core.annotation.AnnotationUtils; +import org.springframework.core.log.LogMessage; import org.springframework.util.ClassUtils; /** @@ -97,22 +98,17 @@ public final class SecurityJackson2Modules { @SuppressWarnings("unchecked") private static Module loadAndGetInstance(String className, ClassLoader loader) { - Module instance = null; try { Class securityModule = (Class) ClassUtils.forName(className, loader); if (securityModule != null) { - if (logger.isDebugEnabled()) { - logger.debug("Loaded module " + className + ", now registering"); - } - instance = securityModule.newInstance(); + logger.debug(LogMessage.format("Loaded module %s, now registering", className)); + return securityModule.newInstance(); } } catch (Exception ex) { - if (logger.isDebugEnabled()) { - logger.debug("Cannot load module " + className, ex); - } + logger.debug(LogMessage.format("Cannot load module %s", className), ex); } - return instance; + return null; } /** @@ -193,12 +189,23 @@ public final class SecurityJackson2Modules { */ static class AllowlistTypeIdResolver implements TypeIdResolver { - private static final Set ALLOWLIST_CLASS_NAMES = Collections - .unmodifiableSet(new HashSet(Arrays.asList("java.util.ArrayList", "java.util.Collections$EmptyList", - "java.util.Collections$EmptyMap", "java.util.Collections$UnmodifiableRandomAccessList", - "java.util.Collections$SingletonList", "java.util.Date", "java.time.Instant", "java.net.URL", - "java.util.TreeMap", "java.util.HashMap", "java.util.LinkedHashMap", - "org.springframework.security.core.context.SecurityContextImpl"))); + private static final Set ALLOWLIST_CLASS_NAMES; + static { + Set names = new HashSet<>(); + names.add("java.util.ArrayList"); + names.add("java.util.Collections$EmptyList"); + names.add("java.util.Collections$EmptyMap"); + names.add("java.util.Collections$UnmodifiableRandomAccessList"); + names.add("java.util.Collections$SingletonList"); + names.add("java.util.Date"); + names.add("java.time.Instant"); + names.add("java.net.URL"); + names.add("java.util.TreeMap"); + names.add("java.util.HashMap"); + names.add("java.util.LinkedHashMap"); + names.add("org.springframework.security.core.context.SecurityContextImpl"); + ALLOWLIST_CLASS_NAMES = Collections.unmodifiableSet(names); + } private final TypeIdResolver delegate; diff --git a/core/src/main/java/org/springframework/security/jackson2/UserDeserializer.java b/core/src/main/java/org/springframework/security/jackson2/UserDeserializer.java index fd91f1ad9f..8363acc332 100644 --- a/core/src/main/java/org/springframework/security/jackson2/UserDeserializer.java +++ b/core/src/main/java/org/springframework/security/jackson2/UserDeserializer.java @@ -42,6 +42,9 @@ import org.springframework.security.core.userdetails.User; */ class UserDeserializer extends JsonDeserializer { + private static final TypeReference> SIMPLE_GRANTED_AUTHORITY_SET = new TypeReference>() { + }; + /** * This method will create {@link User} object. It will ensure successful object * creation even if password key is null in serialized json, because credentials may @@ -58,15 +61,17 @@ class UserDeserializer extends JsonDeserializer { ObjectMapper mapper = (ObjectMapper) jp.getCodec(); JsonNode jsonNode = mapper.readTree(jp); Set authorities = mapper.convertValue(jsonNode.get("authorities"), - new TypeReference>() { - }); - JsonNode password = readJsonNode(jsonNode, "password"); - User result = new User(readJsonNode(jsonNode, "username").asText(), password.asText(""), - readJsonNode(jsonNode, "enabled").asBoolean(), readJsonNode(jsonNode, "accountNonExpired").asBoolean(), - readJsonNode(jsonNode, "credentialsNonExpired").asBoolean(), - readJsonNode(jsonNode, "accountNonLocked").asBoolean(), authorities); - - if (password.asText(null) == null) { + SIMPLE_GRANTED_AUTHORITY_SET); + JsonNode passwordNode = readJsonNode(jsonNode, "password"); + String username = readJsonNode(jsonNode, "username").asText(); + String password = passwordNode.asText(""); + boolean enabled = readJsonNode(jsonNode, "enabled").asBoolean(); + boolean accountNonExpired = readJsonNode(jsonNode, "accountNonExpired").asBoolean(); + boolean credentialsNonExpired = readJsonNode(jsonNode, "credentialsNonExpired").asBoolean(); + boolean accountNonLocked = readJsonNode(jsonNode, "accountNonLocked").asBoolean(); + User result = new User(username, password, enabled, accountNonExpired, credentialsNonExpired, accountNonLocked, + authorities); + if (passwordNode.asText(null) == null) { result.eraseCredentials(); } return result; diff --git a/core/src/main/java/org/springframework/security/jackson2/UsernamePasswordAuthenticationTokenDeserializer.java b/core/src/main/java/org/springframework/security/jackson2/UsernamePasswordAuthenticationTokenDeserializer.java index 25e47eac9b..c5d815ad79 100644 --- a/core/src/main/java/org/springframework/security/jackson2/UsernamePasswordAuthenticationTokenDeserializer.java +++ b/core/src/main/java/org/springframework/security/jackson2/UsernamePasswordAuthenticationTokenDeserializer.java @@ -19,11 +19,13 @@ package org.springframework.security.jackson2; import java.io.IOException; import java.util.List; +import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.MissingNode; @@ -48,6 +50,12 @@ import org.springframework.security.core.GrantedAuthority; */ class UsernamePasswordAuthenticationTokenDeserializer extends JsonDeserializer { + private static final TypeReference> GRANTED_AUTHORITY_LIST = new TypeReference>() { + }; + + private static final TypeReference OBJECT = new TypeReference() { + }; + /** * This method construct {@link UsernamePasswordAuthenticationToken} object from * serialized json. @@ -60,47 +68,44 @@ class UsernamePasswordAuthenticationTokenDeserializer extends JsonDeserializer authorities = mapper.readValue(readJsonNode(jsonNode, "authorities").traverse(mapper), - new TypeReference>() { - }); - if (authenticated) { - token = new UsernamePasswordAuthenticationToken(principal, credentials, authorities); - } - else { - token = new UsernamePasswordAuthenticationToken(principal, credentials); - } + GRANTED_AUTHORITY_LIST); + UsernamePasswordAuthenticationToken token = (!authenticated) + ? new UsernamePasswordAuthenticationToken(principal, credentials) + : new UsernamePasswordAuthenticationToken(principal, credentials, authorities); JsonNode detailsNode = readJsonNode(jsonNode, "details"); if (detailsNode.isNull() || detailsNode.isMissingNode()) { token.setDetails(null); } else { - Object details = mapper.readValue(detailsNode.toString(), new TypeReference() { - }); + Object details = mapper.readValue(detailsNode.toString(), OBJECT); token.setDetails(details); } return token; } + private Object getCredentials(JsonNode credentialsNode) { + if (credentialsNode.isNull() || credentialsNode.isMissingNode()) { + return null; + } + return credentialsNode.asText(); + } + + private Object getPrincipal(ObjectMapper mapper, JsonNode principalNode) + throws IOException, JsonParseException, JsonMappingException { + if (principalNode.isObject()) { + return mapper.readValue(principalNode.traverse(mapper), Object.class); + } + return principalNode.asText(); + } + private JsonNode readJsonNode(JsonNode jsonNode, String field) { return jsonNode.has(field) ? jsonNode.get(field) : MissingNode.getInstance(); } diff --git a/core/src/main/java/org/springframework/security/provisioning/InMemoryUserDetailsManager.java b/core/src/main/java/org/springframework/security/provisioning/InMemoryUserDetailsManager.java index 762a0e3785..346c481cf8 100644 --- a/core/src/main/java/org/springframework/security/provisioning/InMemoryUserDetailsManager.java +++ b/core/src/main/java/org/springframework/security/provisioning/InMemoryUserDetailsManager.java @@ -25,6 +25,7 @@ import java.util.Properties; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; @@ -74,21 +75,21 @@ public class InMemoryUserDetailsManager implements UserDetailsManager, UserDetai public InMemoryUserDetailsManager(Properties users) { Enumeration names = users.propertyNames(); UserAttributeEditor editor = new UserAttributeEditor(); - while (names.hasMoreElements()) { String name = (String) names.nextElement(); editor.setAsText(users.getProperty(name)); UserAttribute attr = (UserAttribute) editor.getValue(); - UserDetails user = new User(name, attr.getPassword(), attr.isEnabled(), true, true, true, - attr.getAuthorities()); - createUser(user); + createUser(createUserDetails(name, attr)); } } + private User createUserDetails(String name, UserAttribute attr) { + return new User(name, attr.getPassword(), attr.isEnabled(), true, true, true, attr.getAuthorities()); + } + @Override public void createUser(UserDetails user) { Assert.isTrue(!userExists(user.getUsername()), "user should not exist"); - this.users.put(user.getUsername().toLowerCase(), new MutableUser(user)); } @@ -100,7 +101,6 @@ public class InMemoryUserDetailsManager implements UserDetailsManager, UserDetai @Override public void updateUser(UserDetails user) { Assert.isTrue(userExists(user.getUsername()), "user should exist"); - this.users.put(user.getUsername().toLowerCase(), new MutableUser(user)); } @@ -112,34 +112,24 @@ public class InMemoryUserDetailsManager implements UserDetailsManager, UserDetai @Override public void changePassword(String oldPassword, String newPassword) { Authentication currentUser = SecurityContextHolder.getContext().getAuthentication(); - if (currentUser == null) { // This would indicate bad coding somewhere throw new AccessDeniedException( "Can't change password as no Authentication object found in context " + "for current user."); } - String username = currentUser.getName(); - - this.logger.debug("Changing password for user '" + username + "'"); - + this.logger.debug(LogMessage.format("Changing password for user '%s'", username)); // If an authentication manager has been set, re-authenticate the user with the // supplied password. if (this.authenticationManager != null) { - this.logger.debug("Reauthenticating user '" + username + "' for password change request."); - + this.logger.debug(LogMessage.format("Reauthenticating user '%s' for password change request.", username)); this.authenticationManager.authenticate(new UsernamePasswordAuthenticationToken(username, oldPassword)); } else { this.logger.debug("No authentication manager set. Password won't be re-checked."); } - MutableUserDetails user = this.users.get(username); - - if (user == null) { - throw new IllegalStateException("Current user doesn't exist in database."); - } - + Assert.state(user != null, "Current user doesn't exist in database."); user.setPassword(newPassword); } @@ -154,11 +144,9 @@ public class InMemoryUserDetailsManager implements UserDetailsManager, UserDetai @Override public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { UserDetails user = this.users.get(username.toLowerCase()); - if (user == null) { throw new UsernameNotFoundException(username); } - return new User(user.getUsername(), user.getPassword(), user.isEnabled(), user.isAccountNonExpired(), user.isCredentialsNonExpired(), user.isAccountNonLocked(), user.getAuthorities()); } diff --git a/core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java b/core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java index f3d6e7ee8a..070b0f3f13 100644 --- a/core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java +++ b/core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java @@ -16,6 +16,8 @@ package org.springframework.security.provisioning; +import java.sql.ResultSet; +import java.sql.SQLException; import java.util.Collection; import java.util.List; @@ -25,6 +27,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.context.ApplicationContextException; +import org.springframework.core.log.LogMessage; import org.springframework.dao.IncorrectResultSizeDataAccessException; import org.springframework.jdbc.core.PreparedStatementSetter; import org.springframework.security.access.AccessDeniedException; @@ -60,7 +63,6 @@ import org.springframework.util.Assert; */ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsManager, GroupManager { - // UserDetailsManager SQL public static final String DEF_CREATE_USER_SQL = "insert into users (username, password, enabled) values (?,?,?)"; public static final String DEF_DELETE_USER_SQL = "delete from users where username = ?"; @@ -75,7 +77,6 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa public static final String DEF_CHANGE_PASSWORD_SQL = "update users set password = ? where username = ?"; - // GroupManager SQL public static final String DEF_FIND_GROUPS_SQL = "select group_name from groups"; public static final String DEF_FIND_USERS_IN_GROUP_SQL = "select username from group_members gm, groups g " @@ -160,10 +161,9 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa @Override protected void initDao() throws ApplicationContextException { if (this.authenticationManager == null) { - this.logger.info("No authentication manager set. Reauthentication of users when changing passwords will " - + "not be performed."); + this.logger.info( + "No authentication manager set. Reauthentication of users when changing passwords will not be performed."); } - super.initDao(); } @@ -173,36 +173,33 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa */ @Override protected List loadUsersByUsername(String username) { - return getJdbcTemplate().query(getUsersByUsernameQuery(), new String[] { username }, (rs, rowNum) -> { + return getJdbcTemplate().query(getUsersByUsernameQuery(), new String[] { username }, this::mapToUser); + } - String userName = rs.getString(1); - String password = rs.getString(2); - boolean enabled = rs.getBoolean(3); - - boolean accLocked = false; - boolean accExpired = false; - boolean credsExpired = false; - - if (rs.getMetaData().getColumnCount() > 3) { - // NOTE: acc_locked, acc_expired and creds_expired are also to be loaded - accLocked = rs.getBoolean(4); - accExpired = rs.getBoolean(5); - credsExpired = rs.getBoolean(6); - } - return new User(userName, password, enabled, !accExpired, !credsExpired, !accLocked, - AuthorityUtils.NO_AUTHORITIES); - }); + private UserDetails mapToUser(ResultSet rs, int rowNum) throws SQLException { + String userName = rs.getString(1); + String password = rs.getString(2); + boolean enabled = rs.getBoolean(3); + boolean accLocked = false; + boolean accExpired = false; + boolean credsExpired = false; + if (rs.getMetaData().getColumnCount() > 3) { + // NOTE: acc_locked, acc_expired and creds_expired are also to be loaded + accLocked = rs.getBoolean(4); + accExpired = rs.getBoolean(5); + credsExpired = rs.getBoolean(6); + } + return new User(userName, password, enabled, !accExpired, !credsExpired, !accLocked, + AuthorityUtils.NO_AUTHORITIES); } @Override public void createUser(final UserDetails user) { validateUserDetails(user); - getJdbcTemplate().update(this.createUserSql, (ps) -> { ps.setString(1, user.getUsername()); ps.setString(2, user.getPassword()); ps.setBoolean(3, user.isEnabled()); - int paramCount = ps.getParameterMetaData().getParameterCount(); if (paramCount > 3) { // NOTE: acc_locked, acc_expired and creds_expired are also to be inserted @@ -211,7 +208,6 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa ps.setBoolean(6, !user.isCredentialsNonExpired()); } }); - if (getEnableAuthorities()) { insertUserAuthorities(user); } @@ -220,11 +216,9 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa @Override public void updateUser(final UserDetails user) { validateUserDetails(user); - getJdbcTemplate().update(this.updateUserSql, (ps) -> { ps.setString(1, user.getPassword()); ps.setBoolean(2, user.isEnabled()); - int paramCount = ps.getParameterMetaData().getParameterCount(); if (paramCount == 3) { ps.setString(3, user.getUsername()); @@ -234,17 +228,13 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa ps.setBoolean(3, !user.isAccountNonLocked()); ps.setBoolean(4, !user.isAccountNonExpired()); ps.setBoolean(5, !user.isCredentialsNonExpired()); - ps.setString(6, user.getUsername()); } - }); - if (getEnableAuthorities()) { deleteUserAuthorities(user.getUsername()); insertUserAuthorities(user); } - this.userCache.removeUserFromCache(user.getUsername()); } @@ -270,42 +260,32 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa @Override public void changePassword(String oldPassword, String newPassword) throws AuthenticationException { Authentication currentUser = SecurityContextHolder.getContext().getAuthentication(); - if (currentUser == null) { // This would indicate bad coding somewhere throw new AccessDeniedException( "Can't change password as no Authentication object found in context " + "for current user."); } - String username = currentUser.getName(); - // If an authentication manager has been set, re-authenticate the user with the // supplied password. if (this.authenticationManager != null) { - this.logger.debug("Reauthenticating user '" + username + "' for password change request."); - + this.logger.debug(LogMessage.format("Reauthenticating user '%s' for password change request.", username)); this.authenticationManager.authenticate(new UsernamePasswordAuthenticationToken(username, oldPassword)); } else { this.logger.debug("No authentication manager set. Password won't be re-checked."); } - this.logger.debug("Changing password for user '" + username + "'"); - getJdbcTemplate().update(this.changePasswordSql, newPassword, username); - SecurityContextHolder.getContext().setAuthentication(createNewAuthentication(currentUser, newPassword)); - this.userCache.removeUserFromCache(username); } protected Authentication createNewAuthentication(Authentication currentAuth, String newPassword) { UserDetails user = loadUserByUsername(currentAuth.getName()); - UsernamePasswordAuthenticationToken newAuthentication = new UsernamePasswordAuthenticationToken(user, null, user.getAuthorities()); newAuthentication.setDetails(currentAuth.getDetails()); - return newAuthentication; } @@ -313,12 +293,10 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa public boolean userExists(String username) { List users = getJdbcTemplate().queryForList(this.userExistsSql, new String[] { username }, String.class); - if (users.size() > 1) { throw new IncorrectResultSizeDataAccessException("More than one user found with name '" + username + "'", 1); } - return users.size() == 1; } @@ -337,16 +315,12 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa public void createGroup(final String groupName, final List authorities) { Assert.hasText(groupName, "groupName should have text"); Assert.notNull(authorities, "authorities cannot be null"); - this.logger.debug("Creating new group '" + groupName + "' with authorities " + AuthorityUtils.authorityListToSet(authorities)); - getJdbcTemplate().update(this.insertGroupSql, groupName); - - final int groupId = findGroupId(groupName); - + int groupId = findGroupId(groupName); for (GrantedAuthority a : authorities) { - final String authority = a.getAuthority(); + String authority = a.getAuthority(); getJdbcTemplate().update(this.insertGroupAuthoritySql, (ps) -> { ps.setInt(1, groupId); ps.setString(2, authority); @@ -358,8 +332,7 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa public void deleteGroup(String groupName) { this.logger.debug("Deleting group '" + groupName + "'"); Assert.hasText(groupName, "groupName should have text"); - - final int id = findGroupId(groupName); + int id = findGroupId(groupName); PreparedStatementSetter groupIdPSS = (ps) -> ps.setInt(1, id); getJdbcTemplate().update(this.deleteGroupMembersSql, groupIdPSS); getJdbcTemplate().update(this.deleteGroupAuthoritiesSql, groupIdPSS); @@ -371,7 +344,6 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa this.logger.debug("Changing group name from '" + oldName + "' to '" + newName + "'"); Assert.hasText(oldName, "oldName should have text"); Assert.hasText(newName, "newName should have text"); - getJdbcTemplate().update(this.renameGroupSql, newName, oldName); } @@ -380,13 +352,11 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa this.logger.debug("Adding user '" + username + "' to group '" + groupName + "'"); Assert.hasText(username, "username should have text"); Assert.hasText(groupName, "groupName should have text"); - - final int id = findGroupId(groupName); + int id = findGroupId(groupName); getJdbcTemplate().update(this.insertGroupMemberSql, (ps) -> { ps.setInt(1, id); ps.setString(2, username); }); - this.userCache.removeUserFromCache(username); } @@ -395,14 +365,11 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa this.logger.debug("Removing user '" + username + "' to group '" + groupName + "'"); Assert.hasText(username, "username should have text"); Assert.hasText(groupName, "groupName should have text"); - - final int id = findGroupId(groupName); - + int id = findGroupId(groupName); getJdbcTemplate().update(this.deleteGroupMemberSql, (ps) -> { ps.setInt(1, id); ps.setString(2, username); }); - this.userCache.removeUserFromCache(username); } @@ -410,12 +377,13 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa public List findGroupAuthorities(String groupName) { this.logger.debug("Loading authorities for group '" + groupName + "'"); Assert.hasText(groupName, "groupName should have text"); + return getJdbcTemplate().query(this.groupAuthoritiesSql, new String[] { groupName }, + this::mapToGrantedAuthority); + } - return getJdbcTemplate().query(this.groupAuthoritiesSql, new String[] { groupName }, (rs, rowNum) -> { - String roleName = getRolePrefix() + rs.getString(3); - - return new SimpleGrantedAuthority(roleName); - }); + private GrantedAuthority mapToGrantedAuthority(ResultSet rs, int rowNum) throws SQLException { + String roleName = getRolePrefix() + rs.getString(3); + return new SimpleGrantedAuthority(roleName); } @Override @@ -423,9 +391,7 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa this.logger.debug("Removing authority '" + authority + "' from group '" + groupName + "'"); Assert.hasText(groupName, "groupName should have text"); Assert.notNull(authority, "authority cannot be null"); - - final int id = findGroupId(groupName); - + int id = findGroupId(groupName); getJdbcTemplate().update(this.deleteGroupAuthoritySql, (ps) -> { ps.setInt(1, id); ps.setString(2, authority.getAuthority()); @@ -437,8 +403,7 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa this.logger.debug("Adding authority '" + authority + "' to group '" + groupName + "'"); Assert.hasText(groupName, "groupName should have text"); Assert.notNull(authority, "authority cannot be null"); - - final int id = findGroupId(groupName); + int id = findGroupId(groupName); getJdbcTemplate().update(this.insertGroupAuthoritySql, (ps) -> { ps.setInt(1, id); ps.setString(2, authority.getAuthority()); @@ -571,7 +536,6 @@ public class JdbcUserDetailsManager extends JdbcDaoImpl implements UserDetailsMa private void validateAuthorities(Collection authorities) { Assert.notNull(authorities, "Authorities list must not be null"); - for (GrantedAuthority authority : authorities) { Assert.notNull(authority, "Authorities list contains a null entry"); Assert.hasText(authority.getAuthority(), "getAuthority() method must return a non-empty string"); diff --git a/core/src/main/java/org/springframework/security/task/DelegatingSecurityContextAsyncTaskExecutor.java b/core/src/main/java/org/springframework/security/task/DelegatingSecurityContextAsyncTaskExecutor.java index f52d99c86d..3bbba07a0e 100644 --- a/core/src/main/java/org/springframework/security/task/DelegatingSecurityContextAsyncTaskExecutor.java +++ b/core/src/main/java/org/springframework/security/task/DelegatingSecurityContextAsyncTaskExecutor.java @@ -61,20 +61,17 @@ public class DelegatingSecurityContextAsyncTaskExecutor extends DelegatingSecuri @Override public final void execute(Runnable task, long startTimeout) { - task = wrap(task); - getDelegate().execute(task, startTimeout); + getDelegate().execute(wrap(task), startTimeout); } @Override public final Future submit(Runnable task) { - task = wrap(task); - return getDelegate().submit(task); + return getDelegate().submit(wrap(task)); } @Override public final Future submit(Callable task) { - task = wrap(task); - return getDelegate().submit(task); + return getDelegate().submit(wrap(task)); } private AsyncTaskExecutor getDelegate() { diff --git a/core/src/main/java/org/springframework/security/util/FieldUtils.java b/core/src/main/java/org/springframework/security/util/FieldUtils.java index be6de9d0e6..261d264b0d 100644 --- a/core/src/main/java/org/springframework/security/util/FieldUtils.java +++ b/core/src/main/java/org/springframework/security/util/FieldUtils.java @@ -42,16 +42,14 @@ public final class FieldUtils { public static Field getField(Class clazz, String fieldName) throws IllegalStateException { Assert.notNull(clazz, "Class required"); Assert.hasText(fieldName, "Field name required"); - try { return clazz.getDeclaredField(fieldName); } - catch (NoSuchFieldException nsf) { + catch (NoSuchFieldException ex) { // Try superclass if (clazz.getSuperclass() != null) { return getField(clazz.getSuperclass(), fieldName); } - throw new IllegalStateException("Could not locate field '" + fieldName + "' on class " + clazz); } } @@ -68,7 +66,6 @@ public final class FieldUtils { String[] nestedFields = StringUtils.tokenizeToStringArray(fieldName, "."); Class componentClass = bean.getClass(); Object value = bean; - for (String nestedField : nestedFields) { Field field = getField(componentClass, nestedField); field.setAccessible(true); @@ -77,29 +74,24 @@ public final class FieldUtils { componentClass = value.getClass(); } } - return value; } public static Object getProtectedFieldValue(String protectedField, Object object) { Field field = FieldUtils.getField(object.getClass(), protectedField); - try { field.setAccessible(true); - return field.get(object); } catch (Exception ex) { ReflectionUtils.handleReflectionException(ex); - return null; // unreachable - previous line throws exception } } public static void setProtectedFieldValue(String protectedField, Object object, Object newValue) { Field field = FieldUtils.getField(object.getClass(), protectedField); - try { field.setAccessible(true); field.set(object, newValue); diff --git a/core/src/main/java/org/springframework/security/util/MethodInvocationUtils.java b/core/src/main/java/org/springframework/security/util/MethodInvocationUtils.java index 09d44f5760..26ade0d092 100644 --- a/core/src/main/java/org/springframework/security/util/MethodInvocationUtils.java +++ b/core/src/main/java/org/springframework/security/util/MethodInvocationUtils.java @@ -50,19 +50,15 @@ public final class MethodInvocationUtils { */ public static MethodInvocation create(Object object, String methodName, Object... args) { Assert.notNull(object, "Object required"); - Class[] classArgs = null; - if (args != null) { classArgs = new Class[args.length]; - for (int i = 0; i < args.length; i++) { classArgs[i] = args[i].getClass(); } } - - // Determine the type that declares the requested method, taking into account - // proxies + // Determine the type that declares the requested method, + // taking into account proxies Class target = AopUtils.getTargetClass(object); if (object instanceof Advised) { Advised a = (Advised) object; @@ -75,13 +71,12 @@ public final class MethodInvocationUtils { target = possibleInterface; break; } - catch (Exception ignored) { + catch (Exception ex) { // try the next one } } } } - return createFromClass(object, target, methodName, classArgs, args); } @@ -100,21 +95,17 @@ public final class MethodInvocationUtils { * problem */ public static MethodInvocation createFromClass(Class clazz, String methodName) { - MethodInvocation mi = createFromClass(null, clazz, methodName, null, null); - - if (mi == null) { - for (Method m : clazz.getDeclaredMethods()) { - if (m.getName().equals(methodName)) { - if (mi != null) { - throw new IllegalArgumentException( - "The class " + clazz + " has more than one method named" + " '" + methodName + "'"); - } - mi = new SimpleMethodInvocation(null, m); + MethodInvocation invocation = createFromClass(null, clazz, methodName, null, null); + if (invocation == null) { + for (Method method : clazz.getDeclaredMethods()) { + if (method.getName().equals(methodName)) { + Assert.isTrue(invocation == null, + () -> "The class " + clazz + " has more than one method named" + " '" + methodName + "'"); + invocation = new SimpleMethodInvocation(null, method); } } } - - return mi; + return invocation; } /** @@ -134,17 +125,13 @@ public final class MethodInvocationUtils { Class[] classArgs, Object[] args) { Assert.notNull(clazz, "Class required"); Assert.hasText(methodName, "MethodName required"); - - Method method; - try { - method = clazz.getMethod(methodName, classArgs); + Method method = clazz.getMethod(methodName, classArgs); + return new SimpleMethodInvocation(targetObject, method, args); } catch (NoSuchMethodException ex) { return null; } - - return new SimpleMethodInvocation(targetObject, method, args); } }