fix: correct handling of HttpServletRequest#getHeaders and tests

Properly handles cases where container may return blank String values in the returned list.
Tests were previously adding List objects as headers - need to add just String values.

uw-spring-security-core/src/main/java/edu/wisc/uwss/UWUserDetailsImpl.java

@@ -62,7 +62,7 @@ public class UWUserDetailsImpl extends User implements UWUserDetails, HasModifia
    * @param fullName
    * @param emailAddress
    * @param uddsMembership
-   * @param grantedAuthorities
+   * @param authorities
    */
   public UWUserDetailsImpl(String pvi, String username, String password,
                            String fullName, String emailAddress,

uw-spring-security-core/src/main/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapper.java

@@ -9,6 +9,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Enumeration;
+import java.util.List;
 
 import javax.servlet.http.HttpServletRequest;
 
@@ -60,7 +61,7 @@ public interface PreauthenticatedUserDetailsAttributeMapper {
     private String identityProviderHeader = "Shib-Identity-Provider";
     private String customLogoutPrefix = "/Shibboleth.sso/Logout?return=";
     private String customLogoutSuffix = "/logout/";
-    private String manifestHeader = "ismemberof";
+    private String manifestHeader = "isMemberOf";
     
     private static final Logger logger = LoggerFactory.getLogger(Default.class);
     /**
@@ -82,17 +83,9 @@ public interface PreauthenticatedUserDetailsAttributeMapper {
       String pvi = request.getHeader(pviHeader);
       String cn = request.getHeader(fullNameHeader);
       String emplid = request.getHeader(isisEmplidHeader);
-      Collection<String> uddsMembership = new ArrayList<>();
-      Enumeration<String> uddsHeaders = request.getHeaders(uddsHeader);
-      if(uddsHeaders != null) {
-        uddsMembership = Collections.list(uddsHeaders);
-      }
+      Collection<String> uddsMembership = safeGetHeaders(request, uddsHeader);
       String email = request.getHeader(emailAddressHeader);
-      Collection<String> manifestGroups = new ArrayList<>();
-      Enumeration<String> manifestHeaders = request.getHeaders(manifestHeader);
-      if(manifestHeaders != null) {
-        manifestGroups = Collections.list(manifestHeaders);
-      }
+      Collection<String> manifestGroups = safeGetHeaders(request, manifestHeader);
       UWUserDetailsImpl result = UWUserDetailsImpl.newInstance(pvi, uid, "", cn, email, uddsMembership, manifestGroups);
       result.setSource("edu.wisc.uwss.preauth");
       result.setEppn(eppn);
@@ -105,7 +98,42 @@ public interface PreauthenticatedUserDetailsAttributeMapper {
       logger.debug("mapUser constructed {} from headers in request", result);
       return result;
     }
-    
+
+    /**
+     * {@link HttpServletRequest#getHeaders(String)} has 3 possible return values:
+     *
+     * <ul>
+     *   <li>A non-empty {@link Enumeration} of {@link String} values for the header,</li>
+     *   <li>An "empty" {@link Enumeration} of {@link String} values for the header,</li>
+     *   <li>null, if the container does not allow access to the header(s).</li>
+     * </ul>
+     *
+     * Further complicating things is that some containers may return a non-empty {@link Enumeration}
+     * of {@link String} values, but those {@link String} values may be blank. For this particular case,
+     * {@link Collections#list(Enumeration)} is unsafe, because the returned list may include blank values.
+     *
+     * This method provides a safe way to get the values for the specified header as a non-null
+     * (but still potentially empty) {@link List}.
+     *
+     * @see HttpServletRequest#getHeaders(String)
+     * @see StringUtils#isNotBlank(CharSequence)
+     * @param request the HTTP request
+     * @param headerName the name of the multi-valued header
+     * @return a never null, but potentially empty, {@link List} of not blank values for the requested header
+     */
+    protected List<String> safeGetHeaders(HttpServletRequest request, String headerName) {
+      Enumeration<String> enumeration = request.getHeaders(headerName);
+      if(enumeration == null) {
+        return Collections.emptyList();
+      }
+      List<String> result = new ArrayList<>();
+      for(String next : Collections.list(enumeration)){
+        if(StringUtils.isNotBlank(next)) {
+          result.add(next);
+        }
+      }
+      return result;
+    }
     /**
      * 
      * @param identityProviderHeaderValue the value of the identityProviderHeader

uw-spring-security-core/src/test/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapperTest.java

@@ -10,6 +10,7 @@ import static org.junit.Assert.assertTrue;
 
 import java.net.URL;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
@@ -18,6 +19,8 @@ import javax.servlet.http.HttpServletRequest;
 import org.junit.Test;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.core.authority.AuthorityUtils;
+import org.springframework.security.core.authority.SimpleGrantedAuthority;
 
 import edu.wisc.uwss.UWUserDetails;
 import edu.wisc.uwss.preauth.PreauthenticatedUserDetailsAttributeMapper.Default;
@@ -30,9 +33,10 @@ import edu.wisc.uwss.preauth.PreauthenticatedUserDetailsAttributeMapper.Default;
 public class PreauthenticatedUserDetailsAttributeMapperTest {
 
   private PreauthenticatedUserDetailsAttributeMapper.Default filter = new PreauthenticatedUserDetailsAttributeMapper.Default();
-  
+
   /**
    * Verify expected behaviour for {@link PreauthenticatedUserDetailsAttributeMapper#mapUser(HttpServletRequest)}
+   * for a simple case with 1 value for each expected header.
    */
   @Test
   public void mapUser_success() {
@@ -43,18 +47,16 @@ public class PreauthenticatedUserDetailsAttributeMapperTest {
     String name = "some body";
     String email = "some.body@wisc.edu";
     String emplid = "0000123456";
-    List<String> uddsMembership = Collections.singletonList("udds1234");
-    List<String> manifestGroups = Collections.singletonList("uw:domain:something");
     
     request.addHeader("eppn", eppn);
     request.addHeader("wiscedupvi", pvi);
     request.addHeader("uid", uid);
     request.addHeader("cn", name);
     request.addHeader("mail", email);
-    request.addHeader("wisceduudds", uddsMembership);
+    request.addHeader("wisceduudds", "udds1234");
     request.addHeader("wisceduisisemplid", emplid);
     request.addHeader("Shib-Identity-Provider", "https://logintest.wisc.edu/idp/shibboleth");
-    request.addHeader("ismemberof",manifestGroups);
+    request.addHeader("isMemberOf", "uw:domain:something");
     
     UWUserDetails result = filter.mapUser(request);
     
@@ -64,37 +66,93 @@ public class PreauthenticatedUserDetailsAttributeMapperTest {
     assertEquals(pvi, result.getPvi());
     assertEquals(name, result.getFullName());
     assertEquals(email, result.getEmailAddress());
-    assertEquals(uddsMembership, result.getUddsMembership()); 
+    assertEquals(Collections.singletonList("udds1234"), result.getUddsMembership());
     assertEquals(emplid, result.getIsisEmplid());
     assertEquals("/Shibboleth.sso/Logout?return=https://logintest.wisc.edu/logout/", result.getCustomLogoutUrl());
     assertEquals(1,result.getAuthorities().size());
-    assertEquals(manifestGroups.toString(),result.getAuthorities().toString());
+    assertTrue(result.getAuthorities().contains(new SimpleGrantedAuthority("uw:domain:something")));
   }
 
   /**
-   * Verify that a user can have multiple Manifest groups in their {@link GrantedAuthority}s
+   * Verify expected behaviour for {@link PreauthenticatedUserDetailsAttributeMapper#mapUser(HttpServletRequest)}
+   * when the isMemberOf header is empty.
+   */
+  @Test
+  public void mapUser_empty_isMemberOf() {
+    MockHttpServletRequest request = new MockHttpServletRequest();
+    String uid = "somebody";
+    String eppn = "somebody@wisc.edu";
+    String pvi = "1234567";
+    String name = "some body";
+    String email = "some.body@wisc.edu";
+    String emplid = "0000123456";
+    List<String> uddsMembership = Collections.singletonList("udds1234");
+
+    request.addHeader("eppn", eppn);
+    request.addHeader("wiscedupvi", pvi);
+    request.addHeader("uid", uid);
+    request.addHeader("cn", name);
+    request.addHeader("mail", email);
+    request.addHeader("wisceduudds", uddsMembership);
+    request.addHeader("wisceduisisemplid", emplid);
+    request.addHeader("Shib-Identity-Provider", "https://logintest.wisc.edu/idp/shibboleth");
+
+    UWUserDetails result = filter.mapUser(request);
+
+    assertNotNull(result);
+    assertEquals(uid, result.getUsername());
+    assertEquals(eppn, result.getEppn());
+    assertEquals(pvi, result.getPvi());
+    assertEquals(name, result.getFullName());
+    assertEquals(email, result.getEmailAddress());
+    assertEquals(uddsMembership, result.getUddsMembership());
+    assertEquals(emplid, result.getIsisEmplid());
+    assertEquals("/Shibboleth.sso/Logout?return=https://logintest.wisc.edu/logout/", result.getCustomLogoutUrl());
+    assertEquals(0, result.getAuthorities().size());
+  }
+  /**
+   * Verify expected behaviour for {@link PreauthenticatedUserDetailsAttributeMapper#mapUser(HttpServletRequest)}
+   * when the isMemberOf header has multiple values.
    */
   @Test
   public void mapUser_multipleManifestGroups() {
     MockHttpServletRequest request = new MockHttpServletRequest();
 
     String uid = "somebody";
-    ArrayList<String> manifestGroups = new ArrayList<>();
-    manifestGroups.add("uw:domain:onegroup");
-    manifestGroups.add("uw:domain:anothergroup");
 
     request.addHeader("uid", uid);
-    request.addHeader("ismemberof",manifestGroups);
+    request.addHeader("isMemberOf", "uw:domain:onegroup");
+    request.addHeader("isMemberOf", "uw:domain:anothergroup");
 
     UWUserDetails result = filter.mapUser(request);
 
     assertEquals(uid, result.getUsername());
     assertEquals(2,result.getAuthorities().size());
 
-    for (GrantedAuthority authority : result.getAuthorities()) {
-      assertTrue(manifestGroups.contains(authority.getAuthority()));
-    }
+    assertTrue(result.getAuthorities().contains(new SimpleGrantedAuthority("uw:domain:onegroup")));
+    assertTrue(result.getAuthorities().contains(new SimpleGrantedAuthority("uw:domain:anothergroup")));
+  }
+  /**
+   * Verify expected behaviour for {@link PreauthenticatedUserDetailsAttributeMapper#mapUser(HttpServletRequest)}
+   * when the isMemberOf header has multiple values.
+   */
+  @Test
+  public void mapUser_multipleudds() {
+    MockHttpServletRequest request = new MockHttpServletRequest();
+
+    String uid = "somebody";
+
+    request.addHeader("uid", uid);
+    request.addHeader("wisceduudds", "A061234");
+    request.addHeader("wisceduudds", "B062345");
+
+    UWUserDetails result = filter.mapUser(request);
+
+    assertEquals(uid, result.getUsername());
+    assertEquals(2,result.getUddsMembership().size());
 
+    assertTrue(result.getUddsMembership().contains("A061234"));
+    assertTrue(result.getUddsMembership().contains("B062345"));
   }
   /**
    * Verify behavior of {@link Default#toCustomLogoutUrl(String)} for