From ece824fca2298d90cb9f1c7f64247ddea8d07fa7 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 27 Jan 2011 10:49:24 -0600 Subject: [PATCH] SEC-1592: Updated CasAuthenticationFilter so that it does not continue FilterChain when handling proxy requests. The fix moves CommonUtils.readAndRespondToProxyReceptorRequest into CasAuthenticationFilter.attemptAuthentication. This makes sense since The CAS server is authenticating that the proxy url is valid (i.e. it exists and the SSL handshake succeeds). It also allows the FilterChain to not be processed by returning a null Authentication. --- .../cas/web/CasAuthenticationFilter.java | 32 ++++++---- .../cas/web/CasAuthenticationFilterTests.java | 59 +++++++++++++++++++ 2 files changed, 78 insertions(+), 13 deletions(-) diff --git a/cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java b/cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java index 314a1fdfd2..12157b8ce0 100644 --- a/cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java +++ b/cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java @@ -54,6 +54,7 @@ import org.springframework.security.web.authentication.AbstractAuthenticationPro * By default this filter processes the URL /j_spring_cas_security_check. * * @author Ben Alex + * @author Rob Winch */ public class CasAuthenticationFilter extends AbstractAuthenticationProcessingFilter { //~ Static fields/initializers ===================================================================================== @@ -89,7 +90,13 @@ public class CasAuthenticationFilter extends AbstractAuthenticationProcessingFil //~ Methods ======================================================================================================== public Authentication attemptAuthentication(final HttpServletRequest request, final HttpServletResponse response) - throws AuthenticationException { + throws AuthenticationException, IOException { + // if the request is a proxy request process it and return null to indicate the request has been processed + if(isProxyRequest(request)) { + CommonUtils.readAndRespondToProxyReceptorRequest(request, response, this.proxyGrantingTicketStorage); + return null; + } + final String username = CAS_STATEFUL_IDENTIFIER; String password = request.getParameter(this.artifactParameter); @@ -108,18 +115,7 @@ public class CasAuthenticationFilter extends AbstractAuthenticationProcessingFil * Overridden to provide proxying capabilities. */ protected boolean requiresAuthentication(final HttpServletRequest request, final HttpServletResponse response) { - final String requestUri = request.getRequestURI(); - - if (CommonUtils.isEmpty(this.proxyReceptorUrl) || !requestUri.endsWith(this.proxyReceptorUrl) || this.proxyGrantingTicketStorage == null) { - return super.requiresAuthentication(request, response); - } - - try { - CommonUtils.readAndRespondToProxyReceptorRequest(request, response, this.proxyGrantingTicketStorage); - return false; - } catch (final IOException e) { - return super.requiresAuthentication(request, response); - } + return isProxyRequest(request) || super.requiresAuthentication(request, response); } public final void setProxyReceptorUrl(final String proxyReceptorUrl) { @@ -134,4 +130,14 @@ public class CasAuthenticationFilter extends AbstractAuthenticationProcessingFil public final void setServiceProperties(final ServiceProperties serviceProperties) { this.artifactParameter = serviceProperties.getArtifactParameter(); } + + /** + * Indicates if the request is eligible to be processed as a proxy request. + * @param request + * @return + */ + private boolean isProxyRequest(final HttpServletRequest request) { + final String requestUri = request.getRequestURI(); + return this.proxyGrantingTicketStorage != null && !CommonUtils.isEmpty(this.proxyReceptorUrl) && requestUri.endsWith(this.proxyReceptorUrl); + } } diff --git a/cas/src/test/java/org/springframework/security/cas/web/CasAuthenticationFilterTests.java b/cas/src/test/java/org/springframework/security/cas/web/CasAuthenticationFilterTests.java index e3221b722e..79f689e0b4 100644 --- a/cas/src/test/java/org/springframework/security/cas/web/CasAuthenticationFilterTests.java +++ b/cas/src/test/java/org/springframework/security/cas/web/CasAuthenticationFilterTests.java @@ -16,7 +16,11 @@ package org.springframework.security.cas.web; import static org.junit.Assert.*; +import static org.mockito.Mockito.*; +import javax.servlet.FilterChain; + +import org.jasig.cas.client.proxy.ProxyGrantingTicketStorage; import org.junit.Test; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -30,6 +34,7 @@ import org.springframework.security.core.AuthenticationException; * Tests {@link CasAuthenticationFilter}. * * @author Ben Alex + * @author Rob Winch */ public class CasAuthenticationFilterTests { //~ Methods ======================================================================================================== @@ -67,4 +72,58 @@ public class CasAuthenticationFilterTests { filter.attemptAuthentication(new MockHttpServletRequest(), new MockHttpServletResponse()); } + + @Test + public void testRequiresAuthenticationFilterProcessUrl() { + CasAuthenticationFilter filter = new CasAuthenticationFilter(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + request.setRequestURI(filter.getFilterProcessesUrl()); + assertTrue(filter.requiresAuthentication(request, response)); + } + + @Test + public void testRequiresAuthenticationProxyRequest() { + CasAuthenticationFilter filter = new CasAuthenticationFilter(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + request.setRequestURI("/pgtCallback"); + assertFalse(filter.requiresAuthentication(request, response)); + filter.setProxyReceptorUrl(request.getRequestURI()); + assertFalse(filter.requiresAuthentication(request, response)); + filter.setProxyGrantingTicketStorage(mock(ProxyGrantingTicketStorage.class)); + assertTrue(filter.requiresAuthentication(request, response)); + request.setRequestURI("/other"); + assertFalse(filter.requiresAuthentication(request, response)); + } + + @Test + public void testAuthenticateProxyUrl() throws Exception { + CasAuthenticationFilter filter = new CasAuthenticationFilter(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + request.setRequestURI("/pgtCallback"); + filter.setProxyGrantingTicketStorage(mock(ProxyGrantingTicketStorage.class)); + filter.setProxyReceptorUrl(request.getRequestURI()); + assertNull(filter.attemptAuthentication(request, response)); + } + + // SEC-1592 + @Test + public void testChainNotInvokedForProxy() throws Exception { + CasAuthenticationFilter filter = new CasAuthenticationFilter(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain chain = mock(FilterChain.class); + + request.setRequestURI("/pgtCallback"); + filter.setProxyGrantingTicketStorage(mock(ProxyGrantingTicketStorage.class)); + filter.setProxyReceptorUrl(request.getRequestURI()); + + filter.doFilter(request,response,chain); + verifyZeroInteractions(chain); + } }