Release 5.4.4

Issue gh-9387

gradle.properties

@@ -2,7 +2,7 @@ aspectjVersion=1.9.6
 gaeVersion=1.9.86
 springJavaformatVersion=0.0.25
 springBootVersion=2.4.2
-version=5.4.3
+version=5.4.4
 kotlinVersion=1.4.30
 org.gradle.jvmargs=-Xmx3g -XX:MaxPermSize=2048m -XX:+HeapDumpOnOutOfMemoryError
 org.gradle.parallel=true

web/src/main/java/org/springframework/security/web/context/HttpSessionSecurityContextRepository.java

@@ -136,12 +136,7 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo
 				SaveContextOnUpdateOrErrorResponseWrapper.class);
 		Assert.state(responseWrapper != null, () -> "Cannot invoke saveContext on response " + response
 				+ ". You must use the HttpRequestResponseHolder.response after invoking loadContext");
-		// saveContext() might already be called by the response wrapper if something in
-		// the chain called sendError() or sendRedirect(). This ensures we only call it
-		// once per request.
-		if (!responseWrapper.isContextSaved()) {
-			responseWrapper.saveContext(context);
-		}
+		responseWrapper.saveContext(context);
 	}
 
 	@Override
@@ -296,6 +291,8 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo
 
 		private final Authentication authBeforeExecution;
 
+		private boolean isSaveContextInvoked;
+
 		/**
 		 * Takes the parameters required to call <code>saveContext()</code> successfully
 		 * in addition to the request and the response object we are wrapping.
@@ -339,6 +336,7 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo
 					// SEC-1587 A non-anonymous context may still be in the session
 					// SEC-1735 remove if the contextBeforeExecution was not anonymous
 					httpSession.removeAttribute(springSecurityContextKey);
+					this.isSaveContextInvoked = true;
 				}
 				if (this.logger.isDebugEnabled()) {
 					if (authentication == null) {
@@ -358,6 +356,7 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo
 				// is set SEC-1561
 				if (contextChanged(context) || httpSession.getAttribute(springSecurityContextKey) == null) {
 					httpSession.setAttribute(springSecurityContextKey, context);
+					this.isSaveContextInvoked = true;
 					if (this.logger.isDebugEnabled()) {
 						this.logger.debug(LogMessage.format("Stored %s to HttpSession [%s]", context, httpSession));
 					}
@@ -366,7 +365,8 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo
 		}
 
 		private boolean contextChanged(SecurityContext context) {
-			return context != this.contextBeforeExecution || context.getAuthentication() != this.authBeforeExecution;
+			return this.isSaveContextInvoked || context != this.contextBeforeExecution
+					|| context.getAuthentication() != this.authBeforeExecution;
 		}
 
 		private HttpSession createNewSessionIfAllowed(SecurityContext context) {

web/src/test/java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java

@@ -16,12 +16,16 @@
 
 package org.springframework.security.web.context;
 
+import java.io.IOException;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
+import javax.servlet.Filter;
+import javax.servlet.ServletException;
 import javax.servlet.ServletOutputStream;
+import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
@@ -31,6 +35,7 @@ import javax.servlet.http.HttpSession;
 import org.junit.After;
 import org.junit.Test;
 
+import org.springframework.mock.web.MockFilterChain;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.mock.web.MockHttpSession;
@@ -38,10 +43,14 @@ import org.springframework.security.authentication.AbstractAuthenticationToken;
 import org.springframework.security.authentication.AnonymousAuthenticationToken;
 import org.springframework.security.authentication.AuthenticationTrustResolver;
 import org.springframework.security.authentication.TestingAuthenticationToken;
+import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
 import org.springframework.security.core.Transient;
 import org.springframework.security.core.authority.AuthorityUtils;
 import org.springframework.security.core.context.SecurityContext;
 import org.springframework.security.core.context.SecurityContextHolder;
+import org.springframework.security.core.context.SecurityContextImpl;
+import org.springframework.security.core.userdetails.User;
+import org.springframework.security.core.userdetails.UserDetails;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
@@ -162,6 +171,48 @@ public class HttpSessionSecurityContextRepositoryTests {
 		verify(session).setAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY, ctx);
 	}
 
+	@Test
+	public void saveContextWhenSaveNewContextThenOriginalContextThenOriginalContextSaved() throws Exception {
+		HttpSessionSecurityContextRepository repository = new HttpSessionSecurityContextRepository();
+		SecurityContextPersistenceFilter securityContextPersistenceFilter = new SecurityContextPersistenceFilter(
+				repository);
+
+		UserDetails original = User.withUsername("user").password("password").roles("USER").build();
+		SecurityContext originalContext = createSecurityContext(original);
+		UserDetails impersonate = User.withUserDetails(original).username("impersonate").build();
+		SecurityContext impersonateContext = createSecurityContext(impersonate);
+
+		MockHttpServletRequest mockRequest = new MockHttpServletRequest();
+		MockHttpServletResponse mockResponse = new MockHttpServletResponse();
+
+		Filter saveImpersonateContext = (request, response, chain) -> {
+			SecurityContextHolder.setContext(impersonateContext);
+			// ensure the response is committed to trigger save
+			response.flushBuffer();
+			chain.doFilter(request, response);
+		};
+		Filter saveOriginalContext = (request, response, chain) -> {
+			SecurityContextHolder.setContext(originalContext);
+			chain.doFilter(request, response);
+		};
+		HttpServlet servlet = new HttpServlet() {
+			@Override
+			protected void service(HttpServletRequest req, HttpServletResponse resp)
+					throws ServletException, IOException {
+				resp.getWriter().write("Hi");
+			}
+		};
+
+		SecurityContextHolder.setContext(originalContext);
+		MockFilterChain chain = new MockFilterChain(servlet, saveImpersonateContext, saveOriginalContext);
+
+		securityContextPersistenceFilter.doFilter(mockRequest, mockResponse, chain);
+
+		assertThat(
+				mockRequest.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY))
+						.isEqualTo(originalContext);
+	}
+
 	@Test
 	public void nonSecurityContextInSessionIsIgnored() {
 		HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository();
@@ -577,6 +628,13 @@ public class HttpSessionSecurityContextRepositoryTests {
 		assertThat(session).isNull();
 	}
 
+	private SecurityContext createSecurityContext(UserDetails userDetails) {
+		UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken(userDetails,
+				userDetails.getPassword(), userDetails.getAuthorities());
+		SecurityContext securityContext = new SecurityContextImpl(token);
+		return securityContext;
+	}
+
 	@Transient
 	private static class SomeTransientAuthentication extends AbstractAuthenticationToken {