From 3f7f87e19f9474f726bad169c0bc98e183ee6ce4 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 27 Jan 2011 09:14:42 -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 | 61 ++++++++++++++++++- 2 files changed, 78 insertions(+), 15 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 efb8325838..11328c3641 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,9 @@ package org.springframework.security.cas.web; import static org.junit.Assert.*; -import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.*; + +import javax.servlet.FilterChain; import org.jasig.cas.client.proxy.ProxyGrantingTicketStorage; import org.junit.Test; @@ -33,6 +35,7 @@ import org.springframework.security.core.AuthenticationException; * Tests {@link CasAuthenticationFilter}. * * @author Ben Alex + * @author Rob Winch */ public class CasAuthenticationFilterTests { //~ Methods ======================================================================================================== @@ -75,4 +78,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); + } +} \ No newline at end of file