From 8b51c2c97d010af2c43f019891ba81827333eb4e Mon Sep 17 00:00:00 2001
From: Luke Taylor <luke.taylor@springsource.com>
Date: Tue, 9 Nov 2010 13:55:45 +0000
Subject: [PATCH] SEC-1587: Add explicit call to removeAttribute() to remove
 the context from the session if the current context is empty or anonymous.

Allows for the situation where a user is logged out without invalidating the session.
---
 .../HttpSessionSecurityContextRepository.java | 18 ++++++++----
 ...SessionSecurityContextRepositoryTests.java | 29 +++++++++++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/web/src/main/java/org/springframework/security/web/context/HttpSessionSecurityContextRepository.java b/web/src/main/java/org/springframework/security/web/context/HttpSessionSecurityContextRepository.java
index 6a82a6a3cb..79b28c5606 100644
--- a/web/src/main/java/org/springframework/security/web/context/HttpSessionSecurityContextRepository.java
+++ b/web/src/main/java/org/springframework/security/web/context/HttpSessionSecurityContextRepository.java
@@ -262,22 +262,28 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo
          */
         @Override
         protected void saveContext(SecurityContext context) {
+            final Authentication authentication = context.getAuthentication();
+            HttpSession httpSession = request.getSession(false);
+
             // See SEC-776
-            if (authenticationTrustResolver.isAnonymous(context.getAuthentication())) {
+            if (authentication == null || authenticationTrustResolver.isAnonymous(authentication)) {
                 if (logger.isDebugEnabled()) {
-                    logger.debug("SecurityContext contents are anonymous - context will not be stored in HttpSession.");
+                    logger.debug("SecurityContext is empty or contents are anonymous - context will not be stored in HttpSession.");
+                }
+
+                if (httpSession != null) {
+                    // SEC-1587 A non-anonymous context may still be in the session
+                    httpSession.removeAttribute(SPRING_SECURITY_CONTEXT_KEY);
                 }
                 return;
             }
 
-            HttpSession httpSession = request.getSession(false);
-
             if (httpSession == null) {
                 httpSession = createNewSessionIfAllowed(context);
             }
 
-            // If HttpSession exists, store current SecurityContextHolder contents but only if
-            // the SecurityContext has actually changed in this thread (see SEC-37, SEC-1307, SEC-1528)
+            // If HttpSession exists, store current SecurityContext but only if it has
+            // actually changed in this thread (see SEC-37, SEC-1307, SEC-1528)
             if (httpSession != null) {
                 // We may have a new session, so check also whether the context attribute is set SEC-1561
                 if (contextChanged(context) || httpSession.getAttribute(SPRING_SECURITY_CONTEXT_KEY) == null) {
diff --git a/web/src/test/java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java b/web/src/test/java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java
index 419d29790a..70dfb92787 100644
--- a/web/src/test/java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java
+++ b/web/src/test/java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java
@@ -170,6 +170,35 @@ public class HttpSessionSecurityContextRepositoryTests {
         assertNull(request.getSession(false));
     }
 
+    // SEC-1587
+    @Test
+    public void contextIsRemovedFromSessionIfCurrentContextIsAnonymous() throws Exception {
+        HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository();
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        SecurityContext ctxInSession = SecurityContextHolder.createEmptyContext();
+        ctxInSession.setAuthentication(testToken);
+        request.getSession().setAttribute(SPRING_SECURITY_CONTEXT_KEY, ctxInSession);
+        HttpRequestResponseHolder holder = new HttpRequestResponseHolder(request, new MockHttpServletResponse());
+        repo.loadContext(holder);
+        SecurityContextHolder.getContext().setAuthentication(new AnonymousAuthenticationToken("x","x", testToken.getAuthorities()));
+        repo.saveContext(SecurityContextHolder.getContext(), holder.getRequest(), holder.getResponse());
+        assertNull(request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY));
+    }
+
+    @Test
+    public void contextIsRemovedFromSessionIfCurrentContextIsEmpty() throws Exception {
+        HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository();
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        SecurityContext ctxInSession = SecurityContextHolder.createEmptyContext();
+        ctxInSession.setAuthentication(testToken);
+        request.getSession().setAttribute(SPRING_SECURITY_CONTEXT_KEY, ctxInSession);
+        HttpRequestResponseHolder holder = new HttpRequestResponseHolder(request, new MockHttpServletResponse());
+        repo.loadContext(holder);
+        // Save an empty context
+        repo.saveContext(SecurityContextHolder.getContext(), holder.getRequest(), holder.getResponse());
+        assertNull(request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY));
+    }
+
     @Test
     @SuppressWarnings("deprecation")
     public void sessionDisableUrlRewritingPreventsSessionIdBeingWrittenToUrl() throws Exception {