From a5ec116e80963cd215a2e0b3a16c64288e96dfcf Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Tue, 31 Jul 2012 16:03:48 -0500 Subject: [PATCH] SEC-1919: Log error when fail to communicate with LDAP Previously communication errors with LDAP were only logged at debug level. Communication errors (along with other non-authenticated related NamingExceptions) are now logged as error messages. We created an InternalAuthetnicationServiceException to represent errors that should be logged as errors to distinguish between internal and external authentication failures. For example, we do not want an OpenID Provider being able to report errors that cause our logs to fill up. However, an LDAP system is internal and should be trusted so logging at an error level makes sense. --- .../AuthenticationServiceException.java | 1 + ...nternalAuthenticationServiceException.java | 40 +++++++++++++++++++ .../LdapAuthenticationProvider.java | 4 +- .../LdapAuthenticationProviderTests.java | 19 +++++++++ ...bstractAuthenticationProcessingFilter.java | 6 +++ ...ctAuthenticationProcessingFilterTests.java | 26 ++++++++++++ 6 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/org/springframework/security/authentication/InternalAuthenticationServiceException.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 6a6de158ed..950fd4586f 100644 --- a/core/src/main/java/org/springframework/security/authentication/AuthenticationServiceException.java +++ b/core/src/main/java/org/springframework/security/authentication/AuthenticationServiceException.java @@ -23,6 +23,7 @@ import org.springframework.security.core.AuthenticationException; * This might be thrown if a backend authentication repository is unavailable, for example. * * @author Ben Alex + * @see InternalAuthenticationServiceException */ public class AuthenticationServiceException extends AuthenticationException { //~ Constructors =================================================================================================== diff --git a/core/src/main/java/org/springframework/security/authentication/InternalAuthenticationServiceException.java b/core/src/main/java/org/springframework/security/authentication/InternalAuthenticationServiceException.java new file mode 100644 index 0000000000..648f3b5291 --- /dev/null +++ b/core/src/main/java/org/springframework/security/authentication/InternalAuthenticationServiceException.java @@ -0,0 +1,40 @@ +/* + * Copyright 2002-2012 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package org.springframework.security.authentication; + +/** + *

+ * Thrown if an authentication request could not be processed due to a system problem that occurred internally. It + * differs from {@link AuthenticationServiceException} in that it would not be thrown if an external system has an + * internal error or failure. This ensures that we can handle errors that are within our control distinctly from errors + * of other systems. The advantage to this distinction is that the unrusted external system should not be able to fill + * up logs and cause excessive IO. However, an internal system should report errors. + *

+ *

+ * This might be thrown if a backend authentication repository is unavailable, for example. However, it would not be + * thrown in the event that an error occurred when validating an OpenID response with an OpenID Provider. + *

+ * + * @author Rob Winch + * + */ +public class InternalAuthenticationServiceException extends AuthenticationServiceException { + + public InternalAuthenticationServiceException(String message, Throwable cause) { + super(message, cause); + } + + public InternalAuthenticationServiceException(String message) { + super(message); + } +} \ No newline at end of file diff --git a/ldap/src/main/java/org/springframework/security/ldap/authentication/LdapAuthenticationProvider.java b/ldap/src/main/java/org/springframework/security/ldap/authentication/LdapAuthenticationProvider.java index e205c85964..10f5976690 100644 --- a/ldap/src/main/java/org/springframework/security/ldap/authentication/LdapAuthenticationProvider.java +++ b/ldap/src/main/java/org/springframework/security/ldap/authentication/LdapAuthenticationProvider.java @@ -17,8 +17,8 @@ package org.springframework.security.ldap.authentication; import org.springframework.ldap.NamingException; import org.springframework.ldap.core.DirContextOperations; -import org.springframework.security.authentication.AuthenticationServiceException; import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.InternalAuthenticationServiceException; import org.springframework.security.authentication.LockedException; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.GrantedAuthority; @@ -188,7 +188,7 @@ public class LdapAuthenticationProvider extends AbstractLdapAuthenticationProvid throw notFound; } } catch (NamingException ldapAccessFailure) { - throw new AuthenticationServiceException(ldapAccessFailure.getMessage(), ldapAccessFailure); + throw new InternalAuthenticationServiceException(ldapAccessFailure.getMessage(), ldapAccessFailure); } } diff --git a/ldap/src/test/java/org/springframework/security/ldap/authentication/LdapAuthenticationProviderTests.java b/ldap/src/test/java/org/springframework/security/ldap/authentication/LdapAuthenticationProviderTests.java index 63aa441a23..8ec29e2940 100644 --- a/ldap/src/test/java/org/springframework/security/ldap/authentication/LdapAuthenticationProviderTests.java +++ b/ldap/src/test/java/org/springframework/security/ldap/authentication/LdapAuthenticationProviderTests.java @@ -21,10 +21,12 @@ import static org.mockito.Mockito.*; import java.util.*; import org.junit.Test; +import org.springframework.ldap.CommunicationException; import org.springframework.ldap.core.DirContextAdapter; import org.springframework.ldap.core.DirContextOperations; import org.springframework.ldap.core.DistinguishedName; import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.InternalAuthenticationServiceException; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; @@ -39,6 +41,7 @@ import org.springframework.security.ldap.userdetails.LdapUserDetailsMapper; * Tests {@link LdapAuthenticationProvider}. * * @author Luke Taylor + * @author Rob Winch */ public class LdapAuthenticationProviderTests { @@ -147,6 +150,22 @@ public class LdapAuthenticationProviderTests { assertTrue(AuthorityUtils.authorityListToSet(user.getAuthorities()).contains("ROLE_FROM_ENTRY")); } + @Test + public void authenticateWithNamingException() { + UsernamePasswordAuthenticationToken authRequest = new UsernamePasswordAuthenticationToken("ben", "benspassword"); + LdapAuthenticator mockAuthenticator = mock(LdapAuthenticator.class); + CommunicationException expectedCause = new CommunicationException(new javax.naming.CommunicationException()); + when(mockAuthenticator.authenticate(authRequest)).thenThrow(expectedCause); + + LdapAuthenticationProvider ldapProvider = new LdapAuthenticationProvider(mockAuthenticator); + try { + ldapProvider.authenticate(authRequest); + fail("Expected Exception"); + } catch(InternalAuthenticationServiceException success) { + assertSame(expectedCause, success.getCause()); + } + } + //~ Inner Classes ================================================================================================== class MockAuthenticator implements LdapAuthenticator { diff --git a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java index 1f94305519..a2db490781 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java @@ -31,6 +31,7 @@ import org.springframework.context.MessageSourceAware; import org.springframework.context.support.MessageSourceAccessor; import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationManager; +import org.springframework.security.authentication.InternalAuthenticationServiceException; import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; @@ -197,6 +198,11 @@ public abstract class AbstractAuthenticationProcessingFilter extends GenericFilt return; } sessionStrategy.onAuthentication(authResult, request, response); + } catch(InternalAuthenticationServiceException failed) { + logger.error("An internal error occurred while trying to authenticate the user.", failed); + unsuccessfulAuthentication(request, response, failed); + + return; } catch (AuthenticationException failed) { // Authentication failed diff --git a/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java index 693a24d1cb..21be453b4e 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java @@ -18,6 +18,7 @@ package org.springframework.security.web.authentication; import static org.junit.Assert.*; import static org.mockito.Mockito.*; +import org.apache.commons.logging.Log; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -26,6 +27,7 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.InternalAuthenticationServiceException; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; @@ -33,6 +35,7 @@ import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.authentication.rememberme.TokenBasedRememberMeServices; import org.springframework.security.web.authentication.session.SessionAuthenticationStrategy; +import org.springframework.test.util.ReflectionTestUtils; import javax.servlet.FilterChain; import javax.servlet.ServletException; @@ -49,6 +52,7 @@ import java.io.IOException; * * @author Ben Alex * @author Luke Taylor + * @author Rob Winch */ @SuppressWarnings("deprecation") public class AbstractAuthenticationProcessingFilterTests { @@ -352,6 +356,28 @@ public class AbstractAuthenticationProcessingFilterTests { assertEquals(HttpServletResponse.SC_UNAUTHORIZED, response.getStatus()); } + /** + * SEC-1919 + */ + @Test + public void loginErrorWithInternAuthenticationServiceExceptionLogsError() throws Exception { + MockHttpServletRequest request = createMockAuthenticationRequest(); + + MockFilterChain chain = new MockFilterChain(true); + MockHttpServletResponse response = new MockHttpServletResponse(); + + Log logger = mock(Log.class); + MockAuthenticationFilter filter = new MockAuthenticationFilter(false); + ReflectionTestUtils.setField(filter, "logger", logger); + filter.exceptionToThrow = new InternalAuthenticationServiceException("Mock requested to do so"); + successHandler.setDefaultTargetUrl("http://monkeymachine.co.uk/"); + filter.setAuthenticationSuccessHandler(successHandler); + + filter.doFilter(request, response, chain); + + verify(logger).error(anyString(), eq(filter.exceptionToThrow)); + assertEquals(HttpServletResponse.SC_UNAUTHORIZED, response.getStatus()); + } //~ Inner Classes ==================================================================================================