Skip client whitelist when lacking client ID

[#63392710] https://www.pivotaltracker.com/story/show/63392710
cloudfoundry · Feb 16, 2016 · b644b1e · b644b1e
1 parent 9eec4e3
commit b644b1e
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 15 deletions.
diff --git a/...src/main/java/org/cloudfoundry/identity/uaa/authentication/SamlRedirectLogoutHandler.java b/...src/main/java/org/cloudfoundry/identity/uaa/authentication/SamlRedirectLogoutHandler.java
@@ -28,8 +28,8 @@ public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse resp
         String relayState = request.getParameter("RelayState");
         Map<String, String> params = JsonUtils.readValue(relayState, new TypeReference<Map<String, String>>() {});
         if(params != null) {
-            requestWrapper.setParameter("redirect", params.get("redirect"));
-            requestWrapper.setParameter("client_id", params.get("client_id"));
+            requestWrapper.setParameterIfAbsent("redirect", params.get("redirect"));
+            requestWrapper.setParameterIfAbsent("client_id", params.get("client_id"));
         }
 
         wrappedHandler.onLogoutSuccess(requestWrapper, response, authentication);
@@ -43,8 +43,8 @@ public RequestWrapper(HttpServletRequest request) {
             parameterMap = new HashMap<>(request.getParameterMap());
         }
 
-        public void setParameter(String name, String... value) {
-            parameterMap.put(name, value);
+        public void setParameterIfAbsent(String name, String... value) {
+            parameterMap.putIfAbsent(name, value);
         }
 
         public String getParameter(String name) {

diff --git a/...er/src/main/java/org/cloudfoundry/identity/uaa/authentication/WhitelistLogoutHandler.java b/...er/src/main/java/org/cloudfoundry/identity/uaa/authentication/WhitelistLogoutHandler.java
@@ -11,6 +11,8 @@
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -60,11 +62,14 @@ public void setClientDetailsService(ClientDetailsService clientDetailsService) {
     private Set<String> getClientWhitelist(HttpServletRequest request) {
         String clientId = request.getParameter(CLIENT_ID);
         Set<String> redirectUris = null;
-        try {
-            ClientDetails client = clientDetailsService.loadClientByClientId(clientId);
-            redirectUris = client.getRegisteredRedirectUri();
-        } catch (NoSuchClientException x) {
-            logger.debug(String.format("Unable to find client with ID:%s for logout redirect", clientId));
+
+        if (StringUtils.hasText(clientId)) {
+            try {
+                ClientDetails client = clientDetailsService.loadClientByClientId(clientId);
+                redirectUris = client.getRegisteredRedirectUri();
+            } catch (NoSuchClientException x) {
+                logger.debug(String.format("Unable to find client with ID:%s for logout redirect", clientId));
+            }
         }
         return redirectUris;
     }
@@ -73,14 +78,25 @@ private Set<String> getClientWhitelist(HttpServletRequest request) {
     protected String determineTargetUrl(HttpServletRequest request, HttpServletResponse response) {
         String targetUrl = super.determineTargetUrl(request, response);
         String defaultTargetUrl = getDefaultTargetUrl();
-        if(targetUrl.equals(defaultTargetUrl)) {
+        if (targetUrl.equals(defaultTargetUrl)) {
             return targetUrl;
         }
 
         Set<String> clientWhitelist = getClientWhitelist(request);
-        String whiteListRedirect = UaaUrlUtils.findMatchingRedirectUri(whitelist, targetUrl, defaultTargetUrl);
-        String redirectUrl = UaaUrlUtils.findMatchingRedirectUri(clientWhitelist, targetUrl, whiteListRedirect);
+        Set<String> combinedWhitelist = combineSets(whitelist, clientWhitelist);
+        String whiteListRedirect = UaaUrlUtils.findMatchingRedirectUri(combinedWhitelist, targetUrl, defaultTargetUrl);
+
+        return whiteListRedirect;
+    }
 
-        return redirectUrl;
+    private static <T> Set<T> combineSets(Collection<T>... sets) {
+        Set<T> combined = null;
+        for(Collection<T> set : sets) {
+            if(set != null) {
+                if(combined == null) { combined = new HashSet<>(set); }
+                else { combined.addAll(set); }
+            }
+        }
+        return combined;
     }
 }
diff --git a/...rc/test/java/org/cloudfoundry/identity/uaa/authentication/WhitelistLogoutHandlerTest.java b/...rc/test/java/org/cloudfoundry/identity/uaa/authentication/WhitelistLogoutHandlerTest.java
@@ -121,4 +121,4 @@ public void test_client_redirect_using_wildcard() throws Exception {
         assertEquals("http://www.testing.com", handler.determineTargetUrl(request, response));
     }
 
-}
+}
diff --git a/...va/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java b/...va/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java
@@ -153,4 +153,4 @@ public void test_client_redirect_using_wildcard() throws Exception {
         assertEquals("http://www.testing.com", handler.determineTargetUrl(request, response));
     }
 
-}
+}