From a19cc8f1c7d74643aa07872ae38e95d5aee09f87 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Tue, 31 Jul 2012 12:35:23 -0500 Subject: [PATCH] SEC-2020: Set eraseCredentialsAfterAuthentication when using http@authentication-manager-ref Previously the namespace configuration did not properly set the eraseCredentialsAfterAuthentication property on the parent AuthenticationProvider when using http@authentication-manager-ref. Now the ProviderManager that is created by the namespace consults the original AuthenticationManager to determine if eraseCredentialsAfterAuthentication should be set on the wrapped instance. If the original is not a ProviderManager the eraseCredentialsAfterAuthentication is set to false since we should not "magically" add behavior to the custom AuthenticationManager without knowing the desired behavior. --- .../HttpSecurityBeanDefinitionParser.java | 39 ++++++++++++++++++- .../config/http/MiscHttpConfigTests.groovy | 23 +++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java b/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java index 46eabadd3e..8dc80023f1 100644 --- a/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java +++ b/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java @@ -32,6 +32,7 @@ import org.springframework.beans.factory.xml.BeanDefinitionParser; import org.springframework.beans.factory.xml.ParserContext; import org.springframework.core.OrderComparator; import org.springframework.core.Ordered; +import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.ProviderManager; import org.springframework.security.config.BeanIds; import org.springframework.security.config.Elements; @@ -231,7 +232,13 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { authManager.addConstructorArgValue(authenticationProviders); if (StringUtils.hasText(parentMgrRef)) { - authManager.addConstructorArgValue(new RuntimeBeanReference(parentMgrRef)); + RuntimeBeanReference parentAuthManager = new RuntimeBeanReference(parentMgrRef); + authManager.addConstructorArgValue(parentAuthManager); + RootBeanDefinition clearCredentials = new RootBeanDefinition(ClearCredentialsMethodInvokingFactoryBean.class); + clearCredentials.getPropertyValues().addPropertyValue("targetObject", parentAuthManager); + clearCredentials.getPropertyValues().addPropertyValue("targetMethod", "isEraseCredentialsAfterAuthentication"); + + authManager.addPropertyValue("eraseCredentialsAfterAuthentication", clearCredentials); } else { RootBeanDefinition amfb = new RootBeanDefinition(AuthenticationManagerFactoryBean.class); amfb.setRole(BeanDefinition.ROLE_INFRASTRUCTURE); @@ -363,3 +370,33 @@ class OrderDecorator implements Ordered { } } +/** + * Custom {@link MethodInvokingFactoryBean} that is specifically used for looking up the child {@link ProviderManager} + * value for {@link ProviderManager#setEraseCredentialsAfterAuthentication(boolean)} given the parent + * {@link AuthenticationManager}. This is necessary because the parent {@link AuthenticationManager} might not be a + * {@link ProviderManager}. + * + * @author Rob Winch + */ +final class ClearCredentialsMethodInvokingFactoryBean extends MethodInvokingFactoryBean { + public void afterPropertiesSet() throws Exception { + boolean isTargetProviderManager = getTargetObject() instanceof ProviderManager; + if(!isTargetProviderManager) { + setTargetObject(this); + } + super.afterPropertiesSet(); + } + + /** + * The default value if the target object is not a ProviderManager is false. We use false because this feature is + * associated with {@link ProviderManager} not {@link AuthenticationManager}. If the user wants to leverage + * {@link ProviderManager#setEraseCredentialsAfterAuthentication(boolean)} their original + * {@link AuthenticationManager} must be a {@link ProviderManager} (we should not magically add this functionality + * to their implementation since we cannot determine if it should be on or off). + * + * @return + */ + public boolean isEraseCredentialsAfterAuthentication() { + return false; + } +} \ No newline at end of file diff --git a/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy index b29e8ec6c4..848e76d842 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy @@ -624,6 +624,29 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { !getFilter(UsernamePasswordAuthenticationFilter).authenticationManager.eraseCredentialsAfterAuthentication } + def 'SEC-2020 authentication-manager@erase-credentials with http@authentication-manager-ref'() { + xml.http('authentication-manager-ref':'authMgr') { + 'form-login'() + } + createAppContext(""); + expect: + def authManager = getFilter(UsernamePasswordAuthenticationFilter).authenticationManager + !authManager.eraseCredentialsAfterAuthentication + !authManager.parent.eraseCredentialsAfterAuthentication + } + + def 'authentication-manager@erase-credentials with http@authentication-manager-ref not ProviderManager'() { + xml.http('authentication-manager-ref':'authMgr') { + 'form-login'() + } + xml.'b:bean'(id: 'authMgr', 'class': MockAuthenticationManager.class.name) + createAppContext() + expect: + def authManager = getFilter(UsernamePasswordAuthenticationFilter).authenticationManager + !authManager.eraseCredentialsAfterAuthentication + authManager.parent instanceof MockAuthenticationManager + } + def jeeFilterExtractsExpectedRoles() { xml.http() { jee('mappable-roles': 'admin,user,a,b,c')