diff --git a/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java b/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java index df5b86b027f..b2706275f9d 100644 --- a/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java +++ b/core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java @@ -38,12 +38,18 @@ * Default implementation of * {@link org.springframework.security.core.session.SessionRegistry SessionRegistry} which * listens for {@link org.springframework.security.core.session.SessionDestroyedEvent - * SessionDestroyedEvent}s published in the Spring application context. + * SessionDestroyedEvent}s and + * {@link org.springframework.security.core.session.SessionIdChangedEvent + * SessionIdChangedEvent}s published in the Spring application context. *

- * For this class to function correctly in a web application, it is important that you - * register an HttpSessionEventPublisher in - * the web.xml file so that this class is notified of sessions that expire. + * For this class to be notified of sessions that expire, you must register an HttpSessionEventPublisher + * in the {@code web.xml} file (or equivalent servlet container configuration). + *

+ * Session ID changes that occur as part of session fixation protection (e.g. via + * {@link org.springframework.security.web.authentication.session.ChangeSessionIdAuthenticationStrategy}) + * are tracked automatically without requiring + * {@code HttpSessionEventPublisher} to be registered. * * @author Ben Alex * @author Luke Taylor diff --git a/core/src/test/java/org/springframework/security/core/session/SessionRegistryImplTests.java b/core/src/test/java/org/springframework/security/core/session/SessionRegistryImplTests.java index 9e5e2ecf474..7205123b1bb 100644 --- a/core/src/test/java/org/springframework/security/core/session/SessionRegistryImplTests.java +++ b/core/src/test/java/org/springframework/security/core/session/SessionRegistryImplTests.java @@ -192,6 +192,29 @@ public String getNewSessionId() { assertThat(this.sessionRegistry.getSessionInformation(newSessionId)).isNull(); } + @Test + public void sessionIdChangedEventWhenOldSessionRegisteredThenMigratesSessionWithoutHttpSessionEventPublisher() { + Object principal = "Some principal object"; + final String oldSessionId = "old-session-id"; + final String newSessionId = "new-session-id"; + this.sessionRegistry.registerNewSession(oldSessionId, principal); + this.sessionRegistry.onApplicationEvent(new SessionIdChangedEvent("") { + @Override + public String getOldSessionId() { + return oldSessionId; + } + + @Override + public String getNewSessionId() { + return newSessionId; + } + }); + assertThat(this.sessionRegistry.getSessionInformation(oldSessionId)).isNull(); + assertThat(this.sessionRegistry.getSessionInformation(newSessionId)).isNotNull(); + assertThat(this.sessionRegistry.getSessionInformation(newSessionId).getPrincipal()).isEqualTo(principal); + assertThat(this.sessionRegistry.getAllSessions(principal, false)).hasSize(1); + } + private boolean contains(String sessionId, Object principal) { List info = this.sessionRegistry.getAllSessions(principal, false); for (SessionInformation sessionInformation : info) { diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java index ed0788d8458..4bab9eaad7f 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java +++ b/web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java @@ -116,10 +116,14 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r * subclasses to plug in additional behaviour. * *

* The default implementation of this method publishes a - * {@link SessionFixationProtectionEvent} to notify the application that the session - * ID has changed. If you override this method and still wish these events to be - * published, you should call {@code super.onSessionChange()} within your overriding - * method. + * {@link SessionFixationProtectionEvent} and a + * {@link SessionFixationProtectionSessionIdChangedEvent} to notify the application + * that the session ID has changed. The latter allows + * {@link org.springframework.security.core.session.SessionRegistryImpl} to track the + * session ID change without requiring + * {@link org.springframework.security.web.session.HttpSessionEventPublisher} to be + * registered. If you override this method and still wish these events to be published, + * you should call {@code super.onSessionChange()} within your overriding method. * @param originalSessionId the original session identifier * @param newSession the newly created session * @param auth the token for the newly authenticated principal @@ -127,6 +131,8 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r protected void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) { this.applicationEventPublisher .publishEvent(new SessionFixationProtectionEvent(auth, originalSessionId, newSession.getId())); + this.applicationEventPublisher + .publishEvent(new SessionFixationProtectionSessionIdChangedEvent(newSession, originalSessionId)); } /** diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionSessionIdChangedEvent.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionSessionIdChangedEvent.java new file mode 100644 index 00000000000..4bf42b4d0c5 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionSessionIdChangedEvent.java @@ -0,0 +1,62 @@ +/* + * Copyright 2004-present 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 + * + * https://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.web.authentication.session; + +import java.io.Serial; + +import jakarta.servlet.http.HttpSession; + +import org.springframework.security.core.session.SessionIdChangedEvent; + +/** + * Published by {@link AbstractSessionFixationProtectionStrategy} when a session ID + * changes during session fixation protection. This allows + * {@link org.springframework.security.core.session.SessionRegistryImpl} to track the + * session ID change without requiring + * {@link org.springframework.security.web.session.HttpSessionEventPublisher} to be + * registered. + * + * @author Adolfo Gonzalez + * @since 6.5 + * @see AbstractSessionFixationProtectionStrategy + */ +class SessionFixationProtectionSessionIdChangedEvent extends SessionIdChangedEvent { + + @Serial + private static final long serialVersionUID = 1L; + + private final String oldSessionId; + + private final String newSessionId; + + SessionFixationProtectionSessionIdChangedEvent(HttpSession newSession, String oldSessionId) { + super(newSession); + this.oldSessionId = oldSessionId; + this.newSessionId = newSession.getId(); + } + + @Override + public String getOldSessionId() { + return this.oldSessionId; + } + + @Override + public String getNewSessionId() { + return this.newSessionId; + } + +} diff --git a/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java b/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java index 8458d5b7bf6..8cc7700e4db 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java @@ -16,11 +16,24 @@ package org.springframework.security.web.authentication.session; +import java.util.List; + import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.context.ApplicationEvent; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.context.event.SimpleApplicationEventMulticaster; import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.session.SessionIdChangedEvent; +import org.springframework.security.core.session.SessionRegistryImpl; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; /** * @author Rob Winch @@ -36,4 +49,49 @@ public void applySessionFixation() { assertThat(request.getSession().getId()).isNotEqualTo(id); } + @Test + public void onAuthenticationWhenRegistryHasOldSessionThenMigratesWithoutHttpSessionEventPublisher() { + // Reproduces gh-19007: without HttpSessionEventPublisher the old session ID used + // to remain as a ghost entry in the registry after session fixation rotation, + // causing ConcurrentSessionControlAuthenticationStrategy to count an extra session. + SessionRegistryImpl registry = new SessionRegistryImpl(); + SimpleApplicationEventMulticaster multicaster = new SimpleApplicationEventMulticaster(); + multicaster.addApplicationListener(registry); + ChangeSessionIdAuthenticationStrategy strategy = new ChangeSessionIdAuthenticationStrategy(); + strategy.setApplicationEventPublisher((event) -> { + if (event instanceof ApplicationEvent applicationEvent) { + multicaster.multicastEvent(applicationEvent); + } + }); + MockHttpServletRequest request = new MockHttpServletRequest(); + Object principal = "testPrincipal"; + registry.registerNewSession(request.getSession().getId(), principal); + strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse()); + String newSessionId = request.getSession().getId(); + assertThat(registry.getSessionInformation(newSessionId)).isNotNull(); + assertThat(registry.getAllSessions(principal, false)).hasSize(1); + } + + @Test + public void onAuthenticationPublishesSessionFixationAndSessionIdChangedEvents() { + ChangeSessionIdAuthenticationStrategy strategy = new ChangeSessionIdAuthenticationStrategy(); + MockHttpServletRequest request = new MockHttpServletRequest(); + String oldSessionId = request.getSession().getId(); + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse()); + ArgumentCaptor captor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher, times(2)).publishEvent(captor.capture()); + List events = captor.getAllValues(); + assertThat(events).hasAtLeastOneElementOfType(SessionFixationProtectionEvent.class); + SessionIdChangedEvent idChangedEvent = events.stream() + .filter(SessionIdChangedEvent.class::isInstance) + .map(SessionIdChangedEvent.class::cast) + .findFirst() + .orElseThrow(); + assertThat(idChangedEvent.getOldSessionId()).isEqualTo(oldSessionId); + assertThat(idChangedEvent.getNewSessionId()).isEqualTo(request.getSession().getId()); + assertThat(idChangedEvent.getNewSessionId()).isNotEqualTo(oldSessionId); + } + } diff --git a/web/src/test/java/org/springframework/security/web/session/DefaultSessionAuthenticationStrategyTests.java b/web/src/test/java/org/springframework/security/web/session/DefaultSessionAuthenticationStrategyTests.java index 5d12e1531d7..b3a7d8c5908 100644 --- a/web/src/test/java/org/springframework/security/web/session/DefaultSessionAuthenticationStrategyTests.java +++ b/web/src/test/java/org/springframework/security/web/session/DefaultSessionAuthenticationStrategyTests.java @@ -26,11 +26,13 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.core.Authentication; +import org.springframework.security.core.session.SessionIdChangedEvent; import org.springframework.security.web.authentication.session.SessionFixationProtectionEvent; import org.springframework.security.web.authentication.session.SessionFixationProtectionStrategy; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; /** @@ -69,16 +71,20 @@ public void newSessionIsCreatedIfSessionAlreadyExistsWithEventPublisher() { Authentication mockAuthentication = mock(Authentication.class); strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse()); ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); - verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + verify(eventPublisher, times(2)).publishEvent(eventArgumentCaptor.capture()); assertThat(oldSessionId.equals(request.getSession().getId())).isFalse(); assertThat(request.getSession().getAttribute("blah")).isNotNull(); assertThat(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")).isNotNull(); - assertThat(eventArgumentCaptor.getValue()).isNotNull(); - assertThat(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionEvent).isTrue(); - SessionFixationProtectionEvent event = (SessionFixationProtectionEvent) eventArgumentCaptor.getValue(); - assertThat(event.getOldSessionId()).isEqualTo(oldSessionId); - assertThat(event.getNewSessionId()).isEqualTo(request.getSession().getId()); - assertThat(event.getAuthentication()).isSameAs(mockAuthentication); + assertThat(eventArgumentCaptor.getAllValues().get(0)).isInstanceOf(SessionFixationProtectionEvent.class); + SessionFixationProtectionEvent fixationEvent = (SessionFixationProtectionEvent) eventArgumentCaptor.getAllValues() + .get(0); + assertThat(fixationEvent.getOldSessionId()).isEqualTo(oldSessionId); + assertThat(fixationEvent.getNewSessionId()).isEqualTo(request.getSession().getId()); + assertThat(fixationEvent.getAuthentication()).isSameAs(mockAuthentication); + assertThat(eventArgumentCaptor.getAllValues().get(1)).isInstanceOf(SessionIdChangedEvent.class); + SessionIdChangedEvent idChangedEvent = (SessionIdChangedEvent) eventArgumentCaptor.getAllValues().get(1); + assertThat(idChangedEvent.getOldSessionId()).isEqualTo(oldSessionId); + assertThat(idChangedEvent.getNewSessionId()).isEqualTo(request.getSession().getId()); } // See SEC-1077 @@ -110,15 +116,19 @@ public void onlySavedRequestAttributeIsMigratedIfMigrateAttributesIsFalseWithEve Authentication mockAuthentication = mock(Authentication.class); strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse()); ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); - verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + verify(eventPublisher, times(2)).publishEvent(eventArgumentCaptor.capture()); assertThat(request.getSession().getAttribute("blah")).isNull(); assertThat(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")).isNotNull(); - assertThat(eventArgumentCaptor.getValue()).isNotNull(); - assertThat(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionEvent).isTrue(); - SessionFixationProtectionEvent event = (SessionFixationProtectionEvent) eventArgumentCaptor.getValue(); - assertThat(event.getOldSessionId()).isEqualTo(oldSessionId); - assertThat(event.getNewSessionId()).isEqualTo(request.getSession().getId()); - assertThat(event.getAuthentication()).isSameAs(mockAuthentication); + assertThat(eventArgumentCaptor.getAllValues().get(0)).isInstanceOf(SessionFixationProtectionEvent.class); + SessionFixationProtectionEvent fixationEvent = (SessionFixationProtectionEvent) eventArgumentCaptor.getAllValues() + .get(0); + assertThat(fixationEvent.getOldSessionId()).isEqualTo(oldSessionId); + assertThat(fixationEvent.getNewSessionId()).isEqualTo(request.getSession().getId()); + assertThat(fixationEvent.getAuthentication()).isSameAs(mockAuthentication); + assertThat(eventArgumentCaptor.getAllValues().get(1)).isInstanceOf(SessionIdChangedEvent.class); + SessionIdChangedEvent idChangedEvent = (SessionIdChangedEvent) eventArgumentCaptor.getAllValues().get(1); + assertThat(idChangedEvent.getOldSessionId()).isEqualTo(oldSessionId); + assertThat(idChangedEvent.getNewSessionId()).isEqualTo(request.getSession().getId()); } @Test