Fix SessionRegistryImpl losing track of session when ChangeSessionIdAuthenticationStrategy runs without HttpSessionEventPublisher

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.
  * <p>
- * For this class to function correctly in a web application, it is important that you
- * register an <a href="
- * {@docRoot}/org/springframework/security/web/session/HttpSessionEventPublisher.html">HttpSessionEventPublisher</a> in
- * the <tt>web.xml</tt> 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 <a href="
+ * {@docRoot}/org/springframework/security/web/session/HttpSessionEventPublisher.html">HttpSessionEventPublisher</a>
+ * in the {@code web.xml} file (or equivalent servlet container configuration).
+ * <p>
+ * 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

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<SessionInformation> info = this.sessionRegistry.getAllSessions(principal, false);
 		for (SessionInformation sessionInformation : info) {

web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java

@@ -116,17 +116,23 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r
 	 * subclasses to plug in additional behaviour. *
 	 * <p>
 	 * 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
 	 */
 	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));
 	}
 
 	/**

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;
+	}
+
+}

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<ApplicationEvent> captor = ArgumentCaptor.forClass(ApplicationEvent.class);
+		verify(eventPublisher, times(2)).publishEvent(captor.capture());
+		List<ApplicationEvent> 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);
+	}
+
 }

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<ApplicationEvent> 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<ApplicationEvent> 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