From 51267bd052f5978a6f222f76a19ddc87ed10a5b2 Mon Sep 17 00:00:00 2001
From: Nicholas Blair <nicholas.blair@wisc.edu>
Date: Tue, 13 Sep 2016 11:26:48 -0500
Subject: [PATCH] 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.
---
.../java/edu/wisc/uwss/UWUserDetailsImpl.java | 2 +-
...thenticatedUserDetailsAttributeMapper.java | 52 ++++++++---
...ticatedUserDetailsAttributeMapperTest.java | 88 +++++++++++++++----
3 files changed, 114 insertions(+), 28 deletions(-)
diff --git a/uw-spring-security-core/src/main/java/edu/wisc/uwss/UWUserDetailsImpl.java b/uw-spring-security-core/src/main/java/edu/wisc/uwss/UWUserDetailsImpl.java
index 2e73b58..00ed353 100644
--- a/uw-spring-security-core/src/main/java/edu/wisc/uwss/UWUserDetailsImpl.java
+++ b/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,
diff --git a/uw-spring-security-core/src/main/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapper.java b/uw-spring-security-core/src/main/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapper.java
index 20bf725..46e5473 100644
--- a/uw-spring-security-core/src/main/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapper.java
+++ b/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
diff --git a/uw-spring-security-core/src/test/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapperTest.java b/uw-spring-security-core/src/test/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapperTest.java
index d7e86ee..9f6ee0d 100644
--- a/uw-spring-security-core/src/test/java/edu/wisc/uwss/preauth/PreauthenticatedUserDetailsAttributeMapperTest.java
+++ b/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
--
GitLab