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.

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 ===================================================================================================

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;
+
+/**
+ * <p>
+ * 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.
+ * </p>
+ * <p>
+ * 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.
+ * </p>
+ *
+ * @author Rob Winch
+ *
+ */
+public class InternalAuthenticationServiceException extends AuthenticationServiceException {
+
+    public InternalAuthenticationServiceException(String message, Throwable cause) {
+        super(message, cause);
+    }
+
+    public InternalAuthenticationServiceException(String message) {
+        super(message);
+    }
+}

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);
         }
     }
 

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 {

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

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 ==================================================================================================