fix: only redirect a failed SAML sign-in to a redirect_uri registered…

docs/UAA-Configuration-Reference.md

@@ -2390,6 +2390,11 @@ Bootstrap SAML Identity Provider definitions. Each entry is keyed by a provider
 | `authnContext` | `List<String>` | Requested authentication context class references |
 | `override` | `boolean` | Overwrite an existing provider with the same alias on startup. Default `true` |
 
+When SAML sign-in fails during a saved authorization request (for example, when `addShadowUserOnLogin` is `false`
+and no shadow user exists), UAA sends `error=access_denied` and `error_description` only to a `redirect_uri`
+that resolves against the client's registered redirect URIs in the current identity zone. If the saved URI is
+malformed or unregistered, or the `client_id` is missing or unknown, UAA falls back to `/saml_error`.
+
 #### `idpMetadata` and entity ID resolution
 
 The value of `idpMetadata` determines how UAA parses the IDP's entity ID:

server/src/main/java/org/cloudfoundry/identity/uaa/provider/saml/SamlAuthenticationFilterConfig.java

@@ -3,14 +3,17 @@
 import org.cloudfoundry.identity.uaa.authentication.SamlLogoutRequestValidator;
 import org.cloudfoundry.identity.uaa.authentication.SamlLogoutResponseValidator;
 import org.cloudfoundry.identity.uaa.login.UaaAuthenticationFailureHandler;
+import org.cloudfoundry.identity.uaa.oauth.provider.endpoint.RedirectResolver;
 import org.cloudfoundry.identity.uaa.provider.JdbcIdentityProviderProvisioning;
 import org.cloudfoundry.identity.uaa.scim.ScimGroupExternalMembershipManager;
 import org.cloudfoundry.identity.uaa.security.web.CookieBasedCsrfTokenRepository;
 import org.cloudfoundry.identity.uaa.user.UaaUserDatabase;
 import org.cloudfoundry.identity.uaa.web.UaaSavedRequestAwareAuthenticationSuccessHandler;
+import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
 import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
 import org.opensaml.saml.saml2.core.LogoutRequest;
 import org.opensaml.saml.saml2.core.NameID;
+import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.boot.web.servlet.FilterRegistrationBean;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.context.annotation.Bean;
@@ -133,8 +136,12 @@ FilterRegistrationBean<Filter> saml2WebSsoAuthenticationFilter(AuthenticationPro
      * Handler deciding where to redirect user after unsuccessful login
      */
     @Bean
-    public SamlLoginAuthenticationFailureHandler samlLoginAuthenticationFailureHandler() {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+    public SamlLoginAuthenticationFailureHandler samlLoginAuthenticationFailureHandler(
+            RedirectResolver redirectResolver,
+            @Qualifier("jdbcClientDetailsService") MultitenantClientServices clientServices,
+            IdentityZoneManager identityZoneManager) {
+        SamlLoginAuthenticationFailureHandler handler =
+                new SamlLoginAuthenticationFailureHandler(redirectResolver, clientServices, identityZoneManager);
         handler.setDefaultFailureUrl("/saml_error");
         return handler;
     }

server/src/main/java/org/cloudfoundry/identity/uaa/provider/saml/SamlLoginAuthenticationFailureHandler.java

@@ -3,7 +3,13 @@
 import lombok.extern.slf4j.Slf4j;
 import org.apache.hc.core5.net.URIBuilder;
 import org.cloudfoundry.identity.uaa.authentication.MalformedSamlResponseLogger;
+import org.cloudfoundry.identity.uaa.oauth.common.exceptions.OAuth2Exception;
+import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails;
+import org.cloudfoundry.identity.uaa.oauth.provider.endpoint.RedirectResolver;
+import org.cloudfoundry.identity.uaa.provider.ClientRegistrationException;
 import org.cloudfoundry.identity.uaa.util.SessionUtils;
+import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
+import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
 import org.springframework.security.authentication.AuthenticationServiceException;
 import org.springframework.security.core.AuthenticationException;
 import org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException;
@@ -22,10 +28,26 @@
  * with LoginSAMLException. Currently, the only scenario for this is when a
  * shadow account does not exist for the user and the IdP configuration does not
  * allow automatic creation of the shadow account.
+ * <p>
+ * The error is only sent to the {@code redirect_uri} of the saved authorization request when that
+ * URI is registered for the client of the request. Otherwise the default failure URL is used.
  */
 @Slf4j
 public class SamlLoginAuthenticationFailureHandler extends SimpleUrlAuthenticationFailureHandler {
+
     MalformedSamlResponseLogger malformedLogger = new MalformedSamlResponseLogger();
+    private final RedirectResolver redirectResolver;
+    private final MultitenantClientServices clientServices;
+    private final IdentityZoneManager identityZoneManager;
+
+    public SamlLoginAuthenticationFailureHandler(
+            final RedirectResolver redirectResolver,
+            final MultitenantClientServices clientServices,
+            final IdentityZoneManager identityZoneManager) {
+        this.redirectResolver = redirectResolver;
+        this.clientServices = clientServices;
+        this.identityZoneManager = identityZoneManager;
+    }
 
     @Override
     public void onAuthenticationFailure(final HttpServletRequest request, final HttpServletResponse response,
@@ -63,8 +85,11 @@ private String handleSamlLoginException(HttpServletRequest request, HttpServletR
                 String[] redirectURI = savedRequest.getParameterMap().get("redirect_uri");
 
                 if (redirectURI != null && redirectURI.length > 0) {
-                    URI uri = URI.create(redirectURI[0]);
-                    URIBuilder uriBuilder = new URIBuilder(uri);
+                    String registeredRedirect = resolveRegisteredRedirect(savedRequest, redirectURI[0]);
+                    if (registeredRedirect == null) {
+                        return null;
+                    }
+                    URIBuilder uriBuilder = new URIBuilder(URI.create(registeredRedirect));
                     uriBuilder.addParameter("error", "access_denied");
                     uriBuilder.addParameter("error_description", exception.getMessage());
                     redirectTo = uriBuilder.toString();
@@ -77,4 +102,18 @@ private String handleSamlLoginException(HttpServletRequest request, HttpServletR
 
         return redirectTo;
     }
+
+    private String resolveRegisteredRedirect(DefaultSavedRequest savedRequest, String requestedRedirect) {
+        String[] clientId = savedRequest.getParameterMap().get("client_id");
+        if (clientId == null || clientId.length == 0) {
+            return null;
+        }
+        try {
+            ClientDetails client = clientServices.loadClientByClientId(clientId[0], identityZoneManager.getCurrentIdentityZoneId());
+            return redirectResolver.resolveRedirect(requestedRedirect, client);
+        } catch (ClientRegistrationException | OAuth2Exception | IllegalArgumentException e) {
+            log.debug("SAML sign-in failed, the redirect_uri is not registered for the client of the request: {}", e.getMessage());
+            return null;
+        }
+    }
 }

server/src/test/java/org/cloudfoundry/identity/uaa/provider/saml/SamlLoginAuthenticationFailureHandlerTest.java

@@ -1,8 +1,18 @@
 package org.cloudfoundry.identity.uaa.provider.saml;
 
 import org.apache.hc.core5.http.HttpStatus;
+import org.cloudfoundry.identity.uaa.client.UaaClientDetails;
+import org.cloudfoundry.identity.uaa.oauth.beans.RedirectResolverFactoryBean;
+import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails;
+import org.cloudfoundry.identity.uaa.oauth.provider.endpoint.RedirectResolver;
+import org.cloudfoundry.identity.uaa.oauth.common.exceptions.RedirectMismatchException;
+import org.cloudfoundry.identity.uaa.provider.ClientRegistrationException;
 import org.cloudfoundry.identity.uaa.util.SessionUtils;
+import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
+import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.mock.web.MockHttpSession;
@@ -14,18 +24,33 @@
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 class SamlLoginAuthenticationFailureHandlerTest {
 
+    private final RedirectResolver redirectResolver = mock(RedirectResolver.class);
+    private final MultitenantClientServices clientServices = mock(MultitenantClientServices.class);
+    private final IdentityZoneManager identityZoneManager = mock(IdentityZoneManager.class);
+
+    private SamlLoginAuthenticationFailureHandler newHandler() throws Exception {
+        when(identityZoneManager.getCurrentIdentityZoneId()).thenReturn("uaa");
+        when(clientServices.loadClientByClientId("client", "uaa")).thenReturn(mock(ClientDetails.class));
+        // a registered redirect_uri is resolved to itself
+        when(redirectResolver.resolveRedirect(anyString(), any())).thenAnswer(invocation -> invocation.getArgument(0));
+        return new SamlLoginAuthenticationFailureHandler(redirectResolver, clientServices, identityZoneManager);
+    }
+
     @Test
     void errorRedirect() throws Exception {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
 
         DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
         Map<String, String[]> parameterMap = new HashMap<>();
         parameterMap.put("redirect_uri", new String[]{"https://example.com"});
+        parameterMap.put("client_id", new String[]{"client"});
         when(savedRequest.getParameterMap()).thenReturn(parameterMap);
 
         MockHttpSession session = new MockHttpSession();
@@ -45,11 +70,12 @@ void errorRedirect() throws Exception {
 
     @Test
     void errorRedirectWithExistingQueryParameters() throws Exception {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
 
         DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
         Map<String, String[]> parameterMap = new HashMap<>();
         parameterMap.put("redirect_uri", new String[]{"https://example.com?go=bears"});
+        parameterMap.put("client_id", new String[]{"client"});
         when(savedRequest.getParameterMap()).thenReturn(parameterMap);
 
         MockHttpSession session = new MockHttpSession();
@@ -69,11 +95,12 @@ void errorRedirectWithExistingQueryParameters() throws Exception {
 
     @Test
     void someOtherErrorCondition() throws Exception {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
 
         DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
         Map<String, String[]> parameterMap = new HashMap<>();
         parameterMap.put("redirect_uri", new String[]{"https://example.com?go=bears"});
+        parameterMap.put("client_id", new String[]{"client"});
         when(savedRequest.getParameterMap()).thenReturn(parameterMap);
 
         MockHttpSession session = new MockHttpSession();
@@ -98,7 +125,7 @@ void someOtherErrorCondition() throws Exception {
 
     @Test
     void noSession() throws Exception {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
 
         MockHttpServletRequest request = new MockHttpServletRequest();
         MockHttpServletResponse response = new MockHttpServletResponse();
@@ -114,11 +141,12 @@ void noSession() throws Exception {
 
     @Test
     void noSavedRequest() throws Exception {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
 
         DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
         Map<String, String[]> parameterMap = new HashMap<>();
         parameterMap.put("redirect_uri", new String[]{"https://example.com"});
+        parameterMap.put("client_id", new String[]{"client"});
         when(savedRequest.getParameterMap()).thenReturn(parameterMap);
 
         MockHttpSession session = new MockHttpSession();
@@ -137,7 +165,7 @@ void noSavedRequest() throws Exception {
 
     @Test
     void noRedirectURI() throws Exception {
-        SamlLoginAuthenticationFailureHandler handler = new SamlLoginAuthenticationFailureHandler();
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
 
         DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
         Map<String, String[]> parameterMap = new HashMap<>();
@@ -156,4 +184,112 @@ void noRedirectURI() throws Exception {
         int status = response.getStatus();
         assertThat(status).isEqualTo(HttpStatus.SC_UNAUTHORIZED);
     }
+
+    @Test
+    void unregisteredRedirectUriIsNotFollowed() throws Exception {
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
+        when(redirectResolver.resolveRedirect(anyString(), any())).thenThrow(new RedirectMismatchException("no match"));
+        Map<String, String[]> parameterMap = new HashMap<>();
+        parameterMap.put("redirect_uri", new String[]{"https://unregistered.example.com"});
+        parameterMap.put("client_id", new String[]{"client"});
+
+        DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
+        when(savedRequest.getParameterMap()).thenReturn(parameterMap);
+
+        MockHttpSession session = new MockHttpSession();
+        SessionUtils.setSavedRequestSession(session, savedRequest);
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        request.setSession(session);
+        MockHttpServletResponse response = new MockHttpServletResponse();
+
+        handler.onAuthenticationFailure(request, response, new SamlLoginException("Denied!"));
+
+        String actual = response.getRedirectedUrl();
+        assertThat(actual).isNull();
+        assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_UNAUTHORIZED);
+    }
+
+    @ParameterizedTest
+    @CsvSource({
+            "false, /callback",
+            "false, //example.com/callback",
+            "false, https:/callback",
+            "false, https://[invalid",
+            "true, /callback",
+            "true, //example.com/callback",
+            "true, https:/callback",
+            "true, https://[invalid"
+    })
+    void malformedRedirectUriFallsBackToSamlError(boolean allowUnsafeMatching, String redirectUri) throws Exception {
+        when(identityZoneManager.getCurrentIdentityZoneId()).thenReturn("uaa");
+        UaaClientDetails client = new UaaClientDetails("client", "", "openid", "authorization_code", "",
+                "https://example.com/callback");
+        when(clientServices.loadClientByClientId("client", "uaa")).thenReturn(client);
+        RedirectResolver resolver = new RedirectResolverFactoryBean(allowUnsafeMatching).getObject();
+        SamlLoginAuthenticationFailureHandler handler =
+                new SamlLoginAuthenticationFailureHandler(resolver, clientServices, identityZoneManager);
+        handler.setDefaultFailureUrl("/saml_error");
+
+        DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
+        when(savedRequest.getParameterMap()).thenReturn(Map.of(
+                "redirect_uri", new String[]{redirectUri},
+                "client_id", new String[]{"client"}));
+
+        MockHttpSession session = new MockHttpSession();
+        SessionUtils.setSavedRequestSession(session, savedRequest);
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        request.setSession(session);
+        MockHttpServletResponse response = new MockHttpServletResponse();
+
+        handler.onAuthenticationFailure(request, response, new SamlLoginException("Denied!"));
+
+        assertThat(response.getRedirectedUrl()).isEqualTo("/saml_error");
+        assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_MOVED_TEMPORARILY);
+    }
+
+    @Test
+    void unknownClientIsNotFollowed() throws Exception {
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
+        when(clientServices.loadClientByClientId("client", "uaa")).thenThrow(new ClientRegistrationException("no client"));
+        Map<String, String[]> parameterMap = new HashMap<>();
+        parameterMap.put("redirect_uri", new String[]{"https://unregistered.example.com"});
+        parameterMap.put("client_id", new String[]{"client"});
+
+        DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
+        when(savedRequest.getParameterMap()).thenReturn(parameterMap);
+
+        MockHttpSession session = new MockHttpSession();
+        SessionUtils.setSavedRequestSession(session, savedRequest);
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        request.setSession(session);
+        MockHttpServletResponse response = new MockHttpServletResponse();
+
+        handler.onAuthenticationFailure(request, response, new SamlLoginException("Denied!"));
+
+        String actual = response.getRedirectedUrl();
+        assertThat(actual).isNull();
+        assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_UNAUTHORIZED);
+    }
+
+    @Test
+    void requestWithoutClientIdIsNotFollowed() throws Exception {
+        SamlLoginAuthenticationFailureHandler handler = newHandler();
+        Map<String, String[]> parameterMap = new HashMap<>();
+        parameterMap.put("redirect_uri", new String[]{"https://unregistered.example.com"});
+
+        DefaultSavedRequest savedRequest = mock(DefaultSavedRequest.class);
+        when(savedRequest.getParameterMap()).thenReturn(parameterMap);
+
+        MockHttpSession session = new MockHttpSession();
+        SessionUtils.setSavedRequestSession(session, savedRequest);
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        request.setSession(session);
+        MockHttpServletResponse response = new MockHttpServletResponse();
+
+        handler.onAuthenticationFailure(request, response, new SamlLoginException("Denied!"));
+
+        String actual = response.getRedirectedUrl();
+        assertThat(actual).isNull();
+        assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_UNAUTHORIZED);
+    }
 }