KEYCLOAK-19143 Split note for broker and SP SAML request ID

services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java

@@ -445,7 +445,7 @@ public class SAMLEndpoint {
                 }
 
                 // Validate InResponseTo attribute: must match the generated request ID
-                String expectedRequestId = authSession.getClientNote(SamlProtocol.SAML_REQUEST_ID);
+                String expectedRequestId = authSession.getClientNote(SamlProtocol.SAML_REQUEST_ID_BROKER);
                 final boolean inResponseToValidationSuccess = validateInResponseToAttribute(responseType, expectedRequestId);
                 if (!inResponseToValidationSuccess)
                 {

services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java

@@ -189,7 +189,7 @@ public class SAMLIdentityProvider extends AbstractIdentityProvider<SAMLIdentityP
             }
 
             // Save the current RequestID in the Auth Session as we need to verify it against the ID returned from the IdP
-            request.getAuthenticationSession().setClientNote(SamlProtocol.SAML_REQUEST_ID, authnRequest.getID());
+            request.getAuthenticationSession().setClientNote(SamlProtocol.SAML_REQUEST_ID_BROKER, authnRequest.getID());
 
             if (postBinding) {
                 return binding.postBinding(authnRequestBuilder.toDocument()).request(destinationUrl);

services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java

@@ -122,6 +122,7 @@ public class SamlProtocol implements LoginProtocol {
     public static final String SAML_SOAP_BINDING = "soap";
     public static final String SAML_REDIRECT_BINDING = "get";
     public static final String SAML_REQUEST_ID = "SAML_REQUEST_ID";
+    public static final String SAML_REQUEST_ID_BROKER = "SAML_REQUEST_ID_BROKER";
     public static final String SAML_LOGOUT_BINDING = "saml.logout.binding";
     public static final String SAML_LOGOUT_ADD_EXTENSIONS_ELEMENT_WITH_KEY_INFO = "saml.logout.addExtensionsElementWithKeyInfo";
     public static final String SAML_SERVER_SIGNATURE_KEYINFO_KEY_NAME_TRANSFORMER = "SAML_SERVER_SIGNATURE_KEYINFO_KEY_NAME_TRANSFORMER";

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BrokerTest.java

@@ -18,6 +18,7 @@ package org.keycloak.testsuite.saml;
 
 import org.keycloak.admin.client.resource.ClientsResource;
 import org.keycloak.admin.client.resource.RealmResource;
+import org.keycloak.admin.client.resource.UserResource;
 import org.keycloak.authentication.authenticators.broker.IdpReviewProfileAuthenticatorFactory;
 import org.keycloak.broker.saml.SAMLIdentityProviderConfig;
 import org.keycloak.broker.saml.SAMLIdentityProviderFactory;
@@ -56,6 +57,7 @@ import java.security.KeyPair;
 import java.util.List;
 import java.util.Objects;
 import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
 import javax.ws.rs.core.Response.Status;
 import javax.xml.datatype.XMLGregorianCalendar;
 import javax.xml.namespace.QName;
@@ -69,8 +71,9 @@ import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 import org.w3c.dom.NodeList;
 
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
-import static org.junit.Assert.assertThat;
+import static org.hamcrest.Matchers.is;
 import static org.keycloak.saml.SignatureAlgorithm.RSA_SHA1;
 import static org.keycloak.testsuite.saml.AbstractSamlTest.REALM_NAME;
 import static org.keycloak.testsuite.saml.AbstractSamlTest.SAML_ASSERTION_CONSUMER_URL_SALES_POST;
@@ -179,6 +182,47 @@ public class BrokerTest extends AbstractSamlTest {
         }
     }
 
+    @Test
+    public void testInResponseToSetCorrectly() throws IOException {
+        final RealmResource realm = adminClient.realm(REALM_NAME);
+
+        try (IdentityProviderCreator idp = new IdentityProviderCreator(realm, addIdentityProvider("https://saml.idp/saml"))) {
+            AtomicReference<String> serviceProvidersId = new AtomicReference<>();
+            SAMLDocumentHolder samlResponse = new SamlClientBuilder()
+              .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST)
+                .transformObject(ar -> {
+                    serviceProvidersId.set(ar.getID());
+                    return ar;
+                })
+                .build()
+
+              .login().idp(SAML_BROKER_ALIAS).build()
+
+              // Virtually perform login at IdP (return artificial SAML response)
+              .processSamlResponse(REDIRECT)
+                .transformObject(this::createAuthnResponse)
+                .targetAttributeSamlResponse()
+                .targetUri(getSamlBrokerUrl(REALM_NAME))
+                .build()
+              .followOneRedirect()  // first-broker-login
+              .updateProfile().username("userInResponseTo").email("f@g.h").firstName("a").lastName("b").build()
+              .followOneRedirect()  // after-first-broker-login
+              .getSamlResponse(POST);
+
+            assertThat(samlResponse.getSamlObject(), isSamlStatusResponse(JBossSAMLURIConstants.STATUS_SUCCESS));
+            assertThat(((ResponseType) samlResponse.getSamlObject()).getInResponseTo(), is(serviceProvidersId.get()));
+        } finally {
+            clearUsers(realm);
+        }
+    }
+
+    private void clearUsers(final RealmResource realm) {
+        realm.users().list().stream()
+          .map(UserRepresentation::getId)
+          .map(realm.users()::get)
+          .forEach(UserResource::remove);
+    }
+
     @Test
     public void testNoNameIDAndPrincipalFromAttribute() throws IOException {
         final String userName = "newUser-" + UUID.randomUUID();

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/ConcurrentAuthnRequestTest.java

@@ -17,9 +17,14 @@
 package org.keycloak.testsuite.saml;
 
 import org.keycloak.dom.saml.v2.protocol.AuthnRequestType;
+import org.keycloak.dom.saml.v2.protocol.ResponseType;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
+import org.keycloak.saml.SAMLRequestParser;
+import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
 import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request;
+import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
+import org.keycloak.testsuite.util.Matchers;
 import org.keycloak.testsuite.util.SamlClient;
 import org.keycloak.testsuite.util.saml.LoginBuilder;
 import org.keycloak.testsuite.utils.io.IOUtil;
@@ -43,6 +48,8 @@ import org.junit.Ignore;
 import org.junit.Test;
 import org.w3c.dom.Document;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
 import static org.keycloak.testsuite.util.SamlClient.*;
 
 /**
@@ -55,20 +62,22 @@ public class ConcurrentAuthnRequestTest extends AbstractSamlTest {
     public static final int ITERATIONS = 10000;
     public static final int CONCURRENT_THREADS = 5;
 
-    private static void loginRepeatedly(UserRepresentation user, URI samlEndpoint,
+    private void loginRepeatedly(UserRepresentation user, URI samlEndpoint,
-      Document samlRequest, String relayState, Binding requestBinding) {
+      String relayState, Binding requestBinding) {
         CloseableHttpResponse response = null;
         SamlClient.RedirectStrategyWithSwitchableFollowRedirect strategy = new SamlClient.RedirectStrategyWithSwitchableFollowRedirect();
         ExecutorService threadPool = Executors.newFixedThreadPool(CONCURRENT_THREADS);
 
         try (CloseableHttpClient client = HttpClientBuilder.create().setRedirectStrategy(strategy).build()) {
-            HttpUriRequest post = requestBinding.createSamlUnsignedRequest(samlEndpoint, relayState, samlRequest);
             
             Collection<Callable<Void>> futures = new LinkedList<>();
             for (int i = 0; i < ITERATIONS; i ++) {
                 final int j = i;
+                AuthnRequestType loginRep = createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, REALM_NAME);
+                Document samlRequest = SAML2Request.convert(loginRep);
+                HttpUriRequest post = requestBinding.createSamlUnsignedRequest(samlEndpoint, relayState, samlRequest);
                 Callable<Void> f = () -> {
-                    performLogin(post, samlEndpoint, relayState, samlRequest, response, client, user, strategy);
+                    performLogin(post, samlEndpoint, relayState, loginRep.getID(), samlRequest, response, client, user, strategy);
                     return null;
                 };
                 futures.add(f);
@@ -81,7 +90,7 @@ public class ConcurrentAuthnRequestTest extends AbstractSamlTest {
     }
 
     public static void performLogin(HttpUriRequest post, URI samlEndpoint, String relayState,
-      Document samlRequest, CloseableHttpResponse response, final CloseableHttpClient client,
+      String requestId, Document samlRequest, CloseableHttpResponse response, final CloseableHttpClient client,
       UserRepresentation user,
       RedirectStrategyWithSwitchableFollowRedirect strategy) {
         try {
@@ -95,6 +104,9 @@ public class ConcurrentAuthnRequestTest extends AbstractSamlTest {
 
             strategy.setRedirectable(false);
             response = client.execute(loginRequest, context);
+            SAMLDocumentHolder parseResponsePostBinding = SAMLRequestParser.parseResponsePostBinding(EntityUtils.toString(response.getEntity()));
+            assertThat(parseResponsePostBinding.getSamlObject(), Matchers.isSamlResponse(JBossSAMLURIConstants.STATUS_SUCCESS));
+            assertThat(((ResponseType) parseResponsePostBinding.getSamlObject()).getInResponseTo(), is(requestId));
             response.close();
         } catch (Exception ex) {
             throw new RuntimeException(ex);
@@ -117,9 +129,7 @@ public class ConcurrentAuthnRequestTest extends AbstractSamlTest {
     }
 
     private void testLogin(Binding requestBinding) throws Exception {
-        AuthnRequestType loginRep = createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, REALM_NAME);
+        loginRepeatedly(bburkeUser, getAuthServerSamlEndpoint(REALM_NAME), null, requestBinding);
-        Document samlRequest = SAML2Request.convert(loginRep);
-        loginRepeatedly(bburkeUser, getAuthServerSamlEndpoint(REALM_NAME), samlRequest, null, requestBinding);
     }
 
     @Test