More tests. Minor refactoring.

cas/src/test/java/org/springframework/security/cas/userdetails/GrantedAuthorityFromAssertionAttributesUserDetailsServiceTests.java

@@ -0,0 +1,49 @@
+package org.springframework.security.cas.userdetails;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.jasig.cas.client.authentication.AttributePrincipal;
+import org.jasig.cas.client.validation.Assertion;
+import org.junit.Test;
+import org.springframework.security.cas.authentication.CasAssertionAuthenticationToken;
+import org.springframework.security.core.authority.AuthorityUtils;
+import org.springframework.security.core.userdetails.UserDetails;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * @author Luke Taylor
+ */
+public class GrantedAuthorityFromAssertionAttributesUserDetailsServiceTests {
+
+    @Test
+    public void correctlyExtractsNamedAttributesFromAssertionAndConvertsThemToAuthorities() {
+        GrantedAuthorityFromAssertionAttributesUserDetailsService uds =
+                new GrantedAuthorityFromAssertionAttributesUserDetailsService(new String[] {"a", "b", "c", "d"});
+        uds.setConvertToUpperCase(false);
+        Assertion assertion = mock(Assertion.class);
+        AttributePrincipal principal = mock(AttributePrincipal.class);
+        Map<String, Object> attributes = new HashMap<String, Object>();
+        attributes.put("a", Arrays.asList("role_a1", "role_a2"));
+        attributes.put("b", "role_b");
+        attributes.put("c", "role_c");
+        attributes.put("d", null);
+        attributes.put("someother", "unused");
+        when(assertion.getPrincipal()).thenReturn(principal);
+        when(principal.getAttributes()).thenReturn(attributes);
+        when(principal.getName()).thenReturn("somebody");
+        CasAssertionAuthenticationToken token = new CasAssertionAuthenticationToken(assertion, "ticket");
+        UserDetails user = uds.loadUserDetails(token);
+        Set<String> roles = AuthorityUtils.authorityListToSet(user.getAuthorities());
+        assertTrue(roles.size() == 4);
+        assertTrue(roles.contains("role_a1"));
+        assertTrue(roles.contains("role_a2"));
+        assertTrue(roles.contains("role_b"));
+        assertTrue(roles.contains("role_c"));
+    }
+}

cas/src/test/java/org/springframework/security/cas/web/CasAuthenticationFilterTests.java

@@ -16,12 +16,15 @@
 package org.springframework.security.cas.web;
 
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.mock;
 
+import org.jasig.cas.client.proxy.ProxyGrantingTicketStorage;
 import org.junit.Test;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.security.authentication.AuthenticationManager;
 import org.springframework.security.authentication.BadCredentialsException;
+import org.springframework.security.cas.ServiceProperties;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.AuthenticationException;
 
@@ -35,14 +38,17 @@ public class CasAuthenticationFilterTests {
     //~ Methods ========================================================================================================
 
     @Test
-    public void testGetters() {
+    public void testGettersSetters() {
         CasAuthenticationFilter filter = new CasAuthenticationFilter();
         assertEquals("/j_spring_cas_security_check", filter.getFilterProcessesUrl());
+        filter.setProxyGrantingTicketStorage(mock(ProxyGrantingTicketStorage.class));
+        filter.setProxyReceptorUrl("/someurl");
+        filter.setServiceProperties(new ServiceProperties());
     }
 
     @Test
     public void testNormalOperation() throws Exception {
-        MockHttpServletRequest request = new MockHttpServletRequest();
+        MockHttpServletRequest request = new MockHttpServletRequest("GET", "/j_spring_cas_security_check");
         request.addParameter("ticket", "ST-0-ER94xMJmn6pha35CQRoZ");
 
         CasAuthenticationFilter filter = new CasAuthenticationFilter();
@@ -52,6 +58,8 @@ public class CasAuthenticationFilterTests {
             }
         });
 
+        assertTrue(filter.requiresAuthentication(request, new MockHttpServletResponse()));
+
         Authentication result = filter.attemptAuthentication(request, new MockHttpServletResponse());
         assertTrue(result != null);
     }

cas/src/test/java/org/springframework/security/cas/web/ServicePropertiesTests.java

@@ -15,40 +15,43 @@
 
 package org.springframework.security.cas.web;
 
-import org.springframework.security.cas.ServiceProperties;
-
-import junit.framework.TestCase;
+import static junit.framework.Assert.*;
 
+import org.junit.Test;
+import org.springframework.security.cas.SamlServiceProperties;
+import org.springframework.security.cas.ServiceProperties;
 
 /**
  * Tests {@link ServiceProperties}.
  *
  * @author Ben Alex
  */
-public class ServicePropertiesTests extends TestCase {
+public class ServicePropertiesTests {
     //~ Methods ========================================================================================================
 
-    public void testDetectsMissingLoginFormUrl() throws Exception {
+    @Test(expected=IllegalArgumentException.class)
+    public void detectsMissingService() throws Exception {
         ServiceProperties sp = new ServiceProperties();
-
-        try {
-            sp.afterPropertiesSet();
-            fail("Should have thrown IllegalArgumentException");
-        } catch (IllegalArgumentException expected) {
-            assertEquals("service must be specified.", expected.getMessage());
-        }
+        sp.afterPropertiesSet();
     }
 
+    @Test
     public void testGettersSetters() throws Exception {
-        ServiceProperties sp = new ServiceProperties();
-        sp.setSendRenew(false);
-        assertFalse(sp.isSendRenew());
-        sp.setSendRenew(true);
-        assertTrue(sp.isSendRenew());
-
-        sp.setService("https://mycompany.com/service");
-        assertEquals("https://mycompany.com/service", sp.getService());
+        ServiceProperties[] sps = {new ServiceProperties(), new SamlServiceProperties()};
+        for(ServiceProperties sp : sps) {
+            sp.setSendRenew(false);
+            assertFalse(sp.isSendRenew());
+            sp.setSendRenew(true);
+            assertTrue(sp.isSendRenew());
+            sp.setArtifactParameter("notticket");
+            assertEquals("notticket", sp.getArtifactParameter());
+            sp.setServiceParameter("notservice");
+            assertEquals("notservice", sp.getServiceParameter());
+
+            sp.setService("https://mycompany.com/service");
+            assertEquals("https://mycompany.com/service", sp.getService());
 
-        sp.afterPropertiesSet();
+            sp.afterPropertiesSet();
+        }
     }
 }

gradle/emma.gradle

@@ -8,8 +8,8 @@ dependencies{
     emma "emma:emma_ant:2.0.5312"
 }
 
-def emmaMetaDataFile = "${rootProject.buildDir}/emma/coverage.em"
-def emmaCoverageFile = "${rootProject.buildDir}/emma/coverage.ec"
+def emmaMetaDataFile = "${rootProject.buildDir}/emma/emma.em"
+def emmaCoverageFile = "${rootProject.buildDir}/emma/emma.ec"
 
 task emmaInstrument {
     dependsOn classes
@@ -18,7 +18,7 @@ task emmaInstrument {
         ant.path(id: "emmarun.classpath") {
             pathelement(location: sourceSets.main.classesDir.absolutePath)
         }
-        ant.emma(verbosity: "info") {
+        ant.emma(verbosity: "quiet") { // use "info, verbose, trace1, trace2, trace3 for more info"
             instr(merge: "true", destdir: "$buildDir/emma/classes", instrpathref: "emmarun.classpath", metadatafile: "$emmaMetaDataFile") {
                 instrpath {
                     fileset(dir: sourceSets.main.classesDir.absolutePath, includes: "*.class")
@@ -35,6 +35,7 @@ afterEvaluate {
             task.dependsOn emmaInstrument
             task.configure() {
                 jvmArgs '-Dsec.log.level=DEBUG', "-Demma.coverage.out.file=$emmaCoverageFile"
+                jvmArgs '-Demma.verbosity.level=quiet'
             }
             task.doFirst {
                 classpath = files("$buildDir/emma/classes") + configurations.emma + classpath
@@ -53,7 +54,7 @@ if (rootProject.getTasksByName('coverageReport', false).isEmpty()) {
                 }
             }
         }
-        ant.emma(enabled: "true", verbosity: "trace1") { // use "verbose, trace1, trace2, trace3 for more info"
+        ant.emma(enabled: "true", verbosity: "info") {
             report(sourcepathref:"src.path") {
                 fileset(dir: rootProject.buildDir) {
                     include: '*.ec'

web/src/main/java/org/springframework/security/web/access/ExceptionTranslationFilter.java

@@ -94,9 +94,7 @@ public class ExceptionTranslationFilter extends GenericFilterBean {
         try {
             chain.doFilter(request, response);
 
-            if (logger.isDebugEnabled()) {
-                logger.debug("Chain processed normally");
-            }
+            logger.debug("Chain processed normally");
         }
         catch (IOException ex) {
             throw ex;
@@ -112,7 +110,7 @@ public class ExceptionTranslationFilter extends GenericFilterBean {
             }
 
             if (ase != null) {
-                handleException(request, response, chain, ase);
+                handleSpringSecurityException(request, response, chain, ase);
             } else {
                 // Rethrow ServletExceptions and RuntimeExceptions as-is
                 if (ex instanceof ServletException) {
@@ -122,7 +120,8 @@ public class ExceptionTranslationFilter extends GenericFilterBean {
                     throw (RuntimeException) ex;
                 }
 
-                // Wrap other Exceptions. These are not expected to happen
+                // Wrap other Exceptions. This shouldn't actually happen
+                // as we've already covered all the possibilities for doFilter
                 throw new RuntimeException(ex);
             }
         }
@@ -136,30 +135,23 @@ public class ExceptionTranslationFilter extends GenericFilterBean {
         return authenticationTrustResolver;
     }
 
-    private void handleException(HttpServletRequest request, HttpServletResponse response, FilterChain chain,
+    private void handleSpringSecurityException(HttpServletRequest request, HttpServletResponse response, FilterChain chain,
             RuntimeException exception) throws IOException, ServletException {
         if (exception instanceof AuthenticationException) {
-            if (logger.isDebugEnabled()) {
-                logger.debug("Authentication exception occurred; redirecting to authentication entry point", exception);
-            }
+            logger.debug("Authentication exception occurred; redirecting to authentication entry point", exception);
 
             sendStartAuthentication(request, response, chain, (AuthenticationException) exception);
         }
         else if (exception instanceof AccessDeniedException) {
             if (authenticationTrustResolver.isAnonymous(SecurityContextHolder.getContext().getAuthentication())) {
-                if (logger.isDebugEnabled()) {
-                    logger.debug("Access is denied (user is anonymous); redirecting to authentication entry point",
+                logger.debug("Access is denied (user is anonymous); redirecting to authentication entry point",
                             exception);
-                }
 
                 sendStartAuthentication(request, response, chain, new InsufficientAuthenticationException(
                         "Full authentication is required to access this resource"));
             }
             else {
-                if (logger.isDebugEnabled()) {
-                    logger.debug("Access is denied (user is not anonymous); delegating to AccessDeniedHandler",
-                            exception);
-                }
+                logger.debug("Access is denied (user is not anonymous); delegating to AccessDeniedHandler", exception);
 
                 accessDeniedHandler.handle(request, response, (AccessDeniedException) exception);
             }

web/src/test/java/org/springframework/security/web/access/ExceptionTranslationFilterTests.java

@@ -17,15 +17,8 @@ package org.springframework.security.web.access;
 
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.*;
-
-import java.io.IOException;
-
-import javax.servlet.FilterChain;
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
 
 import org.junit.After;
 import org.junit.Before;
@@ -43,10 +36,16 @@ import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.security.web.AuthenticationEntryPoint;
 import org.springframework.security.web.WebAttributes;
 import org.springframework.security.web.savedrequest.HttpSessionRequestCache;
-import org.springframework.security.web.savedrequest.DefaultSavedRequest;
 import org.springframework.security.web.savedrequest.SavedRequest;
 import org.springframework.security.web.util.ThrowableAnalyzer;
 
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpSession;
+import java.io.IOException;
+
 /**
  * Tests {@link ExceptionTranslationFilter}.
  *
@@ -134,7 +133,7 @@ public class ExceptionTranslationFilterTests {
     }
 
     @Test
-    public void testRedirectedToLoginFormAndSessionShowsOriginalTargetWhenAuthenticationException() throws Exception {
+    public void redirectedToLoginFormAndSessionShowsOriginalTargetWhenAuthenticationException() throws Exception {
         // Setup our HTTP request
         MockHttpServletRequest request = new MockHttpServletRequest();
         request.setServletPath("/secure/page.html");
@@ -159,7 +158,7 @@ public class ExceptionTranslationFilterTests {
     }
 
     @Test
-    public void testRedirectedToLoginFormAndSessionShowsOriginalTargetWithExoticPortWhenAuthenticationException()
+    public void redirectedToLoginFormAndSessionShowsOriginalTargetWithExoticPortWhenAuthenticationException()
             throws Exception {
         // Setup our HTTP request
         MockHttpServletRequest request = new MockHttpServletRequest();
@@ -188,7 +187,7 @@ public class ExceptionTranslationFilterTests {
     }
 
     @Test(expected=IllegalArgumentException.class)
-    public void testStartupDetectsMissingAuthenticationEntryPoint() throws Exception {
+    public void startupDetectsMissingAuthenticationEntryPoint() throws Exception {
         ExceptionTranslationFilter filter = new ExceptionTranslationFilter();
         filter.setThrowableAnalyzer(mock(ThrowableAnalyzer.class));
 
@@ -196,14 +195,15 @@ public class ExceptionTranslationFilterTests {
     }
 
     @Test(expected=IllegalArgumentException.class)
-    public void testStartupDetectsMissingRequestCache() throws Exception {
+    public void startupDetectsMissingRequestCache() throws Exception {
         ExceptionTranslationFilter filter = new ExceptionTranslationFilter();
         filter.setAuthenticationEntryPoint(mockEntryPoint());
 
         filter.setRequestCache(null);
     }
 
-    public void testSuccessfulAccessGrant() throws Exception {
+    @Test
+    public void successfulAccessGrant() throws Exception {
         // Setup our HTTP request
         MockHttpServletRequest request = new MockHttpServletRequest();
         request.setServletPath("/secure/page.html");
@@ -217,36 +217,44 @@ public class ExceptionTranslationFilterTests {
     }
 
     @Test
-    public void testThrowIOException() throws Exception {
+    public void thrownIOExceptionServletExceptionAndRuntimeExceptionsAreRethrown() throws Exception {
         ExceptionTranslationFilter filter = new ExceptionTranslationFilter();
 
         filter.setAuthenticationEntryPoint(mockEntryPoint());
         filter.afterPropertiesSet();
-        FilterChain fc = mock(FilterChain.class);
-        doThrow(new IOException()).when(fc).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));
-        try {
-            filter.doFilter(new MockHttpServletRequest(), new MockHttpServletResponse(), fc);
-            fail("Should have thrown IOException");
-        }
-        catch (IOException e) {
-            assertNull("The IOException thrown should not have been wrapped", e.getCause());
+        Exception[] exceptions = {new IOException(), new ServletException(), new RuntimeException()};
+        for (Exception e : exceptions) {
+            FilterChain fc = mock(FilterChain.class);
+
+            doThrow(e).when(fc).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));
+            try {
+                filter.doFilter(new MockHttpServletRequest(), new MockHttpServletResponse(), fc);
+                fail("Should have thrown Exception");
+            }
+            catch (Exception expected) {
+                assertSame("The exception thrown should not have been wrapped", e, expected);
+            }
         }
     }
 
     @Test
-    public void testThrowServletException() throws Exception {
+    public void unexpectedExceptionsAreWrappedAsRuntime() throws Exception {
         ExceptionTranslationFilter filter = new ExceptionTranslationFilter();
 
         filter.setAuthenticationEntryPoint(mockEntryPoint());
-        filter.afterPropertiesSet();
+
+        Exception e = new Exception("");
+
         FilterChain fc = mock(FilterChain.class);
-        doThrow(new ServletException()).when(fc).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));
+
+        doThrow(e).when(fc).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));
+
         try {
             filter.doFilter(new MockHttpServletRequest(), new MockHttpServletResponse(), fc);
-            fail("Should have thrown ServletException");
+            fail("Should have thrown Exception");
         }
-        catch (ServletException e) {
-            assertNull("The ServletException thrown should not have been wrapped", e.getCause());
+        catch (RuntimeException expected) {
+            assertSame(e, expected.getCause());
         }
     }