From 43be9ea2a467feecaed9027c8399553adf3499d3 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Fri, 26 Nov 2010 13:58:49 +0000 Subject: [PATCH] SEC-1430: Removed caching of username in session upon failed authentication. Improved Javadoc. --- .../openid/OpenIDAuthenticationFilter.java | 14 -------------- sandbox/heavyduty/src/main/webapp/login.jsp | 4 ++-- .../security/web/WebAttributes.java | 11 +++++++++++ .../SimpleUrlAuthenticationSuccessHandler.java | 1 - .../UsernamePasswordAuthenticationFilter.java | 11 ++++------- .../session/SessionFixationProtectionStrategy.java | 5 +++-- .../ui/DefaultLoginPageGeneratingFilter.java | 7 +------ .../UsernamePasswordAuthenticationFilterTests.java | 6 ------ 8 files changed, 21 insertions(+), 38 deletions(-) diff --git a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java index 4522981ec7..af347eb367 100644 --- a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java +++ b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java @@ -123,8 +123,6 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing if (!StringUtils.hasText(identity)) { String claimedIdentity = obtainUsername(request); - // Make the username available to the view - setLastUsername(claimedIdentity, request); try { String returnToUrl = buildReturnToUrl(request); @@ -159,21 +157,9 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing // delegate to the authentication provider Authentication authentication = this.getAuthenticationManager().authenticate(token); - if (authentication.isAuthenticated()) { - setLastUsername(token.getIdentityUrl(), request); - } - return authentication; } - private void setLastUsername(String username, HttpServletRequest request) { - HttpSession session = request.getSession(false); - - if (session != null || getAllowSessionCreation()) { - request.getSession().setAttribute(UsernamePasswordAuthenticationFilter.SPRING_SECURITY_LAST_USERNAME_KEY, username); - } - } - protected String lookupRealm(String returnToUrl) { String mapping = realmMapping.get(returnToUrl); diff --git a/sandbox/heavyduty/src/main/webapp/login.jsp b/sandbox/heavyduty/src/main/webapp/login.jsp index 8a7abb57e1..fbe58d7204 100755 --- a/sandbox/heavyduty/src/main/webapp/login.jsp +++ b/sandbox/heavyduty/src/main/webapp/login.jsp @@ -23,8 +23,8 @@
- - + + diff --git a/web/src/main/java/org/springframework/security/web/WebAttributes.java b/web/src/main/java/org/springframework/security/web/WebAttributes.java index aff2008f5e..5343657b9b 100644 --- a/web/src/main/java/org/springframework/security/web/WebAttributes.java +++ b/web/src/main/java/org/springframework/security/web/WebAttributes.java @@ -7,7 +7,18 @@ package org.springframework.security.web; * @since 3.0.3 */ public final class WebAttributes { + /** + * Used to cache an {@code AccessDeniedException} in the request for rendering. + * + * @see org.springframework.security.web.access.AccessDeniedHandlerImpl + */ public static final String ACCESS_DENIED_403 = "SPRING_SECURITY_403_EXCEPTION"; + + /** + * Used to cache an authentication-failure exception in the session. + * + * @see org.springframework.security.web.authentication.SimpleUrlAuthenticationFailureHandler + */ public static final String AUTHENTICATION_EXCEPTION = "SPRING_SECURITY_LAST_EXCEPTION"; public static final String LAST_USERNAME = "SPRING_SECURITY_LAST_USERNAME"; } diff --git a/web/src/main/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandler.java b/web/src/main/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandler.java index 7770b8d34d..7de2430d23 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandler.java +++ b/web/src/main/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandler.java @@ -55,6 +55,5 @@ public class SimpleUrlAuthenticationSuccessHandler extends AbstractAuthenticatio } session.removeAttribute(WebAttributes.AUTHENTICATION_EXCEPTION); - session.removeAttribute(WebAttributes.LAST_USERNAME); } } diff --git a/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java b/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java index 6a78758267..84542ecfec 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java @@ -50,6 +50,10 @@ public class UsernamePasswordAuthenticationFilter extends AbstractAuthentication public static final String SPRING_SECURITY_FORM_USERNAME_KEY = "j_username"; public static final String SPRING_SECURITY_FORM_PASSWORD_KEY = "j_password"; + /** + * @deprecated If you want to retain the username, cache it in a customized {@code AuthenticationFailureHandler} + */ + @Deprecated public static final String SPRING_SECURITY_LAST_USERNAME_KEY = "SPRING_SECURITY_LAST_USERNAME"; private String usernameParameter = SPRING_SECURITY_FORM_USERNAME_KEY; @@ -84,13 +88,6 @@ public class UsernamePasswordAuthenticationFilter extends AbstractAuthentication UsernamePasswordAuthenticationToken authRequest = new UsernamePasswordAuthenticationToken(username, password); - // Place the last username attempted into HttpSession for views - HttpSession session = request.getSession(false); - - if (session != null || getAllowSessionCreation()) { - request.getSession().setAttribute(SPRING_SECURITY_LAST_USERNAME_KEY, TextEscapeUtils.escapeEntities(username)); - } - // Allow subclasses to set the "details" property setDetails(request, authRequest); diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java index cc24d2b7ef..ffea75861d 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java @@ -14,8 +14,9 @@ import java.util.*; * The default implementation of {@link SessionAuthenticationStrategy}. *

* Creates a new session for the newly authenticated user if they already have a session (as a defence against - * session-fixation protection attacks), and copies their session attributes across to the new session - * (can be disabled by setting {@code migrateSessionAttributes} to {@code false}). + * session-fixation protection attacks), and copies their session attributes across to the new session. + * The copying of the attributes can be disabled by setting {@code migrateSessionAttributes} to {@code false} + * (note that even in this case, internal Spring Security attributes will still be migrated to the new session). *

* This approach will only be effective if your servlet container always assigns a new session Id when a session is * invalidated and a new session created by calling {@link HttpServletRequest#getSession()}. diff --git a/web/src/main/java/org/springframework/security/web/authentication/ui/DefaultLoginPageGeneratingFilter.java b/web/src/main/java/org/springframework/security/web/authentication/ui/DefaultLoginPageGeneratingFilter.java index 6c9691545a..60aee87232 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/ui/DefaultLoginPageGeneratingFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/ui/DefaultLoginPageGeneratingFilter.java @@ -94,18 +94,13 @@ public class DefaultLoginPageGeneratingFilter extends GenericFilterBean { private String generateLoginPageHtml(HttpServletRequest request) { boolean loginError = request.getParameter(ERROR_PARAMETER_NAME) != null; String errorMsg = "none"; - String lastUser = ""; if (loginError) { HttpSession session = request.getSession(false); if(session != null) { - lastUser = (String) session.getAttribute(UsernamePasswordAuthenticationFilter.SPRING_SECURITY_LAST_USERNAME_KEY); AuthenticationException ex = (AuthenticationException) session.getAttribute(WebAttributes.AUTHENTICATION_EXCEPTION); errorMsg = ex != null ? ex.getMessage() : "none"; - if (lastUser == null) { - lastUser = ""; - } } } @@ -128,7 +123,7 @@ public class DefaultLoginPageGeneratingFilter extends GenericFilterBean { sb.append("\n"); sb.append("

User:value='<%= session.getAttribute(UsernamePasswordAuthenticationFilter.SPRING_SECURITY_LAST_USERNAME_KEY) %>'<% } %>>
Password:
User:
Password:
Don't ask for my password for two weeks
\n"); sb.append(" \n"); + sb.append(usernameParameter).append("' value='").append("'>\n"); sb.append(" \n"); if (rememberMeParameter != null) { diff --git a/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java index 207ca01223..f7e0ebd544 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java @@ -55,8 +55,6 @@ public class UsernamePasswordAuthenticationFilterTests extends TestCase { Authentication result = filter.attemptAuthentication(request, new MockHttpServletResponse()); assertTrue(result != null); - assertEquals("rod", request.getSession().getAttribute( - UsernamePasswordAuthenticationFilter.SPRING_SECURITY_LAST_USERNAME_KEY)); assertEquals("127.0.0.1", ((WebAuthenticationDetails) result.getDetails()).getRemoteAddress()); } @@ -123,10 +121,6 @@ public class UsernamePasswordAuthenticationFilterTests extends TestCase { fail("Expected AuthenticationException"); } catch (AuthenticationException e) { } - - // Check username has still been set - assertEquals("rod", request.getSession().getAttribute( - UsernamePasswordAuthenticationFilter.SPRING_SECURITY_LAST_USERNAME_KEY)); } /**
User:
Password: