logout redirect changes

- always support "redirect" parameter by default (had to be enabled through config before)
- disable open redirects completely
[#134954469] https://www.pivotaltracker.com/story/show/134954469

model/src/main/java/org/cloudfoundry/identity/uaa/zone/Links.java

@@ -18,6 +18,7 @@
 import com.fasterxml.jackson.annotation.JsonInclude;
 
 import java.util.List;
+import java.util.Optional;
 
 @JsonInclude(JsonInclude.Include.NON_NULL)
 @JsonIgnoreProperties(ignoreUnknown = true)
@@ -57,7 +58,7 @@ public Links setHomeRedirect(String homeRedirect) {
     public static class Logout {
         private String redirectUrl = "/login";
         private String redirectParameterName = "redirect";
-        private boolean disableRedirectParameter = true;
+        private boolean disableRedirectParameter = false;
         private List<String> whitelist = null;
 
         public boolean isDisableRedirectParameter() {
@@ -79,7 +80,7 @@ public Logout setRedirectParameterName(String redirectParameterName) {
         }
 
         public String getRedirectUrl() {
-            return redirectUrl;
+            return Optional.ofNullable(redirectUrl).orElse("/login");
         }
 
         public Logout setRedirectUrl(String redirectUrl) {

server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java

@@ -30,6 +30,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 
 import static org.cloudfoundry.identity.uaa.codestore.ExpiringCodeType.REGISTRATION;
@@ -125,7 +126,7 @@ private String getRedirect(String clientId, String redirectUri) throws IOExcepti
                 Set<String> registeredRedirectUris = clientDetails.getRegisteredRedirectUri() == null ? Collections.emptySet() :
                         clientDetails.getRegisteredRedirectUri();
                 String signupRedirectUrl = (String) clientDetails.getAdditionalInformation().get(SIGNUP_REDIRECT_URL);
-                String matchingRedirectUri = findMatchingRedirectUri(registeredRedirectUris, redirectUri, signupRedirectUrl);
+                String matchingRedirectUri = findMatchingRedirectUri(registeredRedirectUris, redirectUri, Optional.ofNullable(signupRedirectUrl).orElse("home"));
 
                 if (matchingRedirectUri != null) {
                     return matchingRedirectUri;

server/src/main/java/org/cloudfoundry/identity/uaa/account/UaaResetPasswordService.java

@@ -134,7 +134,7 @@ private ResetPasswordResponse changePasswordCodeAuthenticated(String code, Strin
                     ClientDetails clientDetails = clientDetailsService.loadClientByClientId(clientId);
                     Set<String> redirectUris = clientDetails.getRegisteredRedirectUri() == null ? Collections.emptySet() :
                         clientDetails.getRegisteredRedirectUri();
-                    String matchingRedirectUri = UaaUrlUtils.findMatchingRedirectUri(redirectUris, redirectUri, null);
+                    String matchingRedirectUri = UaaUrlUtils.findMatchingRedirectUri(redirectUris, redirectUri, redirectLocation);
                     if (matchingRedirectUri != null) {
                         redirectLocation = matchingRedirectUri;
                     }

server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java

@@ -33,6 +33,9 @@
 import java.util.List;
 import java.util.Map;
 
+import static java.util.Collections.emptyList;
+import static java.util.Optional.ofNullable;
+
 public abstract class UaaUrlUtils {
 
     public static String getUaaUrl() {
@@ -68,20 +71,28 @@ public static UriComponentsBuilder getURIBuilder(String path, boolean zoneSwitch
         return builder;
     }
 
+    /**
+     * Finds and returns a matching redirect URL according to the following logic:
+     * <ul>
+     *     <li>If the requstedRedirectUri matches the whitelist the requestedRedirectUri is returned</li>
+     *     <li>If the whitelist is null or empty AND the fallbackRedirectUri is null, the requestedRedirectUri is returned - OPEN REDIRECT</li>
+     *     <li>If the whitelist is null or empty AND the fallbackRedirectUri is not null, the fallbackRedirectUri is returned</li>
+     * </ul>
+     * @param redirectUris - a whitelist collection of ant path patterns
+     * @param requestedRedirectUri - the requested redirect URI, returned if whitelist matches or the fallbackRedirectUri is null
+     * @param fallbackRedirectUri - returned if non null and the requestedRedirectUri doesn't match the whitelist redirectUris
+     * @return a redirect URI, either the requested or fallback as described above
+     */
     public static String findMatchingRedirectUri(Collection<String> redirectUris, String requestedRedirectUri, String fallbackRedirectUri) {
         AntPathMatcher matcher = new AntPathMatcher();
 
-        if (redirectUris == null) {
-            return requestedRedirectUri;
-        }
-
-        for (String pattern : redirectUris) {
+        for (String pattern : ofNullable(redirectUris).orElse(emptyList())) {
             if (matcher.match(pattern, requestedRedirectUri)) {
                 return requestedRedirectUri;
             }
         }
 
-        return fallbackRedirectUri;
+        return ofNullable(fallbackRedirectUri).orElse(requestedRedirectUri);
     }
 
     public static String getHostForURI(String uri) {

server/src/test/java/org/cloudfoundry/identity/uaa/authentication/WhitelistLogoutHandlerTest.java

@@ -67,13 +67,14 @@ public void test_whitelist_reject() throws Exception {
     }
 
     @Test
-    public void test_allow_open_redirect() throws Exception {
+    public void test_open_redirect_no_longer_allowed() throws Exception {
         handler.setWhitelist(null);
         handler.setAlwaysUseDefaultTargetUrl(false);
+        handler.setDefaultTargetUrl("/login");
         request.setParameter("redirect", "http://testing.com");
-        assertEquals("http://testing.com", handler.determineTargetUrl(request, response));
+        assertEquals("/login", handler.determineTargetUrl(request, response));
         request.setParameter("redirect", "http://www.testing.com");
-        assertEquals("http://www.testing.com", handler.determineTargetUrl(request, response));
+        assertEquals("/login", handler.determineTargetUrl(request, response));
     }
 
     @Test

server/src/test/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java

@@ -29,8 +29,6 @@
 import java.util.Arrays;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static org.springframework.security.oauth2.common.util.OAuth2Utils.CLIENT_ID;
@@ -91,13 +89,14 @@ public void test_whitelist_reject() throws Exception {
     }
 
     @Test
-    public void test_allow_open_redirect() throws Exception {
+    public void test_open_redirect_no_longer_allowed() throws Exception {
         configuration.getLinks().getLogout().setWhitelist(null);
+        configuration.getLinks().getLogout().setRedirectUrl("/login");
         configuration.getLinks().getLogout().setDisableRedirectParameter(false);
         request.setParameter("redirect", "http://testing.com");
-        assertEquals("http://testing.com", handler.determineTargetUrl(request, response));
+        assertEquals("/login", handler.determineTargetUrl(request, response));
         request.setParameter("redirect", "http://www.testing.com");
-        assertEquals("http://www.testing.com", handler.determineTargetUrl(request, response));
+        assertEquals("/login", handler.determineTargetUrl(request, response));
     }
 
     @Test

server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java

@@ -200,14 +200,22 @@ public void testXForwardedPrefixUrls() {
 
     @Test
     public void findMatchingRedirectUri_usesAntPathMatching() {
-        String pattern1 = "http://matching.redirect/*";
-        String redirect1 = "http://matching.redirect/";
-        String matchingRedirectUri1 = UaaUrlUtils.findMatchingRedirectUri(Collections.singleton(pattern1), redirect1, null);
-        assertThat(matchingRedirectUri1, equalTo(redirect1));
-
-        String redirect2 = "http://matching.redirect/anything-but-forward-slash";
-        String matchingRedirectUri2 = UaaUrlUtils.findMatchingRedirectUri(Collections.singleton(pattern1), redirect2, null);
-        assertThat(matchingRedirectUri2, equalTo(redirect2));
+        //matches pattern
+        String matchingRedirectUri1 = UaaUrlUtils.findMatchingRedirectUri(Collections.singleton("http://matching.redirect/*"), "http://matching.redirect/", null);
+        assertThat(matchingRedirectUri1, equalTo("http://matching.redirect/"));
+
+        //matches pattern
+
+        String matchingRedirectUri2 = UaaUrlUtils.findMatchingRedirectUri(Collections.singleton("http://matching.redirect/*"), "http://matching.redirect/anything-but-forward-slash", null);
+        assertThat(matchingRedirectUri2, equalTo("http://matching.redirect/anything-but-forward-slash"));
+
+        //does not match pattern, but no fallback
+        matchingRedirectUri2 = UaaUrlUtils.findMatchingRedirectUri(Collections.singleton("http://matching.redirect/*"), "http://does.not.match/redirect", null);
+        assertThat(matchingRedirectUri2, equalTo("http://does.not.match/redirect"));
+
+        //does not match pattern, but fallback provided
+        matchingRedirectUri2 = UaaUrlUtils.findMatchingRedirectUri(Collections.singleton("http://matching.redirect/*"), "http://does.not.match/redirect", "http://fallback.url/redirect");
+        assertThat(matchingRedirectUri2, equalTo("http://fallback.url/redirect"));
 
         String pattern2 = "http://matching.redirect/**";
         String redirect3 = "http://matching.redirect/whatever/you/want";

uaa/src/main/webapp/WEB-INF/spring-servlet.xml

@@ -437,7 +437,7 @@
                            @config['logout']['redirect']['parameter']['whitelist']}"/>
         <property name="logoutRedirectParameterName" value="redirect" />
         <property name="logoutDefaultRedirectUrl" value="${logout.redirect.url:/login}" />
-        <property name="logoutDisableRedirectParameter" value="${logout.redirect.parameter.disable:true}"/>
+        <property name="logoutDisableRedirectParameter" value="${logout.redirect.parameter.disable:false}"/>
         <property name="prompts" ref="prompts"/>
         <property name="branding" value="#{@config['login']['branding']}" />
         <property name="samlSpPrivateKey" value="${login.serviceProviderKey}" />

uaa/src/test/java/org/cloudfoundry/identity/uaa/login/BootstrapTests.java

@@ -229,7 +229,7 @@ public void testRootContextDefaults() throws Exception {
         assertEquals("redirect", zoneConfiguration.getLinks().getLogout().getRedirectParameterName());
         assertEquals("/login", zoneConfiguration.getLinks().getLogout().getRedirectUrl());
         assertNull(zoneConfiguration.getLinks().getLogout().getWhitelist());
-        assertTrue(zoneConfiguration.getLinks().getLogout().isDisableRedirectParameter());
+        assertFalse(zoneConfiguration.getLinks().getLogout().isDisableRedirectParameter());
         assertFalse(context.getBean(IdentityZoneProvisioning.class).retrieve(IdentityZone.getUaa().getId()).getConfig().getTokenPolicy().isJwtRevocable());
         assertEquals(
             Arrays.asList(
@@ -366,7 +366,7 @@ public void testPropertyValuesWhenSetInYaml() throws Exception {
         assertEquals("redirect", zoneConfiguration.getLinks().getLogout().getRedirectParameterName());
         assertEquals("/configured_login", zoneConfiguration.getLinks().getLogout().getRedirectUrl());
         assertEquals(Arrays.asList("https://url1.domain1.com/logout-success","https://url2.domain2.com/logout-success"), zoneConfiguration.getLinks().getLogout().getWhitelist());
-        assertFalse(zoneConfiguration.getLinks().getLogout().isDisableRedirectParameter());
+        assertTrue(zoneConfiguration.getLinks().getLogout().isDisableRedirectParameter());
 
         assertEquals(SamlLoginServerKeyManagerTests.CERTIFICATE.trim(), zoneConfiguration.getSamlConfig().getCertificate().trim());
         assertEquals(SamlLoginServerKeyManagerTests.KEY.trim(), zoneConfiguration.getSamlConfig().getPrivateKey().trim());

uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcTests.java

@@ -12,7 +12,6 @@
  *******************************************************************************/
 package org.cloudfoundry.identity.uaa.login;
 
-import org.cloudfoundry.identity.uaa.account.UserAccountStatus;
 import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal;
 import org.cloudfoundry.identity.uaa.codestore.JdbcExpiringCodeStore;
 import org.cloudfoundry.identity.uaa.constants.OriginKeys;
@@ -26,9 +25,9 @@
 import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
 import org.cloudfoundry.identity.uaa.provider.LdapIdentityProviderDefinition;
 import org.cloudfoundry.identity.uaa.provider.LockoutPolicy;
+import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition;
 import org.cloudfoundry.identity.uaa.provider.SamlIdentityProviderDefinition;
 import org.cloudfoundry.identity.uaa.provider.UaaIdentityProviderDefinition;
-import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition;
 import org.cloudfoundry.identity.uaa.provider.saml.BootstrapSamlIdentityProviderConfiguratorTests;
 import org.cloudfoundry.identity.uaa.scim.ScimUser;
 import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning;
@@ -53,7 +52,6 @@
 import org.springframework.mock.env.MockEnvironment;
 import org.springframework.mock.env.MockPropertySource;
 import org.springframework.mock.web.MockHttpServletRequest;
-import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.mock.web.MockHttpSession;
 import org.springframework.security.authentication.AnonymousAuthenticationToken;
 import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
@@ -68,7 +66,6 @@
 import org.springframework.security.web.csrf.HttpSessionCsrfTokenRepository;
 import org.springframework.security.web.savedrequest.DefaultSavedRequest;
 import org.springframework.security.web.savedrequest.SavedRequest;
-import org.springframework.test.web.servlet.MockMvc;
 import org.springframework.test.web.servlet.MvcResult;
 import org.springframework.test.web.servlet.ResultMatcher;
 import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;
@@ -82,6 +79,7 @@
 import java.lang.reflect.Field;
 import java.net.URL;
 import java.net.URLEncoder;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -110,8 +108,8 @@
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static org.springframework.http.MediaType.APPLICATION_JSON;
@@ -121,7 +119,6 @@
 import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.securityContext;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.options;
-import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put;
 import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
@@ -555,6 +552,7 @@ public void testLogOutEnableRedirectParameter() throws Exception {
         Links.Logout original = getLogout();
         Links.Logout logout = getLogout();
         logout.setDisableRedirectParameter(false);
+        logout.setWhitelist(Arrays.asList("https://www.google.com"));
         setLogout(logout);
         try {
             getMockMvc().perform(get("/uaa/logout.do").param("redirect", "https://www.google.com").contextPath("/uaa"))
@@ -620,7 +618,7 @@ public void testLogOutNullWhitelistedRedirectParameter() throws Exception {
         Links.Logout original = getLogout();
         Links.Logout logout = getLogout();
         logout.setDisableRedirectParameter(false);
-        logout.setWhitelist(null);
+        logout.setWhitelist(Arrays.asList("http*://www.google.com"));
         setLogout(logout);
         try {
             getMockMvc().perform(get("/uaa/logout.do").param("redirect", "https://www.google.com").contextPath("/uaa"))
@@ -650,19 +648,19 @@ public void testLogOutEmptyWhitelistedRedirectParameter() throws Exception {
     }
 
     @Test
-    public void testLogoutRedirectIsDisabledInZone() throws Exception {
+    public void testLogoutRedirectIsEnabledInZone() throws Exception {
         String zoneId = generator.generate();
         IdentityZone zone = MultitenancyFixture.identityZone(zoneId, zoneId);
         zone.setConfig(new IdentityZoneConfiguration());
         IdentityZoneProvisioning provisioning = getWebApplicationContext().getBean(IdentityZoneProvisioning.class);
         zone = provisioning.create(zone);
-        assertTrue(zone.getConfig().getLinks().getLogout().isDisableRedirectParameter());
+        assertFalse(zone.getConfig().getLinks().getLogout().isDisableRedirectParameter());
     }
 
     @Test
     public void testLogOutChangeUrlValue() throws Exception {
         Links.Logout original = getLogout();
-        assertTrue(original.isDisableRedirectParameter());
+        assertFalse(original.isDisableRedirectParameter());
         Links.Logout logout = getLogout();
         logout.setRedirectUrl("https://www.google.com");
         setLogout(logout);
@@ -765,6 +763,19 @@ public void testLogOut_Config_For_Zone() throws Exception {
                                 .contextPath("/uaa")
                                 .header("Host", zoneId+".localhost")
                                 .param("redirect", "http://google.com")
+        )
+            .andExpect(status().isFound())
+            .andExpect(redirectedUrl("http://test.redirect.com"))
+            .andExpect(emptyCurrentUserCookie());
+
+        zone.getConfig().getLinks().getLogout().setDisableRedirectParameter(false);
+        zone.getConfig().getLinks().getLogout().setWhitelist(asList("http://google.com"));
+        zone = zoneProvisioning.update(zone);
+
+        getMockMvc().perform(get("/uaa/logout.do")
+                                 .contextPath("/uaa")
+                                 .header("Host", zoneId+".localhost")
+                                 .param("redirect", "http://google.com")
         )
             .andExpect(status().isFound())
             .andExpect(redirectedUrl("http://google.com"))

uaa/src/test/resources/test/bootstrap/bootstrap-test.yml

@@ -114,7 +114,7 @@ logout:
   redirect:
     url: /configured_login
     parameter:
-      disable: false
+      disable: true
       whitelist:
         - https://url1.domain1.com/logout-success
         - https://url2.domain2.com/logout-success