From 65ec2659c40cc5c2488f07cbc51a894cbc32c290 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Fri, 18 Feb 2022 15:13:49 -0600 Subject: [PATCH] HttpSessionSecurityContextRepository saves with original response Previously, the HttpSessionSecurityContextRepository unnecessarily required the HttpServletResponse from the HttpReqeustResponseHolder passed into loadContext. This meant code that wanted to save a SecurityContext had to have a reference to the original HttpRequestResponseHolder. Often that implied that the code that saves the SecurityContext must also load the SecurityContext. This change allows any request / response to be used to save the SecurityContext which means any code can save the SecurityContext not just the code that loaded it. This sets up the code to be permit requiring explicit saves. Using the request/response from the HttpRequestResponseHolder is only necessary for implicit saves. Closes gh-10947 --- .../web/context/HttpSessionSecurityContextRepository.java | 7 +++++-- .../security/web/context/SecurityContextRepository.java | 5 ++++- .../HttpSessionSecurityContextRepositoryTests.java | 8 +++++--- 3 files changed, 14 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 c5316b865e..fae05d41da 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 @@ -134,8 +134,11 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo public void saveContext(SecurityContext context, HttpServletRequest request, HttpServletResponse response) { SaveContextOnUpdateOrErrorResponseWrapper responseWrapper = WebUtils.getNativeResponse(response, SaveContextOnUpdateOrErrorResponseWrapper.class); - Assert.state(responseWrapper != null, () -> "Cannot invoke saveContext on response " + response - + ". You must use the HttpRequestResponseHolder.response after invoking loadContext"); + if (responseWrapper == null) { + boolean httpSessionExists = request.getSession(false) != null; + SecurityContext initialContext = SecurityContextHolder.createEmptyContext(); + responseWrapper = new SaveToSessionResponseWrapper(response, request, httpSessionExists, initialContext); + } responseWrapper.saveContext(context); } diff --git a/web/src/main/java/org/springframework/security/web/context/SecurityContextRepository.java b/web/src/main/java/org/springframework/security/web/context/SecurityContextRepository.java index 506dda8e38..3a6806bdc9 100644 --- a/web/src/main/java/org/springframework/security/web/context/SecurityContextRepository.java +++ b/web/src/main/java/org/springframework/security/web/context/SecurityContextRepository.java @@ -48,9 +48,12 @@ public interface SecurityContextRepository { * to return wrapped versions of the request or response (or both), allowing them to * access implementation-specific state for the request. The values obtained from the * holder will be passed on to the filter chain and also to the saveContext - * method when it is finally called. Implementations may wish to return a subclass of + * method when it is finally called to allow implicit saves of the + * SecurityContext. Implementations may wish to return a subclass of * {@link SaveContextOnUpdateOrErrorResponseWrapper} as the response object, which * guarantees that the context is persisted when an error or redirect occurs. + * Implementations may allow passing in the original request response to allow + * explicit saves. * @param requestResponseHolder holder for the current request and response for which * the context should be loaded. * @return The security context which should be used for the current request, never 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 e8856f59a1..d0b0209131 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 @@ -58,7 +58,6 @@ import org.springframework.security.core.userdetails.UserDetails; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; -import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -581,13 +580,16 @@ public class HttpSessionSecurityContextRepositoryTests { } @Test - public void failsWithStandardResponse() { + public void standardResponseWorks() { HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository(); MockHttpServletRequest request = new MockHttpServletRequest(); MockHttpServletResponse response = new MockHttpServletResponse(); SecurityContext context = SecurityContextHolder.createEmptyContext(); context.setAuthentication(this.testToken); - assertThatIllegalStateException().isThrownBy(() -> repo.saveContext(context, request, response)); + repo.saveContext(context, request, response); + assertThat(request.getSession(false)).isNotNull(); + assertThat(request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)) + .isEqualTo(context); } @Test