More tinkering with encoded query in redirect URI

We needed to add another special treatment for query params to only encode the ones coming from the authorization endpoint (unless they were registered as a redirect with special characters). The output is slightly different but the URLs are more correct than before. Fixes spring-atticgh-404
Golfire · Feb 23, 2015 · 05192c2 · 05192c2
1 parent b6e9012
commit 05192c2
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 27 deletions.
diff --git a/...ain/java/org/springframework/security/oauth2/provider/endpoint/AuthorizationEndpoint.java b/...ain/java/org/springframework/security/oauth2/provider/endpoint/AuthorizationEndpoint.java
@@ -65,6 +65,7 @@
 import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.View;
 import org.springframework.web.servlet.view.RedirectView;
+import org.springframework.web.util.UriComponents;
 import org.springframework.web.util.UriComponentsBuilder;
 
 /**
@@ -406,50 +407,47 @@ private String append(String base, Map<String, ?> query, Map<String, String> key
 		catch (Exception e) {
 			// ... but allow client registrations to contain hard-coded non-encoded values
 			redirectUri = builder.build().toUri();
+			builder = UriComponentsBuilder.fromUri(redirectUri);
 		}
 		template.scheme(redirectUri.getScheme()).port(redirectUri.getPort()).host(redirectUri.getHost())
 				.userInfo(redirectUri.getUserInfo()).path(redirectUri.getPath());
 
-		StringBuilder values = new StringBuilder();
-		for (String key : query.keySet()) {
-			if (values.length() > 0) {
-				values.append("&");
-			}
-			String name = key;
-			if (keys != null && keys.containsKey(key)) {
-				name = keys.get(key);
-			}
-			values.append(name + "={" + key + "}");
-		}
-
 		if (fragment) {
+			StringBuilder values = new StringBuilder();
 			if (redirectUri.getFragment() != null) {
 				String append = redirectUri.getFragment();
+				values.append(append);
+			}
+			for (String key : query.keySet()) {
 				if (values.length() > 0) {
-					append = append + "&";
+					values.append("&");
+				}
+				String name = key;
+				if (keys != null && keys.containsKey(key)) {
+					name = keys.get(key);
 				}
-				values.insert(0, append);
+				values.append(name + "={" + key + "}");
 			}
 			if (values.length() > 0) {
 				template.fragment(values.toString());
 			}
-			template.query(redirectUri.getQuery());
+			UriComponents encoded = template.build().expand(query).encode();
+			builder.fragment(encoded.getFragment());
 		}
 		else {
-			if (redirectUri.getQuery() != null) {
-				String append = redirectUri.getQuery();
-				if (values.length() > 0) {
-					append = append + "&";
+			for (String key : query.keySet()) {
+				String name = key;
+				if (keys != null && keys.containsKey(key)) {
+					name = keys.get(key);
 				}
-				values.insert(0, append);
-			}
-			if (values.length() > 0) {
-				template.query(values.toString());
+				template.queryParam(name, "{" + key + "}");
 			}
 			template.fragment(redirectUri.getFragment());
+			UriComponents encoded = template.build().expand(query).encode();
+			builder.query(encoded.getQuery());
 		}
 
-		return template.build().expand(query).encode().toUriString();
+		return builder.build().toUriString();
 
 	}
 

diff --git a/...ava/org/springframework/security/oauth2/provider/endpoint/AuthorizationEndpointTests.java b/...ava/org/springframework/security/oauth2/provider/endpoint/AuthorizationEndpointTests.java
@@ -48,10 +48,12 @@
 import org.springframework.security.oauth2.provider.approval.InMemoryApprovalStore;
 import org.springframework.security.oauth2.provider.client.BaseClientDetails;
 import org.springframework.security.oauth2.provider.code.AuthorizationCodeServices;
+import org.springframework.util.MultiValueMap;
 import org.springframework.web.bind.support.SimpleSessionStatus;
 import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.View;
 import org.springframework.web.servlet.view.RedirectView;
+import org.springframework.web.util.UriComponentsBuilder;
 
 /**
  * @author Dave Syer
@@ -199,7 +201,11 @@ public void testAuthorizationCodeWithTrickyQueryParams() throws Exception {
 						Collections.singleton("code")));
 		View result = endpoint.approveOrDeny(Collections.singletonMap(OAuth2Utils.USER_OAUTH_APPROVAL, "true"), model,
 				sessionStatus, principal);
-		assertEquals("http://anywhere.com?foo=b%20%3D&bar=f%20$&code=thecode", ((RedirectView) result).getUrl());
+		String url = ((RedirectView) result).getUrl();
+		assertEquals("http://anywhere.com?foo=b%20=&bar=f%20$&code=thecode", url);
+		MultiValueMap<String, String> params = UriComponentsBuilder.fromHttpUrl(url).build().getQueryParams();
+		assertEquals("[b%20=]", params.get("foo").toString());
+		assertEquals("[f%20$]", params.get("bar").toString());
 	}
 
 	@Test
@@ -214,6 +220,18 @@ public void testAuthorizationCodeWithTrickyEncodedQueryParams() throws Exception
 		assertEquals("http://anywhere.com/path?foo=b%20%3D&bar=f%20$&code=thecode", ((RedirectView) result).getUrl());
 	}
 
+	@Test
+	public void testAuthorizationCodeWithMoreTrickyEncodedQueryParams() throws Exception {
+		endpoint.setAuthorizationCodeServices(new StubAuthorizationCodeServices());
+		model.put(
+				"authorizationRequest",
+				getAuthorizationRequest("foo", "http://anywhere?t=a%3Db%26ep%3Dtest%2540test.me", null, null,
+						Collections.singleton("code")));
+		View result = endpoint.approveOrDeny(Collections.singletonMap(OAuth2Utils.USER_OAUTH_APPROVAL, "true"), model,
+				sessionStatus, principal);
+		assertEquals("http://anywhere?t=a%3Db%26ep%3Dtest%2540test.me&code=thecode", ((RedirectView) result).getUrl());
+	}
+
 	@Test
 	public void testAuthorizationCodeError() throws Exception {
 		endpoint.setUserApprovalHandler(new DefaultUserApprovalHandler() {
@@ -363,8 +381,8 @@ public boolean isApproved(AuthorizationRequest authorizationRequest, Authenticat
 				return true;
 			}
 		});
-		AuthorizationRequest authorizationRequest = getAuthorizationRequest("foo", "http://anywhere.com",
-				"mystate", "myscope", Collections.singleton("token"));
+		AuthorizationRequest authorizationRequest = getAuthorizationRequest("foo", "http://anywhere.com", "mystate",
+				"myscope", Collections.singleton("token"));
 		ModelAndView result = endpoint.authorize(model, authorizationRequest.getRequestParameters(), sessionStatus,
 				principal);
 		String url = ((RedirectView) result.getView()).getUrl();