SEC-857: Make request wrapper getParameterValues() consistent with getParameterMap() etc.

core/src/main/java/org/springframework/security/wrapper/SavedRequestAwareWrapper.java

@@ -28,6 +28,7 @@ import org.apache.commons.logging.LogFactory;
 import java.text.SimpleDateFormat;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -35,6 +36,7 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.TimeZone;
+import java.util.Map.Entry;
 
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
@@ -280,9 +282,27 @@ public class SavedRequestAwareWrapper extends SecurityContextHolderAwareRequestW
     	
     	// We have a saved request so merge the values, with the wrapped request taking precedence (see getParameter())
     	Map newParameters = new HashMap(savedRequest.getParameterMap().size() + parameters.size());
-    	newParameters.putAll(savedRequest.getParameterMap());
     	newParameters.putAll(parameters);
     	
+    	Iterator savedParams = savedRequest.getParameterMap().entrySet().iterator();
+    	
+    	while (savedParams.hasNext()) {
+    		Map.Entry entry = (Entry) savedParams.next();
+    		String name = (String) entry.getKey();
+    		String[] savedParamValues = (String[]) entry.getValue();
+    		
+    		if (newParameters.containsKey(name)) {
+    			// merge values
+    			String[] existingValues = (String[]) newParameters.get(name);
+    			String[] mergedValues = new String[savedParamValues.length + existingValues.length];
+    			System.arraycopy(existingValues, 0, mergedValues, 0, existingValues.length);
+    			System.arraycopy(savedParamValues, 0, mergedValues, existingValues.length, savedParamValues.length);
+    			newParameters.put(name, mergedValues);
+    		} else {
+    			newParameters.put(name, savedParamValues);
+    		}
+    	}
+
     	return newParameters;
     }
 
@@ -291,10 +311,23 @@ public class SavedRequestAwareWrapper extends SecurityContextHolderAwareRequestW
     }
 
     public String[] getParameterValues(String name) {
-        if (savedRequest == null) {
-            return super.getParameterValues(name);
-        } else {
-            return savedRequest.getParameterValues(name);
-        }
+    	String[] savedRequestParams = savedRequest == null ? null : savedRequest.getParameterValues(name);
+    	String[] wrappedRequestParams = super.getParameterValues(name);
+
+    	if (savedRequestParams == null && wrappedRequestParams == null) {
+    		return null;
+    	}
+
+    	List combinedParams = new ArrayList();
+
+    	if (wrappedRequestParams != null) {
+    		combinedParams.addAll(Arrays.asList(wrappedRequestParams));
+    	}
+
+    	if (savedRequestParams != null) {
+    		combinedParams.addAll(Arrays.asList(savedRequestParams));
+    	}
+
+    	return (String[]) combinedParams.toArray(new String[combinedParams.size()]);
     }
 }

core/src/test/java/org/springframework/security/wrapper/SavedRequestAwareWrapperTests.java

@@ -38,4 +38,29 @@ public class SavedRequestAwareWrapperTests {
 
 		assertEquals("foo", wrapper.getHeader("Authorization"));
 	}
+
+	@Test
+	public void getParameterValuesReturnsNullIfParameterIsntSet() {
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		SavedRequestAwareWrapper wrapper = new SavedRequestAwareWrapper(request, new PortResolverImpl(), "ROLE_");
+		assertNull(wrapper.getParameterValues("action"));
+		assertNull(wrapper.getParameterMap().get("action"));
+	}
+	
+	@Test
+	public void getParameterValuesReturnsCombinedValues() {
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		request.setParameter("action", "foo");
+		SavedRequest savedRequest = new SavedRequest(request, new PortResolverImpl());
+		MockHttpServletRequest request2 = new MockHttpServletRequest();
+		request2.getSession().setAttribute(AbstractProcessingFilter.SPRING_SECURITY_SAVED_REQUEST_KEY, savedRequest);
+		SavedRequestAwareWrapper wrapper = new SavedRequestAwareWrapper(request2, new PortResolverImpl(), "ROLE_");
+		assertArrayEquals(new Object[] {"foo"}, wrapper.getParameterValues("action"));
+		request2.setParameter("action", "bar");
+		assertArrayEquals(new Object[] {"bar","foo"}, wrapper.getParameterValues("action"));
+		// Check map is consistent
+		String[] valuesFromMap = (String[]) wrapper.getParameterMap().get("action"); 
+		assertEquals(2, valuesFromMap.length);
+		assertEquals("bar", valuesFromMap[0]);
+	}
 }