KEYCLOAK-8535 Inconsistent SAML Logout endpoint handling

services/src/main/java/org/keycloak/protocol/saml/installation/SamlSPDescriptorClientInstallation.java

@@ -38,18 +38,34 @@ import org.keycloak.dom.saml.v2.metadata.KeyTypes;
  * @version $Revision: 1 $
  */
 public class SamlSPDescriptorClientInstallation implements ClientInstallationProvider {
+
+    public static final String SAML_CLIENT_INSTALATION_SP_DESCRIPTOR = "saml-sp-descriptor";
+    private static final String FALLBACK_ERROR_URL_STRING = "ERROR:ENDPOINT NOT SET";
+
     public static String getSPDescriptorForClient(ClientModel client) {
         SamlClient samlClient = new SamlClient(client);
-        String assertionUrl = client.getAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_POST_ATTRIBUTE);
+        String assertionUrl;
-        if (assertionUrl == null) assertionUrl = client.getManagementUrl();
+        String logoutUrl;
-        String logoutUrl = client.getAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE);
+        String binding;
-        if (logoutUrl == null) logoutUrl = client.getManagementUrl();
+        if (samlClient.forcePostBinding()) {
+            assertionUrl = client.getAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_POST_ATTRIBUTE);
+            logoutUrl = client.getAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE);
+            binding = JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get();
+        } else { //redirect binding
+            assertionUrl = client.getAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_REDIRECT_ATTRIBUTE);
+            logoutUrl = client.getAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_REDIRECT_ATTRIBUTE);
+            binding = JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get();
+        }
+        if (assertionUrl == null || assertionUrl.isEmpty()) assertionUrl = client.getManagementUrl();
+        if (assertionUrl == null || assertionUrl.isEmpty()) assertionUrl = FALLBACK_ERROR_URL_STRING;
+        if (logoutUrl == null || assertionUrl.isEmpty()) logoutUrl = client.getManagementUrl();
+        if (logoutUrl == null || assertionUrl.isEmpty()) logoutUrl = FALLBACK_ERROR_URL_STRING;
         String nameIdFormat = samlClient.getNameIDFormat();
         if (nameIdFormat == null) nameIdFormat = SamlProtocol.SAML_DEFAULT_NAMEID_FORMAT;
         String spCertificate = SPMetadataDescriptor.xmlKeyInfo("        ", null, samlClient.getClientSigningCertificate(), KeyTypes.SIGNING.value(), true);
         String encCertificate = SPMetadataDescriptor.xmlKeyInfo("        ", null, samlClient.getClientEncryptingCertificate(), KeyTypes.ENCRYPTION.value(), true);
-        return SPMetadataDescriptor.getSPDescriptor(JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get(), assertionUrl, logoutUrl,
+        return SPMetadataDescriptor.getSPDescriptor(binding, assertionUrl, logoutUrl, samlClient.requiresClientSignature(), 
-                samlClient.requiresClientSignature(), samlClient.requiresAssertionSignature(), samlClient.requiresEncryption(),
+                samlClient.requiresAssertionSignature(), samlClient.requiresEncryption(),
                 client.getClientId(), nameIdFormat, spCertificate, encCertificate);
     }
 
@@ -110,6 +126,6 @@ public class SamlSPDescriptorClientInstallation implements ClientInstallationPro
 
     @Override
     public String getId() {
-        return "saml-sp-descriptor";
+        return SAML_CLIENT_INSTALATION_SP_DESCRIPTOR;
     }
 }

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ServerResourceUpdater.java

@@ -69,10 +69,9 @@ public abstract class ServerResourceUpdater<T extends ServerResourceUpdater, Res
 
     @Override
     public void close() throws IOException {
-        if (! this.updated) {
+        if (this.updated) {
-            throw new IOException("Attempt to revert changes that were never applied - have you called " + this.getClass().getName() + ".update()?");
+            performUpdate(rep, origRep);
         }
-        performUpdate(rep, origRep);
     }
 
     /**

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/AbstractClientTest.java

@@ -126,7 +126,6 @@ public abstract class AbstractClientTest extends AbstractAuthTest {
         clientRep.setClientId(name);
         clientRep.setName(name);
         clientRep.setProtocol("saml");
-        clientRep.setAdminUrl("samlEndpoint");
         return createClient(clientRep);
     }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/InstallationTest.java

@@ -17,15 +17,32 @@
 
 package org.keycloak.testsuite.admin.client;
 
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.HashMap;
+import java.util.Map;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.keycloak.admin.client.resource.ClientResource;
 import org.keycloak.events.admin.OperationType;
 import org.keycloak.events.admin.ResourceType;
+import org.keycloak.protocol.saml.SamlConfigAttributes;
+import org.keycloak.protocol.saml.SamlProtocol;
+import org.keycloak.protocol.saml.installation.SamlSPDescriptorClientInstallation;
+import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
 import org.keycloak.testsuite.admin.ApiUtil;
 import org.keycloak.testsuite.arquillian.AuthServerTestEnricher;
+import org.keycloak.testsuite.updaters.ClientAttributeUpdater;
 import org.keycloak.testsuite.util.AdminEventPaths;
+import org.w3c.dom.Document;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
 
 import javax.ws.rs.NotFoundException;
 import static org.junit.Assert.assertThat;
@@ -156,7 +173,7 @@ public class InstallationTest extends AbstractClientTest {
 
     @Test
     public void testSamlMetadataSpDescriptor() {
-        String xml = samlClient.getInstallationProvider("saml-sp-descriptor");
+        String xml = samlClient.getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR);
         assertThat(xml, containsString("<EntityDescriptor"));
         assertThat(xml, containsString("<SPSSODescriptor"));
         assertThat(xml, containsString(SAML_NAME));
@@ -170,4 +187,113 @@ public class InstallationTest extends AbstractClientTest {
         assertThat(xml, not(containsString(ApiUtil.findActiveKey(testRealmResource()).getCertificate())));
         assertThat(xml, containsString(samlUrl()));
     }
+
+    @Test
+    public void testSamlMetadataSpDescriptorPost() throws Exception {
+        try (ClientAttributeUpdater updater = ClientAttributeUpdater.forClient(adminClient, getRealmId(), SAML_NAME)) {
+
+            assertThat(updater.getResource().toRepresentation().getAttributes().get(SamlConfigAttributes.SAML_FORCE_POST_BINDING), equalTo("true"));
+
+            //error fallback
+            Document doc = getDocumentFromXmlString(updater.getResource().getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR));
+            Map<String, String> attrNamesAndValues = new HashMap<>();
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get());
+            attrNamesAndValues.put("Location", "ERROR:ENDPOINT NOT SET");
+            assertElements(doc, "SingleLogoutService", attrNamesAndValues);
+            assertElements(doc, "AssertionConsumerService", attrNamesAndValues);
+            attrNamesAndValues.clear();
+
+            //fallback to adminUrl
+            updater.setAdminUrl("admin-url").update();
+            assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+            doc = getDocumentFromXmlString(updater.getResource().getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR));
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get());
+            attrNamesAndValues.put("Location", "admin-url");
+            assertElements(doc, "SingleLogoutService", attrNamesAndValues);
+            assertElements(doc, "AssertionConsumerService", attrNamesAndValues);
+            attrNamesAndValues.clear();
+
+            //fine grained
+            updater.setAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_POST_ATTRIBUTE, "saml-assertion-post-url")
+                   .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE, "saml-logout-post-url")
+                   .setAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_REDIRECT_ATTRIBUTE, "saml-assertion-redirect-url")
+                   .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_REDIRECT_ATTRIBUTE, "saml-logout-redirect-url")
+                   .update();
+            assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+
+            doc = getDocumentFromXmlString(updater.getResource().getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR));
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get());
+            attrNamesAndValues.put("Location", "saml-logout-post-url");
+            assertElements(doc, "SingleLogoutService", attrNamesAndValues);
+            attrNamesAndValues.clear();
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get());
+            attrNamesAndValues.put("Location", "saml-assertion-post-url");
+            assertElements(doc, "AssertionConsumerService", attrNamesAndValues);
+        }
+        assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+    }
+
+    @Test
+    public void testSamlMetadataSpDescriptorRedirect() throws Exception {
+        try (ClientAttributeUpdater updater = ClientAttributeUpdater.forClient(adminClient, getRealmId(), SAML_NAME)
+                .setAttribute(SamlConfigAttributes.SAML_FORCE_POST_BINDING, "false")
+                .update()) {
+
+            assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+            assertThat(updater.getResource().toRepresentation().getAttributes().get(SamlConfigAttributes.SAML_FORCE_POST_BINDING), equalTo("false"));
+
+            //error fallback
+            Document doc = getDocumentFromXmlString(updater.getResource().getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR));
+            Map<String, String> attrNamesAndValues = new HashMap<>();
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get());
+            attrNamesAndValues.put("Location", "ERROR:ENDPOINT NOT SET");
+            assertElements(doc, "SingleLogoutService", attrNamesAndValues);
+            assertElements(doc, "AssertionConsumerService", attrNamesAndValues);
+            attrNamesAndValues.clear();
+
+            //fallback to adminUrl
+            updater.setAdminUrl("admin-url").update();
+            assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+            doc = getDocumentFromXmlString(updater.getResource().getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR));
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get());
+            attrNamesAndValues.put("Location", "admin-url");
+            assertElements(doc, "SingleLogoutService", attrNamesAndValues);
+            assertElements(doc, "AssertionConsumerService", attrNamesAndValues);
+            attrNamesAndValues.clear();
+
+            //fine grained
+            updater.setAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_POST_ATTRIBUTE, "saml-assertion-post-url")
+                   .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE, "saml-logout-post-url")
+                   .setAttribute(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_REDIRECT_ATTRIBUTE, "saml-assertion-redirect-url")
+                   .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_REDIRECT_ATTRIBUTE, "saml-logout-redirect-url")
+                   .update();
+            assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+            doc = getDocumentFromXmlString(updater.getResource().getInstallationProvider(SamlSPDescriptorClientInstallation.SAML_CLIENT_INSTALATION_SP_DESCRIPTOR));
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get());
+            attrNamesAndValues.put("Location", "saml-logout-redirect-url");
+            assertElements(doc, "SingleLogoutService", attrNamesAndValues);
+            attrNamesAndValues.clear();
+            attrNamesAndValues.put("Binding", JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get());
+            attrNamesAndValues.put("Location", "saml-assertion-redirect-url");
+            assertElements(doc, "AssertionConsumerService", attrNamesAndValues);
+        }
+        assertAdminEvents.assertEvent(getRealmId(), OperationType.UPDATE, AdminEventPaths.clientResourcePath(samlClientId), ResourceType.CLIENT);
+    }
+
+    private Document getDocumentFromXmlString(String xml) throws SAXException, ParserConfigurationException, IOException {
+        DocumentBuilder db = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+        InputSource is = new InputSource();
+        is.setCharacterStream(new StringReader(xml));
+        return db.parse(is);
+    }
+
+    private void assertElements(Document doc, String tagName, Map<String, String> attrNamesAndValues) {
+        NodeList elementsByTagName = doc.getElementsByTagName(tagName);
+        assertThat("Expected exactly one " + tagName + " element!", elementsByTagName.getLength(), is(equalTo(1)));
+        Node element = elementsByTagName.item(0);
+
+        for (String attrName : attrNamesAndValues.keySet()) {
+            assertThat(element.getAttributes().getNamedItem(attrName).getNodeValue(), containsString(attrNamesAndValues.get(attrName)));
+        }
+    }
 }