From 63734cfcf91bfc5cea5585c79f4328e0cb80e684 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Mon, 2 Aug 2010 22:41:57 +0100 Subject: [PATCH] SEC-1528: Remove logic which checks if context in the session is the same as the current context to make sure that session.setAttribute() is called when the value in the session has been modified directly. --- .../HttpSessionSecurityContextRepository.java | 14 ++-- ...SessionSecurityContextRepositoryTests.java | 65 ++++++++++--------- 2 files changed, 39 insertions(+), 40 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 e0bda53964..cb43c6572c 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 @@ -311,17 +311,13 @@ public class HttpSessionSecurityContextRepository implements SecurityContextRepo } // If HttpSession exists, store current SecurityContextHolder contents but only if - // the SecurityContext has actually changed in this thread (see JIRA SEC-37, SEC-1307) + // the SecurityContext has actually changed in this thread (see JIRA SEC-37, SEC-1307, SEC-1528) if (httpSession != null) { - SecurityContext contextFromSession = (SecurityContext) httpSession.getAttribute(SPRING_SECURITY_CONTEXT_KEY); + if (context != contextBeforeExecution || context.getAuthentication() != authBeforeExecution) { + httpSession.setAttribute(SPRING_SECURITY_CONTEXT_KEY, context); - if (context != contextFromSession) { - if (context != contextBeforeExecution || context.getAuthentication() != authBeforeExecution) { - httpSession.setAttribute(SPRING_SECURITY_CONTEXT_KEY, context); - - if (logger.isDebugEnabled()) { - logger.debug("SecurityContext stored to HttpSession: '" + context + "'"); - } + if (logger.isDebugEnabled()) { + logger.debug("SecurityContext stored to HttpSession: '" + context + "'"); } } } 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 62c450ab0c..419d29790a 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 @@ -1,19 +1,19 @@ package org.springframework.security.web.context; import static org.junit.Assert.*; +import static org.mockito.Mockito.*; +import static org.springframework.security.web.context.HttpSessionSecurityContextRepository.*; + +import javax.servlet.http.HttpSession; import org.junit.Test; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.authentication.TestingAuthenticationToken; -import org.springframework.security.core.Authentication; 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.web.context.HttpRequestResponseHolder; -import org.springframework.security.web.context.HttpSessionSecurityContextRepository; -import org.springframework.security.web.context.SaveContextOnUpdateOrErrorResponseWrapper; public class HttpSessionSecurityContextRepositoryTests { private final TestingAuthenticationToken testToken = new TestingAuthenticationToken("someone", "passwd", "ROLE_A"); @@ -49,7 +49,7 @@ public class HttpSessionSecurityContextRepositoryTests { HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository(); MockHttpServletRequest request = new MockHttpServletRequest(); SecurityContextHolder.getContext().setAuthentication(testToken); - request.getSession().setAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY, SecurityContextHolder.getContext()); + request.getSession().setAttribute(SPRING_SECURITY_CONTEXT_KEY, SecurityContextHolder.getContext()); MockHttpServletResponse response = new MockHttpServletResponse(); HttpRequestResponseHolder holder = new HttpRequestResponseHolder(request, response); SecurityContext context = repo.loadContext(holder); @@ -57,7 +57,29 @@ public class HttpSessionSecurityContextRepositoryTests { assertEquals(testToken, context.getAuthentication()); // Won't actually be saved as it hasn't changed, but go through the use case anyway repo.saveContext(context, holder.getRequest(), holder.getResponse()); - assertEquals(context, request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)); + assertEquals(context, request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)); + } + + // SEC-1528 + @Test + public void saveContextCallsSetAttributeIfContextIsModifiedDirectlyDuringRequest() throws Exception { + HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository(); + MockHttpServletRequest request = new MockHttpServletRequest(); + // Set up an existing authenticated context, mocking that it is in the session already + SecurityContext ctx = SecurityContextHolder.getContext(); + ctx.setAuthentication(testToken); + HttpSession session = mock(HttpSession.class); + when(session.getAttribute(SPRING_SECURITY_CONTEXT_KEY)).thenReturn(ctx); + request.setSession(session); + HttpRequestResponseHolder holder = new HttpRequestResponseHolder(request, new MockHttpServletResponse()); + assertSame(ctx, repo.loadContext(holder)); + + // Modify context contents. Same user, different role + SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("someone", "passwd", "ROLE_B")); + repo.saveContext(ctx, holder.getRequest(), holder.getResponse()); + + // Must be called even though the value in the local VM is already the same + verify(session).setAttribute(SPRING_SECURITY_CONTEXT_KEY, ctx); } @Test @@ -65,7 +87,7 @@ public class HttpSessionSecurityContextRepositoryTests { HttpSessionSecurityContextRepository repo = new HttpSessionSecurityContextRepository(); MockHttpServletRequest request = new MockHttpServletRequest(); SecurityContextHolder.getContext().setAuthentication(testToken); - request.getSession().setAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY, "NotASecurityContextInstance"); + request.getSession().setAttribute(SPRING_SECURITY_CONTEXT_KEY, "NotASecurityContextInstance"); MockHttpServletResponse response = new MockHttpServletResponse(); HttpRequestResponseHolder holder = new HttpRequestResponseHolder(request, response); SecurityContext context = repo.loadContext(holder); @@ -85,7 +107,7 @@ public class HttpSessionSecurityContextRepositoryTests { context.setAuthentication(testToken); repo.saveContext(context, holder.getRequest(), holder.getResponse()); assertNotNull(request.getSession(false)); - assertEquals(context, request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)); + assertEquals(context, request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)); } @Test @@ -97,11 +119,11 @@ public class HttpSessionSecurityContextRepositoryTests { SecurityContextHolder.setContext(repo.loadContext(holder)); SecurityContextHolder.getContext().setAuthentication(testToken); holder.getResponse().sendRedirect("/doesntmatter"); - assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)); + assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)); assertTrue(((SaveContextOnUpdateOrErrorResponseWrapper)holder.getResponse()).isContextSaved()); repo.saveContext(SecurityContextHolder.getContext(), holder.getRequest(), holder.getResponse()); // Check it's still the same - assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)); + assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)); } @Test @@ -113,11 +135,11 @@ public class HttpSessionSecurityContextRepositoryTests { SecurityContextHolder.setContext(repo.loadContext(holder)); SecurityContextHolder.getContext().setAuthentication(testToken); holder.getResponse().sendError(404); - assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)); + assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)); assertTrue(((SaveContextOnUpdateOrErrorResponseWrapper)holder.getResponse()).isContextSaved()); repo.saveContext(SecurityContextHolder.getContext(), holder.getRequest(), holder.getResponse()); // Check it's still the same - assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)); + assertEquals(SecurityContextHolder.getContext(), request.getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)); } @Test @@ -190,23 +212,4 @@ public class HttpSessionSecurityContextRepositoryTests { assertEquals(url, holder.getResponse().encodeUrl(url)); assertEquals(url, holder.getResponse().encodeURL(url)); } - - static class MockContext implements Cloneable, SecurityContext { - Authentication a; - - public Authentication getAuthentication() { - return a; - } - - public void setAuthentication(Authentication authentication) { - a = authentication; - } - - public Object clone() { - MockContext mc = new MockContext(); - mc.setAuthentication(this.getAuthentication()); - return mc; - } - } - }