From 06a1cb7730fe41278502760d080a37faf0d6d5d1 Mon Sep 17 00:00:00 2001 From: Nick Williams Date: Tue, 19 Feb 2013 13:58:01 -0600 Subject: [PATCH 1/2] SEC-2002: Added events to notify of session ID change Session fixation protection, whether by clean new session or migrated session, now publishes an event when a session is migrated or its ID is changed. This enables application developers to keep track of the session ID of a particular authentication from the time the authentication is successful until the time of logout. Previously this was not possible since session migration changed the session ID and there was no way to reliably detect that. --- .../SessionFixationProtectionStrategy.java | 22 ++- .../session/SessionIdChangedEvent.java | 73 ++++++++++ .../session/SessionMigrationEvent.java | 41 ++++++ ...ConcurrentSessionControlStrategyTests.java | 20 +++ ...ultSessionAuthenticationStrategyTests.java | 128 ++++++++++++++++-- 5 files changed, 272 insertions(+), 12 deletions(-) create mode 100644 web/src/main/java/org/springframework/security/web/authentication/session/SessionIdChangedEvent.java create mode 100644 web/src/main/java/org/springframework/security/web/authentication/session/SessionMigrationEvent.java diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java index 95326452127..f9250aea76d 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java @@ -2,6 +2,8 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.security.core.Authentication; import org.springframework.util.Assert; @@ -36,9 +38,12 @@ * @author Luke Taylor * @since 3.0 */ -public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy { +public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy, + ApplicationEventPublisherAware { protected final Log logger = LogFactory.getLog(this.getClass()); + private ApplicationEventPublisher applicationEventPublisher; + /** * Indicates that the session attributes of an existing session * should be migrated to the new session. Defaults to true. @@ -112,12 +117,23 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r /** * Called when the session has been changed and the old attributes have been migrated to the new session. * Only called if a session existed to start with. Allows subclasses to plug in additional behaviour. + *

+ * The default implementation of this method publishes the appropriate {@link SessionMigrationEvent} (if + * {@code migrateSessionAttributes} is {@code true}) or {@link SessionIdChangedEvent} (if + * {@code migrateSessionAttributes} is {@code false}). 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) { + String newId = newSession.getId(); + if(migrateSessionAttributes) { + applicationEventPublisher.publishEvent(new SessionMigrationEvent(auth, originalSessionId, newId)); + } else { + applicationEventPublisher.publishEvent(new SessionIdChangedEvent(auth, originalSessionId, newId)); + } } /** @@ -193,6 +209,10 @@ public void setMigrateSessionAttributes(boolean migrateSessionAttributes) { this.migrateSessionAttributes = migrateSessionAttributes; } + public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { + this.applicationEventPublisher = applicationEventPublisher; + } + /** * @deprecated Override the {@code extractAttributes} method instead */ diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionIdChangedEvent.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionIdChangedEvent.java new file mode 100644 index 00000000000..bdf4b2e3722 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionIdChangedEvent.java @@ -0,0 +1,73 @@ +/* + * Copyright 2013 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 + * + * http://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 org.springframework.security.authentication.event.AbstractAuthenticationEvent; +import org.springframework.security.core.Authentication; +import org.springframework.util.Assert; + +/** + * Indicates a session ID was changed for the purposes of session fixation protection. + * + * @since 3.2 + * @see SessionFixationProtectionStrategy + * @author Nick Williams + */ +public class SessionIdChangedEvent extends AbstractAuthenticationEvent { + //~ Instance fields ================================================================================================ + + private final String oldSessionId; + + private final String newSessionId; + + //~ Constructors =================================================================================================== + + /** + * Constructs a new session ID changed event. + * + * @param authentication The authentication object + * @param oldSessionId The old session ID before it was changed + * @param newSessionId The new session ID after it was changed + */ + public SessionIdChangedEvent(Authentication authentication, String oldSessionId, String newSessionId) { + super(authentication); + Assert.hasLength(oldSessionId); + Assert.hasLength(newSessionId); + this.oldSessionId = oldSessionId; + this.newSessionId = newSessionId; + } + + //~ Methods ======================================================================================================== + + /** + * Getter for the session ID before it was changed. + * + * @return the old session ID. + */ + public String getOldSessionId() { + return this.oldSessionId; + } + + /** + * Getter for the session ID after it was changed. + * + * @return the new session ID. + */ + public String getNewSessionId() { + return this.newSessionId; + } +} diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionMigrationEvent.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionMigrationEvent.java new file mode 100644 index 00000000000..dd644bb213d --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionMigrationEvent.java @@ -0,0 +1,41 @@ +/* + * Copyright 2013 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 + * + * http://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 org.springframework.security.core.Authentication; + +/** + * Indicates a session was migrated in for the purposes of session fixation protection. + * + * @since 3.2 + * @see SessionFixationProtectionStrategy + * @author Nick Williams + */ +public class SessionMigrationEvent extends SessionIdChangedEvent { + //~ Constructors =================================================================================================== + + /** + * Constructs a new session migration event. + * + * @param authentication The authentication object + * @param oldSessionId The old session ID before the session was migrated + * @param newSessionId The new session ID after the session was migrated + */ + public SessionMigrationEvent(Authentication authentication, String oldSessionId, String newSessionId) { + super(authentication, oldSessionId, newSessionId); + } +} diff --git a/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java b/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java index c997162fdbf..d5c217d155b 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java @@ -1,17 +1,22 @@ package org.springframework.security.web.authentication.session; +import static org.junit.Assert.*; import static org.mockito.AdditionalMatchers.not; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.context.ApplicationEvent; +import org.springframework.context.ApplicationEventPublisher; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.core.Authentication; @@ -55,9 +60,24 @@ public void onAuthenticationNewSession() { public void onAuthenticationChangeSession() { String originalSessionId = request.getSession().getId(); + // See SEC-2002: Make sure SessionIdChangedEvent or subclass is published + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + strategy.onAuthentication(authentication, request, response); verify(sessionRegistry,times(0)).removeSessionInformation(anyString()); verify(sessionRegistry).registerNewSession(not(eq(originalSessionId)), anyObject()); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + // See SEC-2002: Make sure SessionIdChangedEvent or subclass is published + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionIdChangedEvent); + SessionIdChangedEvent event = (SessionIdChangedEvent)eventArgumentCaptor.getValue(); + assertEquals(originalSessionId, event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(authentication, event.getAuthentication()); } } 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 63ccc731767..88985711743 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 @@ -1,17 +1,24 @@ package org.springframework.security.web.session; -import static org.junit.Assert.*; -import static org.mockito.Mockito.mock; - -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpSession; - import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.context.ApplicationEvent; +import org.springframework.context.ApplicationEventPublisher; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.core.Authentication; import org.springframework.security.web.authentication.session.SessionFixationProtectionStrategy; -import org.springframework.security.web.savedrequest.HttpSessionRequestCache; +import org.springframework.security.web.authentication.session.SessionIdChangedEvent; +import org.springframework.security.web.authentication.session.SessionMigrationEvent; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import java.lang.reflect.Method; + +import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; /** * @@ -33,11 +40,33 @@ public void newSessionShouldNotBeCreatedIfNoSessionExistsAndAlwaysCreateIsFalse( public void newSessionIsCreatedIfSessionAlreadyExists() throws Exception { SessionFixationProtectionStrategy strategy = new SessionFixationProtectionStrategy(); HttpServletRequest request = new MockHttpServletRequest(); - String sessionId = request.getSession().getId(); + HttpSession session = request.getSession(); + session.setAttribute("blah", "blah"); + session.setAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY", "DefaultSavedRequest"); + String oldSessionId = session.getId(); - strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse()); + // See SEC-2002: Make sure SessionMigrationEvent is published + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse()); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); - assertFalse(sessionId.equals(request.getSession().getId())); + assertFalse(oldSessionId.equals(request.getSession().getId())); + assertNotNull(request.getSession().getAttribute("blah")); + assertNotNull(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")); + + // See SEC-2002: Make sure SessionMigrationEvent is published + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionMigrationEvent); + SessionMigrationEvent event = (SessionMigrationEvent)eventArgumentCaptor.getValue(); + assertEquals(oldSessionId, event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); } // See SEC-1077 @@ -49,11 +78,30 @@ public void onlySavedRequestAttributeIsMigratedIfMigrateAttributesIsFalse() thro HttpSession session = request.getSession(); session.setAttribute("blah", "blah"); session.setAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY", "DefaultSavedRequest"); + String oldSessionId = session.getId(); - strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse()); + // See SEC-2002: Make sure SessionIdChangedEvent (not SessionMigrationEvent) is published + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse()); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); assertNull(request.getSession().getAttribute("blah")); assertNotNull(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")); + + // See SEC-2002: Make sure SessionIdChangedEvent (not SessionMigrationEvent) is published + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionIdChangedEvent); + assertFalse(eventArgumentCaptor.getValue() instanceof SessionMigrationEvent); + SessionIdChangedEvent event = (SessionIdChangedEvent)eventArgumentCaptor.getValue(); + assertEquals(oldSessionId, event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); } @Test @@ -65,4 +113,62 @@ public void sessionIsCreatedIfAlwaysCreateTrue() throws Exception { assertNotNull(request.getSession(false)); } + // See SEC-2002 + @Test + public void onSessionChangePublishesMigrationEventIfMigrateAttributesIsTrue() throws Exception { + SessionFixationProtectionStrategy strategy = new SessionFixationProtectionStrategy(); + HttpServletRequest request = new MockHttpServletRequest(); + HttpSession session = request.getSession(); + + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + onSessionChange(strategy, "oldId01", session, mockAuthentication); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionMigrationEvent); + SessionMigrationEvent event = (SessionMigrationEvent)eventArgumentCaptor.getValue(); + assertEquals("oldId01", event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); + } + + @Test + public void onSessionChangePublishesIdChangeEventIfMigrateAttributesIsFalse() throws Exception { + SessionFixationProtectionStrategy strategy = new SessionFixationProtectionStrategy(); + strategy.setMigrateSessionAttributes(false); + HttpServletRequest request = new MockHttpServletRequest(); + HttpSession session = request.getSession(); + + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + onSessionChange(strategy, "oldId02", session, mockAuthentication); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionIdChangedEvent); + assertFalse(eventArgumentCaptor.getValue() instanceof SessionMigrationEvent); + SessionIdChangedEvent event = (SessionIdChangedEvent)eventArgumentCaptor.getValue(); + assertEquals("oldId02", event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); + } + + private void onSessionChange(SessionFixationProtectionStrategy strategy, String id, HttpSession s, Authentication a) + throws Exception{ + Method method = strategy.getClass() + .getDeclaredMethod("onSessionChange", String.class, HttpSession.class, Authentication.class); + method.setAccessible(true); + method.invoke(strategy, id, s, a); + } } From b40b122988f724146f83b90310c8755ff4390a26 Mon Sep 17 00:00:00 2001 From: Nick Williams Date: Tue, 19 Feb 2013 13:58:01 -0600 Subject: [PATCH 2/2] SEC-2002: Added events to notify of session ID change Session fixation protection, whether by clean new session or migrated session, now publishes an event when a session is migrated or its ID is changed. This enables application developers to keep track of the session ID of a particular authentication from the time the authentication is successful until the time of logout. Previously this was not possible since session migration changed the session ID and there was no way to reliably detect that. Revised changes per Rob Winch's suggestions. --- .../SessionFixationProtectionEvent.java | 73 +++++++++ ...ssionFixationProtectionMigrationEvent.java | 59 +++++++ .../SessionFixationProtectionStrategy.java | 21 ++- ...ConcurrentSessionControlStrategyTests.java | 20 +++ ...ultSessionAuthenticationStrategyTests.java | 150 ++++++++++++++++-- 5 files changed, 311 insertions(+), 12 deletions(-) create mode 100644 web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionEvent.java create mode 100644 web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionMigrationEvent.java diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionEvent.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionEvent.java new file mode 100644 index 00000000000..d1e6b4c3b8d --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionEvent.java @@ -0,0 +1,73 @@ +/* + * Copyright 2002-2012 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 + * + * http://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 org.springframework.security.authentication.event.AbstractAuthenticationEvent; +import org.springframework.security.core.Authentication; +import org.springframework.util.Assert; + +/** + * Indicates a session ID was changed for the purposes of session fixation protection. + * + * @since 3.2 + * @see SessionFixationProtectionStrategy + * @author Nick Williams + */ +public class SessionFixationProtectionEvent extends AbstractAuthenticationEvent { + //~ Instance fields ================================================================================================ + + private final String oldSessionId; + + private final String newSessionId; + + //~ Constructors =================================================================================================== + + /** + * Constructs a new session fixation protection event. + * + * @param authentication The authentication object + * @param oldSessionId The old session ID before it was changed + * @param newSessionId The new session ID after it was changed + */ + public SessionFixationProtectionEvent(Authentication authentication, String oldSessionId, String newSessionId) { + super(authentication); + Assert.hasLength(oldSessionId); + Assert.hasLength(newSessionId); + this.oldSessionId = oldSessionId; + this.newSessionId = newSessionId; + } + + //~ Methods ======================================================================================================== + + /** + * Getter for the session ID before it was changed. + * + * @return the old session ID. + */ + public String getOldSessionId() { + return this.oldSessionId; + } + + /** + * Getter for the session ID after it was changed. + * + * @return the new session ID. + */ + public String getNewSessionId() { + return this.newSessionId; + } +} diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionMigrationEvent.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionMigrationEvent.java new file mode 100644 index 00000000000..ef048b5c97f --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionMigrationEvent.java @@ -0,0 +1,59 @@ +/* + * Copyright 2002-2012 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 + * + * http://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 org.springframework.security.core.Authentication; + +/** + * Indicates a session was migrated for the purposes of session fixation protection. + * + * @since 3.2 + * @see SessionFixationProtectionStrategy + * @author Nick Williams + */ +public class SessionFixationProtectionMigrationEvent extends SessionFixationProtectionEvent { + //~ Instance fields ================================================================================================ + + private final boolean sessionAttributesMigrated; + + //~ Constructors =================================================================================================== + + /** + * Constructs a new session migration event. + * + * @param authentication The authentication object + * @param oldSessionId The old session ID before the session was migrated + * @param newSessionId The new session ID after the session was migrated + * @param sessionAttributesMigrated Whether or not all session attributes were migrated + */ + public SessionFixationProtectionMigrationEvent(Authentication authentication, String oldSessionId, + String newSessionId, boolean sessionAttributesMigrated) { + super(authentication, oldSessionId, newSessionId); + this.sessionAttributesMigrated = sessionAttributesMigrated; + } + + /** + * Getter that indicates whether all session attributes were migrated. If all session attributes were not migrated + * (due to the session fixation protection strategy being "new session"), the Spring Security-related session + * attributes were still migrated, regardless. + * + * @return {@code true} if all session attributes were migrated, {@code false} otherwise. + */ + public boolean sessionAttributesWereMigrated() { + return this.sessionAttributesMigrated; + } +} diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java index 95326452127..68718335a07 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java @@ -2,6 +2,8 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.security.core.Authentication; import org.springframework.util.Assert; @@ -36,9 +38,12 @@ * @author Luke Taylor * @since 3.0 */ -public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy { +public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy, + ApplicationEventPublisherAware { protected final Log logger = LogFactory.getLog(this.getClass()); + private ApplicationEventPublisher applicationEventPublisher; + /** * Indicates that the session attributes of an existing session * should be migrated to the new session. Defaults to true. @@ -112,12 +117,22 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r /** * Called when the session has been changed and the old attributes have been migrated to the new session. * Only called if a session existed to start with. Allows subclasses to plug in additional behaviour. + *

+ * The default implementation of this method publishes a {@link SessionFixationProtectionMigrationEvent} to notify + * the application that the session ID has changed and about which attributes were migrated. 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) { + if(applicationEventPublisher != null) { + applicationEventPublisher.publishEvent(new SessionFixationProtectionMigrationEvent( + auth, originalSessionId, newSession.getId(), migrateSessionAttributes + )); + } } /** @@ -193,6 +208,10 @@ public void setMigrateSessionAttributes(boolean migrateSessionAttributes) { this.migrateSessionAttributes = migrateSessionAttributes; } + public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { + this.applicationEventPublisher = applicationEventPublisher; + } + /** * @deprecated Override the {@code extractAttributes} method instead */ diff --git a/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java b/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java index c997162fdbf..a4ea70904f7 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/session/ConcurrentSessionControlStrategyTests.java @@ -1,17 +1,22 @@ package org.springframework.security.web.authentication.session; +import static org.junit.Assert.*; import static org.mockito.AdditionalMatchers.not; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.context.ApplicationEvent; +import org.springframework.context.ApplicationEventPublisher; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.core.Authentication; @@ -55,9 +60,24 @@ public void onAuthenticationNewSession() { public void onAuthenticationChangeSession() { String originalSessionId = request.getSession().getId(); + // See SEC-2002: Make sure SessionFixationProtectionEvent or subclass is published + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + strategy.onAuthentication(authentication, request, response); verify(sessionRegistry,times(0)).removeSessionInformation(anyString()); verify(sessionRegistry).registerNewSession(not(eq(originalSessionId)), anyObject()); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + // See SEC-2002: Make sure SessionFixationProtectionEvent or subclass is published + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionEvent); + SessionFixationProtectionEvent event = (SessionFixationProtectionEvent)eventArgumentCaptor.getValue(); + assertEquals(originalSessionId, event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(authentication, event.getAuthentication()); } } 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 63ccc731767..28e52e4b6cc 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 @@ -1,17 +1,22 @@ package org.springframework.security.web.session; -import static org.junit.Assert.*; -import static org.mockito.Mockito.mock; - -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpSession; - import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.context.ApplicationEvent; +import org.springframework.context.ApplicationEventPublisher; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.core.Authentication; +import org.springframework.security.web.authentication.session.SessionFixationProtectionMigrationEvent; import org.springframework.security.web.authentication.session.SessionFixationProtectionStrategy; -import org.springframework.security.web.savedrequest.HttpSessionRequestCache; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; + +import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; /** * @@ -33,11 +38,35 @@ public void newSessionShouldNotBeCreatedIfNoSessionExistsAndAlwaysCreateIsFalse( public void newSessionIsCreatedIfSessionAlreadyExists() throws Exception { SessionFixationProtectionStrategy strategy = new SessionFixationProtectionStrategy(); HttpServletRequest request = new MockHttpServletRequest(); - String sessionId = request.getSession().getId(); + HttpSession session = request.getSession(); + session.setAttribute("blah", "blah"); + session.setAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY", "DefaultSavedRequest"); + String oldSessionId = session.getId(); - strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse()); + // See SEC-2002: Make sure SessionFixationProtectionMigrationEvent is published + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse()); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + assertFalse(oldSessionId.equals(request.getSession().getId())); + assertNotNull(request.getSession().getAttribute("blah")); + assertNotNull(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")); - assertFalse(sessionId.equals(request.getSession().getId())); + // See SEC-2002: Make sure SessionFixationProtectionMigrationEvent is published + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionMigrationEvent); + SessionFixationProtectionMigrationEvent event = + (SessionFixationProtectionMigrationEvent)eventArgumentCaptor.getValue(); + assertEquals(oldSessionId, event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); + assertTrue(event.sessionAttributesWereMigrated()); } // See SEC-1077 @@ -49,11 +78,31 @@ public void onlySavedRequestAttributeIsMigratedIfMigrateAttributesIsFalse() thro HttpSession session = request.getSession(); session.setAttribute("blah", "blah"); session.setAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY", "DefaultSavedRequest"); + String oldSessionId = session.getId(); - strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse()); + // See SEC-2002: Make sure SessionFixationProtectionMigrationEvent is published + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse()); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); assertNull(request.getSession().getAttribute("blah")); assertNotNull(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")); + + // See SEC-2002: Make sure SessionFixationProtectionMigrationEvent is published + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionMigrationEvent); + SessionFixationProtectionMigrationEvent event = + (SessionFixationProtectionMigrationEvent)eventArgumentCaptor.getValue(); + assertEquals(oldSessionId, event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); + assertFalse(event.sessionAttributesWereMigrated()); } @Test @@ -65,4 +114,83 @@ public void sessionIsCreatedIfAlwaysCreateTrue() throws Exception { assertNotNull(request.getSession(false)); } + // See SEC-2002 + @Test + public void onSessionChangePublishesMigrationEventIfMigrateAttributesIsTrue() throws Exception { + SessionFixationProtectionStrategyWithPublicOnSessionChange strategy = + new SessionFixationProtectionStrategyWithPublicOnSessionChange(); + HttpServletRequest request = new MockHttpServletRequest(); + HttpSession session = request.getSession(); + + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onSessionChange("oldId01", session, mockAuthentication); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionMigrationEvent); + SessionFixationProtectionMigrationEvent event = + (SessionFixationProtectionMigrationEvent)eventArgumentCaptor.getValue(); + assertEquals("oldId01", event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); + assertTrue(event.sessionAttributesWereMigrated()); + } + + // See SEC-2002 + @Test + public void onSessionChangePublishesIdChangeEventIfMigrateAttributesIsFalse() throws Exception { + SessionFixationProtectionStrategyWithPublicOnSessionChange strategy = + new SessionFixationProtectionStrategyWithPublicOnSessionChange(); + strategy.setMigrateSessionAttributes(false); + HttpServletRequest request = new MockHttpServletRequest(); + HttpSession session = request.getSession(); + + ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class); + strategy.setApplicationEventPublisher(eventPublisher); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onSessionChange("oldId02", session, mockAuthentication); + + ArgumentCaptor eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class); + verify(eventPublisher).publishEvent(eventArgumentCaptor.capture()); + + assertNotNull(eventArgumentCaptor.getValue()); + assertTrue(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionMigrationEvent); + SessionFixationProtectionMigrationEvent event = + (SessionFixationProtectionMigrationEvent)eventArgumentCaptor.getValue(); + assertEquals("oldId02", event.getOldSessionId()); + assertEquals(request.getSession().getId(), event.getNewSessionId()); + assertSame(mockAuthentication, event.getAuthentication()); + assertFalse(event.sessionAttributesWereMigrated()); + } + + // See SEC-2002 + @Test + public void onSessionChangeDoesNotThrowNullPointerExceptionIfEventPublisherNotSet() throws Exception { + SessionFixationProtectionStrategyWithPublicOnSessionChange strategy = + new SessionFixationProtectionStrategyWithPublicOnSessionChange(); + strategy.setMigrateSessionAttributes(false); + HttpServletRequest request = new MockHttpServletRequest(); + HttpSession session = request.getSession(); + + Authentication mockAuthentication = mock(Authentication.class); + + strategy.onSessionChange("oldId03", session, mockAuthentication); + } + + // See SEC-2002 + private static class SessionFixationProtectionStrategyWithPublicOnSessionChange + extends SessionFixationProtectionStrategy { + @Override + public void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) { + super.onSessionChange(originalSessionId, newSession , auth); + } + } }