From 978b7d4707cb52ca99103ae6c045fddcedd3c221 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Thu, 2 Dec 2010 18:19:27 +0000 Subject: [PATCH] SEC-1631: Reduced use of reflection in DefaultAuthenticationEventPublisher and added tests. --- .../AuthenticationServiceException.java | 9 +- .../DefaultAuthenticationEventPublisher.java | 78 ++++++----- .../ProviderNotFoundException.java | 11 -- ...aultAuthenticationEventPublisherTests.java | 123 ++++++++++++++++++ .../authentication/ProviderManagerTests.java | 10 +- 5 files changed, 173 insertions(+), 58 deletions(-) create mode 100644 core/src/test/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisherTests.java diff --git a/core/src/main/java/org/springframework/security/authentication/AuthenticationServiceException.java b/core/src/main/java/org/springframework/security/authentication/AuthenticationServiceException.java index b44f67a997..6a6de158ed 100644 --- a/core/src/main/java/org/springframework/security/authentication/AuthenticationServiceException.java +++ b/core/src/main/java/org/springframework/security/authentication/AuthenticationServiceException.java @@ -18,15 +18,16 @@ package org.springframework.security.authentication; import org.springframework.security.core.AuthenticationException; /** - * Thrown if an authentication request could not be processed due to a system problem.

This might be thrown if a - * backend authentication repository is unavailable.

+ * Thrown if an authentication request could not be processed due to a system problem. + *

+ * This might be thrown if a backend authentication repository is unavailable, for example. * * @author Ben Alex */ public class AuthenticationServiceException extends AuthenticationException { //~ Constructors =================================================================================================== -/** + /** * Constructs an AuthenticationServiceException with the * specified message. * @@ -36,7 +37,7 @@ public class AuthenticationServiceException extends AuthenticationException { super(msg); } -/** + /** * Constructs an AuthenticationServiceException with the * specified message and root cause. * 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 5a7a718640..c889978a4f 100644 --- a/core/src/main/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisher.java +++ b/core/src/main/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisher.java @@ -2,13 +2,14 @@ package org.springframework.security.authentication; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; -import java.util.Properties; +import java.util.*; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.security.authentication.event.AbstractAuthenticationEvent; +import org.springframework.security.authentication.event.AbstractAuthenticationFailureEvent; import org.springframework.security.authentication.event.AuthenticationFailureBadCredentialsEvent; import org.springframework.security.authentication.event.AuthenticationFailureCredentialsExpiredEvent; import org.springframework.security.authentication.event.AuthenticationFailureDisabledEvent; @@ -44,7 +45,8 @@ public class DefaultAuthenticationEventPublisher implements AuthenticationEventP private final Log logger = LogFactory.getLog(getClass()); private ApplicationEventPublisher applicationEventPublisher; - private final Properties exceptionMappings; + private final HashMap> exceptionMappings + = new HashMap>(); public DefaultAuthenticationEventPublisher() { this(null); @@ -52,25 +54,17 @@ public class DefaultAuthenticationEventPublisher implements AuthenticationEventP public DefaultAuthenticationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { this.applicationEventPublisher = applicationEventPublisher; - exceptionMappings = new Properties(); - exceptionMappings.put(AccountExpiredException.class.getName(), - AuthenticationFailureExpiredEvent.class.getName()); - exceptionMappings.put(AuthenticationServiceException.class.getName(), - AuthenticationFailureServiceExceptionEvent.class.getName()); - exceptionMappings.put(LockedException.class.getName(), - AuthenticationFailureLockedEvent.class.getName()); - exceptionMappings.put(CredentialsExpiredException.class.getName(), - AuthenticationFailureCredentialsExpiredEvent.class.getName()); - exceptionMappings.put(DisabledException.class.getName(), - AuthenticationFailureDisabledEvent.class.getName()); - exceptionMappings.put(BadCredentialsException.class.getName(), - AuthenticationFailureBadCredentialsEvent.class.getName()); - exceptionMappings.put(UsernameNotFoundException.class.getName(), - AuthenticationFailureBadCredentialsEvent.class.getName()); - exceptionMappings.put(ProviderNotFoundException.class.getName(), - AuthenticationFailureProviderNotFoundEvent.class.getName()); - exceptionMappings.put("org.springframework.security.authentication.cas.ProxyUntrustedException", - AuthenticationFailureProxyUntrustedEvent.class.getName()); + + addMapping(BadCredentialsException.class.getName(), AuthenticationFailureBadCredentialsEvent.class); + addMapping(UsernameNotFoundException.class.getName(), AuthenticationFailureBadCredentialsEvent.class); + addMapping(AccountExpiredException.class.getName(), AuthenticationFailureExpiredEvent.class); + addMapping(ProviderNotFoundException.class.getName(), AuthenticationFailureProviderNotFoundEvent.class); + addMapping(DisabledException.class.getName(), AuthenticationFailureDisabledEvent.class); + addMapping(LockedException.class.getName(), AuthenticationFailureLockedEvent.class); + addMapping(AuthenticationServiceException.class.getName(), AuthenticationFailureServiceExceptionEvent.class); + addMapping(CredentialsExpiredException.class.getName(), AuthenticationFailureCredentialsExpiredEvent.class); + addMapping("org.springframework.security.authentication.cas.ProxyUntrustedException", + AuthenticationFailureProxyUntrustedEvent.class); } public void publishAuthenticationSuccess(Authentication authentication) { @@ -79,23 +73,14 @@ public class DefaultAuthenticationEventPublisher implements AuthenticationEventP } } - public void publishAuthenticationFailure(AuthenticationException exception, - Authentication authentication) { - String className = exceptionMappings.getProperty(exception.getClass().getName()); + public void publishAuthenticationFailure(AuthenticationException exception, Authentication authentication) { + Constructor constructor = exceptionMappings.get(exception.getClass().getName()); AbstractAuthenticationEvent event = null; - if (className != null) { + if (constructor != null) { try { - Class clazz = getClass().getClassLoader().loadClass(className); - Constructor constructor = clazz.getConstructor(new Class[] { - Authentication.class, AuthenticationException.class - }); - Object obj = constructor.newInstance(authentication, exception); - Assert.isInstanceOf(AbstractAuthenticationEvent.class, obj, "Must be an AbstractAuthenticationEvent"); - event = (AbstractAuthenticationEvent) obj; - } catch (ClassNotFoundException ignored) {} - catch (NoSuchMethodException ignored) {} - catch (IllegalAccessException ignored) {} + event = constructor.newInstance(authentication, exception); + } catch (IllegalAccessException ignored) {} catch (InstantiationException ignored) {} catch (InvocationTargetException ignored) {} } @@ -122,8 +107,29 @@ public class DefaultAuthenticationEventPublisher implements AuthenticationEventP * @param additionalExceptionMappings where keys are the fully-qualified string name of the exception class and the * values are the fully-qualified string name of the event class to fire. */ + @SuppressWarnings({"unchecked"}) public void setAdditionalExceptionMappings(Properties additionalExceptionMappings) { Assert.notNull(additionalExceptionMappings, "The exceptionMappings object must not be null"); - exceptionMappings.putAll(additionalExceptionMappings); + for(Object exceptionClass : additionalExceptionMappings.keySet()) { + String eventClass = (String) additionalExceptionMappings.get(exceptionClass); + try { + Class clazz = getClass().getClassLoader().loadClass(eventClass); + Assert.isAssignable(AbstractAuthenticationFailureEvent.class, clazz); + addMapping((String) exceptionClass, (Class) clazz); + } catch (ClassNotFoundException e) { + throw new RuntimeException("Failed to load authentication event class " + eventClass); + } + } + } + + private void addMapping(String exceptionClass, + Class eventClass) { + try { + Constructor constructor = + eventClass.getConstructor(Authentication.class, AuthenticationException.class); + exceptionMappings.put(exceptionClass, constructor); + } catch (NoSuchMethodException e) { + throw new RuntimeException("Authentication event class " + eventClass.getName() + " has no suitable constructor"); + } } } diff --git a/core/src/main/java/org/springframework/security/authentication/ProviderNotFoundException.java b/core/src/main/java/org/springframework/security/authentication/ProviderNotFoundException.java index 305fffaef6..a1f791685a 100644 --- a/core/src/main/java/org/springframework/security/authentication/ProviderNotFoundException.java +++ b/core/src/main/java/org/springframework/security/authentication/ProviderNotFoundException.java @@ -37,15 +37,4 @@ public class ProviderNotFoundException extends AuthenticationException { public ProviderNotFoundException(String msg) { super(msg); } - - /** - * Constructs a ProviderNotFoundException with the specified - * message and root cause. - * - * @param msg the detail message - * @param t root cause - */ - public ProviderNotFoundException(String msg, Throwable t) { - super(msg, t); - } } diff --git a/core/src/test/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisherTests.java b/core/src/test/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisherTests.java new file mode 100644 index 0000000000..1a3db9833b --- /dev/null +++ b/core/src/test/java/org/springframework/security/authentication/DefaultAuthenticationEventPublisherTests.java @@ -0,0 +1,123 @@ +package org.springframework.security.authentication; + +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.*; + +import org.junit.*; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.security.authentication.event.AuthenticationFailureBadCredentialsEvent; +import org.springframework.security.authentication.event.AuthenticationFailureCredentialsExpiredEvent; +import org.springframework.security.authentication.event.AuthenticationFailureDisabledEvent; +import org.springframework.security.authentication.event.AuthenticationFailureExpiredEvent; +import org.springframework.security.authentication.event.AuthenticationFailureLockedEvent; +import org.springframework.security.authentication.event.AuthenticationFailureProviderNotFoundEvent; +import org.springframework.security.authentication.event.AuthenticationFailureServiceExceptionEvent; +import org.springframework.security.authentication.event.AuthenticationSuccessEvent; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.userdetails.UsernameNotFoundException; + +import java.util.*; + +/** + * @author Luke Taylor + */ +public class DefaultAuthenticationEventPublisherTests { + DefaultAuthenticationEventPublisher publisher; + + @Test + public void expectedDefaultMappingsAreSatisfied() throws Exception { + publisher = new DefaultAuthenticationEventPublisher(); + ApplicationEventPublisher appPublisher = mock(ApplicationEventPublisher.class); + publisher.setApplicationEventPublisher(appPublisher); + Authentication a = mock(Authentication.class); + + Exception cause = new Exception(); + Object extraInfo = new Object(); + publisher.publishAuthenticationFailure(new BadCredentialsException(""), a); + publisher.publishAuthenticationFailure(new BadCredentialsException("", extraInfo), a); + publisher.publishAuthenticationFailure(new BadCredentialsException("", cause), a); + verify(appPublisher, times(3)).publishEvent(isA(AuthenticationFailureBadCredentialsEvent.class)); + reset(appPublisher); + publisher.publishAuthenticationFailure(new UsernameNotFoundException(""), a); + publisher.publishAuthenticationFailure(new UsernameNotFoundException("", extraInfo), a); + publisher.publishAuthenticationFailure(new UsernameNotFoundException("", cause), a); + publisher.publishAuthenticationFailure(new AccountExpiredException(""), a); + publisher.publishAuthenticationFailure(new AccountExpiredException("", extraInfo), a); + publisher.publishAuthenticationFailure(new AccountExpiredException("", cause), a); + publisher.publishAuthenticationFailure(new ProviderNotFoundException(""), a); + publisher.publishAuthenticationFailure(new DisabledException(""), a); + publisher.publishAuthenticationFailure(new DisabledException("", extraInfo), a); + publisher.publishAuthenticationFailure(new DisabledException("", cause), a); + publisher.publishAuthenticationFailure(new LockedException(""), a); + publisher.publishAuthenticationFailure(new LockedException("", extraInfo), a); + publisher.publishAuthenticationFailure(new LockedException("", cause), a); + publisher.publishAuthenticationFailure(new AuthenticationServiceException(""), a); + publisher.publishAuthenticationFailure(new AuthenticationServiceException("",cause), a); + publisher.publishAuthenticationFailure(new CredentialsExpiredException(""), a); + publisher.publishAuthenticationFailure(new CredentialsExpiredException("", extraInfo), a); + publisher.publishAuthenticationFailure(new CredentialsExpiredException("", cause), a); + verify(appPublisher, times(3)).publishEvent(isA(AuthenticationFailureBadCredentialsEvent.class)); + verify(appPublisher, times(3)).publishEvent(isA(AuthenticationFailureExpiredEvent.class)); + verify(appPublisher).publishEvent(isA(AuthenticationFailureProviderNotFoundEvent.class)); + verify(appPublisher, times(3)).publishEvent(isA(AuthenticationFailureDisabledEvent.class)); + verify(appPublisher, times(3)).publishEvent(isA(AuthenticationFailureLockedEvent.class)); + verify(appPublisher, times(2)).publishEvent(isA(AuthenticationFailureServiceExceptionEvent.class)); + verify(appPublisher, times(3)).publishEvent(isA(AuthenticationFailureCredentialsExpiredEvent.class)); + verifyNoMoreInteractions(appPublisher); + } + + @Test + public void authenticationSuccessIsPublished() { + publisher = new DefaultAuthenticationEventPublisher(); + ApplicationEventPublisher appPublisher = mock(ApplicationEventPublisher.class); + publisher.setApplicationEventPublisher(appPublisher); + publisher.publishAuthenticationSuccess(mock(Authentication.class)); + verify(appPublisher).publishEvent(isA(AuthenticationSuccessEvent.class)); + + publisher.setApplicationEventPublisher(null); + // Should be ignored with null app publisher + publisher.publishAuthenticationSuccess(mock(Authentication.class)); + } + + @Test + public void additionalExceptionMappingsAreSupported() { + publisher = new DefaultAuthenticationEventPublisher(); + Properties p = new Properties(); + p.put(MockAuthenticationException.class.getName(), AuthenticationFailureDisabledEvent.class.getName()); + publisher.setAdditionalExceptionMappings(p); + ApplicationEventPublisher appPublisher = mock(ApplicationEventPublisher.class); + + publisher.setApplicationEventPublisher(appPublisher); + publisher.publishAuthenticationFailure(new MockAuthenticationException("test"), mock(Authentication.class)); + verify(appPublisher).publishEvent(isA(AuthenticationFailureDisabledEvent.class)); + } + + @Test(expected=RuntimeException.class) + public void missingEventClassExceptionCausesException() { + publisher = new DefaultAuthenticationEventPublisher(); + Properties p = new Properties(); + p.put(MockAuthenticationException.class.getName(), "NoSuchClass"); + publisher.setAdditionalExceptionMappings(p); + } + + @Test + public void unknownFailureExceptionIsIgnored() throws Exception { + publisher = new DefaultAuthenticationEventPublisher(); + Properties p = new Properties(); + p.put(MockAuthenticationException.class.getName(), AuthenticationFailureDisabledEvent.class.getName()); + publisher.setAdditionalExceptionMappings(p); + ApplicationEventPublisher appPublisher = mock(ApplicationEventPublisher.class); + + publisher.setApplicationEventPublisher(appPublisher); + publisher.publishAuthenticationFailure(new AuthenticationException("") {}, mock(Authentication.class)); + verifyZeroInteractions(appPublisher); + } + + private static final class MockAuthenticationException extends AuthenticationException { + public MockAuthenticationException(String msg) { + super(msg); + } + } + +} diff --git a/core/src/test/java/org/springframework/security/authentication/ProviderManagerTests.java b/core/src/test/java/org/springframework/security/authentication/ProviderManagerTests.java index 0113f60aa1..87c912c0ef 100644 --- a/core/src/test/java/org/springframework/security/authentication/ProviderManagerTests.java +++ b/core/src/test/java/org/springframework/security/authentication/ProviderManagerTests.java @@ -296,13 +296,9 @@ public class ProviderManagerTests { } } - public boolean supports(Class authentication) { - if (TestingAuthenticationToken.class.isAssignableFrom(authentication) || - UsernamePasswordAuthenticationToken.class.isAssignableFrom(authentication)) { - return true; - } else { - return false; - } + public boolean supports(Class authentication) { + return TestingAuthenticationToken.class.isAssignableFrom(authentication) || + UsernamePasswordAuthenticationToken.class.isAssignableFrom(authentication); } } }