From 773e309f7539e06889b2583ff6e8775575038501 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 31 Jul 2024 13:25:22 +0200 Subject: [PATCH] Parse saml urls correctly if the bindings are different Closes #31780 Signed-off-by: rmartinc --- .../saml/SAMLIdentityProviderFactory.java | 7 +-- .../testsuite/admin/IdentityProviderTest.java | 55 ++++++++++++------- .../saml-idp-metadata-different-bindings.xml | 39 +++++++++++++ 3 files changed, 76 insertions(+), 25 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/resources/admin-test/saml-idp-metadata-different-bindings.xml diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java index b2e6c60090..66d7b500cb 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java @@ -77,7 +77,6 @@ public class SAMLIdentityProviderFactory extends AbstractIdentityProviderFactory SAMLIdentityProviderConfig samlIdentityProviderConfig = new SAMLIdentityProviderConfig(); String singleSignOnServiceUrl = null; boolean postBindingResponse = false; - boolean postBindingLogout = false; for (EndpointType endpoint : idpDescriptor.getSingleSignOnService()) { if (endpoint.getBinding().toString().equals(JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get())) { singleSignOnServiceUrl = endpoint.getLocation().toString(); @@ -88,14 +87,14 @@ public class SAMLIdentityProviderFactory extends AbstractIdentityProviderFactory } } String singleLogoutServiceUrl = null; + boolean postBindingLogout = false; for (EndpointType endpoint : idpDescriptor.getSingleLogoutService()) { - if (postBindingResponse && endpoint.getBinding().toString().equals(JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get())) { + if (endpoint.getBinding().toString().equals(JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.get())) { singleLogoutServiceUrl = endpoint.getLocation().toString(); postBindingLogout = true; break; - } else if (!postBindingResponse && endpoint.getBinding().toString().equals(JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get())) { + } else if (endpoint.getBinding().toString().equals(JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.get())) { singleLogoutServiceUrl = endpoint.getLocation().toString(); - break; } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java index fb8d0c4e24..b38371ab04 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java @@ -85,6 +85,7 @@ import org.keycloak.representations.idm.IdentityProviderMapperRepresentation; import org.keycloak.representations.idm.IdentityProviderMapperTypeRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.rotation.HardcodedKeyLocator; +import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.exceptions.ConfigurationException; import org.keycloak.saml.common.exceptions.ParsingException; import org.keycloak.saml.common.exceptions.ProcessingException; @@ -99,7 +100,6 @@ import org.keycloak.testsuite.broker.oidc.OverwrittenMappersTestIdentityProvider import org.keycloak.testsuite.updaters.RealmAttributeUpdater; import org.keycloak.testsuite.util.AdminEventPaths; import org.keycloak.testsuite.util.KeyUtils; -import org.keycloak.utils.ReservedCharValidator.ReservedCharException; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NodeList; @@ -717,7 +717,7 @@ public class IdentityProviderTest extends AbstractAdminTest { @Test public void testSamlImportAndExport() throws URISyntaxException, IOException, ParsingException { - testSamlImport("saml-idp-metadata.xml"); + testSamlImport("saml-idp-metadata.xml", true); // Perform export, and make sure some of the values are like they're supposed to be Response response = realm.identityProviders().get("saml").export("xml"); @@ -725,15 +725,27 @@ public class IdentityProviderTest extends AbstractAdminTest { String body = response.readEntity(String.class); response.close(); - assertSamlExport(body); + assertSamlExport(body, true); + } + + @Test + public void testSamlImportAndExportDifferentBindings() throws URISyntaxException, IOException, ParsingException { + testSamlImport("saml-idp-metadata-different-bindings.xml", false); + + // Perform export, and make sure some of the values are like they're supposed to be + try (Response response = realm.identityProviders().get("saml").export("xml")) { + Assert.assertEquals(200, response.getStatus()); + String body = response.readEntity(String.class); + assertSamlExport(body, false); + } } @Test public void testSamlImportWithAnyEncryptionMethod() throws URISyntaxException, IOException, ParsingException { - testSamlImport("saml-idp-metadata-encryption-methods.xml"); + testSamlImport("saml-idp-metadata-encryption-methods.xml", true); } - private void testSamlImport(String fileName) throws URISyntaxException, IOException, ParsingException { + private void testSamlImport(String fileName, boolean postBindingResponse) throws URISyntaxException, IOException, ParsingException { // Use import-config to convert IDPSSODescriptor file into key value pairs // to use when creating a SAML Identity Provider MultipartFormDataOutput form = new MultipartFormDataOutput(); @@ -745,14 +757,14 @@ public class IdentityProviderTest extends AbstractAdminTest { form.addFormData("file", body, MediaType.APPLICATION_XML_TYPE, fileName); Map result = realm.identityProviders().importFrom(form); - assertSamlImport(result, SIGNING_CERT_1,true); + assertSamlImport(result, SIGNING_CERT_1, true, postBindingResponse); // Create new SAML identity provider using configuration retrieved from import-config create(createRep("saml", "saml",true, result)); IdentityProviderResource provider = realm.identityProviders().get("saml"); IdentityProviderRepresentation rep = provider.toRepresentation(); - assertCreatedSamlIdp(rep,true); + assertCreatedSamlIdp(rep, true, postBindingResponse); // Now list the providers - we should see the one just created List providers = realm.identityProviders().findAll(); @@ -776,14 +788,14 @@ public class IdentityProviderTest extends AbstractAdminTest { form.addFormData("file", body, MediaType.APPLICATION_XML_TYPE, "saml-idp-metadata-disabled.xml"); Map result = realm.identityProviders().importFrom(form); - assertSamlImport(result, SIGNING_CERT_1, false); + assertSamlImport(result, SIGNING_CERT_1, false, true); // Create new SAML identity provider using configuration retrieved from import-config create(createRep("saml", "saml", false, result)); IdentityProviderResource provider = realm.identityProviders().get("saml"); IdentityProviderRepresentation rep = provider.toRepresentation(); - assertCreatedSamlIdp(rep, false); + assertCreatedSamlIdp(rep, false, true); } @@ -802,14 +814,14 @@ public class IdentityProviderTest extends AbstractAdminTest { form.addFormData("file", body, MediaType.APPLICATION_XML_TYPE, "saml-idp-metadata-two-signing-certs"); Map result = realm.identityProviders().importFrom(form); - assertSamlImport(result, SIGNING_CERT_1 + "," + SIGNING_CERT_2,true); + assertSamlImport(result, SIGNING_CERT_1 + "," + SIGNING_CERT_2, true, true); // Create new SAML identity provider using configuration retrieved from import-config create(createRep("saml", "saml",true, result)); IdentityProviderResource provider = realm.identityProviders().get("saml"); IdentityProviderRepresentation rep = provider.toRepresentation(); - assertCreatedSamlIdp(rep,true); + assertCreatedSamlIdp(rep, true, true); // Now list the providers - we should see the one just created List providers = realm.identityProviders().findAll(); @@ -823,7 +835,7 @@ public class IdentityProviderTest extends AbstractAdminTest { body = response.readEntity(String.class); response.close(); - assertSamlExport(body); + assertSamlExport(body, true); } @Test @@ -1024,7 +1036,7 @@ public class IdentityProviderTest extends AbstractAdminTest { Assert.assertEquals("config", expected.getConfig(), actual.getConfig()); } - private void assertCreatedSamlIdp(IdentityProviderRepresentation idp,boolean enabled) { + private void assertCreatedSamlIdp(IdentityProviderRepresentation idp, boolean enabled, boolean postBindingResponse) { //System.out.println("idp: " + idp); Assert.assertNotNull("IdentityProviderRepresentation not null", idp); Assert.assertNotNull("internalId", idp.getInternalId()); @@ -1032,10 +1044,10 @@ public class IdentityProviderTest extends AbstractAdminTest { Assert.assertEquals("providerId", "saml", idp.getProviderId()); Assert.assertEquals("enabled",enabled, idp.isEnabled()); Assert.assertNull("firstBrokerLoginFlowAlias", idp.getFirstBrokerLoginFlowAlias()); - assertSamlConfig(idp.getConfig()); + assertSamlConfig(idp.getConfig(), postBindingResponse); } - private void assertSamlConfig(Map config) { + private void assertSamlConfig(Map config, boolean postBindingResponse) { // import endpoint simply converts IDPSSODescriptor into key value pairs. // check that saml-idp-metadata.xml was properly converted into key value pairs //System.out.println(config); @@ -1060,9 +1072,9 @@ public class IdentityProviderTest extends AbstractAdminTest { assertThat(config, hasEntry("validateSignature", "true")); assertThat(config, hasEntry("singleLogoutServiceUrl", "http://localhost:8080/auth/realms/master/protocol/saml")); assertThat(config, hasEntry("artifactResolutionServiceUrl", "http://localhost:8080/auth/realms/master/protocol/saml/resolve")); - assertThat(config, hasEntry("postBindingResponse", "true")); + assertThat(config, hasEntry("postBindingResponse", Boolean.toString(postBindingResponse))); assertThat(config, hasEntry("artifactBindingResponse", "false")); - assertThat(config, hasEntry("postBindingAuthnRequest", "true")); + assertThat(config, hasEntry("postBindingAuthnRequest", Boolean.toString(postBindingResponse))); assertThat(config, hasEntry("singleSignOnServiceUrl", "http://localhost:8080/auth/realms/master/protocol/saml")); assertThat(config, hasEntry("wantAuthnRequestsSigned", "true")); assertThat(config, hasEntry("addExtensionsElementWithKeyInfo", "false")); @@ -1072,16 +1084,16 @@ public class IdentityProviderTest extends AbstractAdminTest { assertThat(config, hasEntry(is("signingCertificate"), notNullValue())); } - private void assertSamlImport(Map config, String expectedSigningCertificates,boolean enabled) { + private void assertSamlImport(Map config, String expectedSigningCertificates, boolean enabled, boolean postBindingResponse) { //firtsly check and remove enabledFromMetadata from config boolean enabledFromMetadata = Boolean.valueOf(config.get(SAMLIdentityProviderConfig.ENABLED_FROM_METADATA)); config.remove(SAMLIdentityProviderConfig.ENABLED_FROM_METADATA); Assert.assertEquals(enabledFromMetadata,enabled); - assertSamlConfig(config); + assertSamlConfig(config, postBindingResponse); assertThat(config, hasEntry("signingCertificate", expectedSigningCertificates)); } - private void assertSamlExport(String body) throws ParsingException, URISyntaxException { + private void assertSamlExport(String body, boolean postBindingResponse) throws ParsingException, URISyntaxException { //System.out.println(body); Object entBody = SAMLParser.getInstance().parse( @@ -1119,7 +1131,8 @@ public class IdentityProviderTest extends AbstractAdminTest { Assert.assertEquals("AssertionConsumerService.Location", new URI(oauth.AUTH_SERVER_ROOT + "/realms/admin-client-test/broker/saml/endpoint"), endpoint.getLocation()); Assert.assertEquals("AssertionConsumerService.Binding", - new URI("urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"), endpoint.getBinding()); + postBindingResponse ? JBossSAMLURIConstants.SAML_HTTP_POST_BINDING.getUri() : JBossSAMLURIConstants.SAML_HTTP_REDIRECT_BINDING.getUri(), + endpoint.getBinding()); Assert.assertTrue("AssertionConsumerService.isDefault", endpoint.isIsDefault()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/admin-test/saml-idp-metadata-different-bindings.xml b/testsuite/integration-arquillian/tests/base/src/test/resources/admin-test/saml-idp-metadata-different-bindings.xml new file mode 100644 index 0000000000..93d555bfb7 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/admin-test/saml-idp-metadata-different-bindings.xml @@ -0,0 +1,39 @@ + + + + + + http://refeds.org/category/hide-from-discovery + + + + + + + + + MIICmzCCAYMCBgFUYnC0OjANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMTYwNDI5MTQzMjEzWhcNMjYwNDI5MTQzMzUzWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCN25AW1poMEZRbuMAHG58AThZmCwMV6/Gcui4mjGacRFyudgqzLjQ2rxpoW41JAtLjbjeAhuWvirUcFVcOeS3gM/ZC27qCpYighAcylZz6MYocnEe1+e8rPPk4JlID6Wv62dgu+pL/vYsQpRhvD3Y2c/ytgr5D32xF+KnzDehUy5BSyzypvu12Wq9mS5vK5tzkN37EjkhpY2ZxaXPubjDIITCAL4Q8M/m5IlacBaUZbzI4AQrHnMP1O1IH2dHSWuMiBe+xSDTco72PmuYPJKTV4wQdeBUIkYbfLc4RxVmXEvgkQgyW86EoMPxlWJpj7+mTIR+l+2thZPr/VgwTs82rAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAA/Ip/Hi8RoVu5ouaFFlc5whT7ltuK8slfLGW4tM4vJXhInYwsqIRQKBNDYW/64xle3eII4u1yAH1OYRRwEs7Em1pr4QuFuTY1at+aE0sE46XDlyESI0txJjWxYoT133vM0We2pj1b2nxgU30rwjKA3whnKEfTEYT/n3JBSqNggy6l8ZGw/oPSgvPaR4+xeB1tfQFC4VrLoYKoqH6hAL530nKxL+qV8AIfL64NDEE8ankIAEDAAFe8x3CPUfXR/p4KOANKkpz8ieQaHDb1eITkAwUwjESj6UF9D1aePlhWls/HX0gujFXtWfWfrJ8CU/ogwlH8y1jgRuLjFQYZk6llc= + + + + + + urn:oasis:names:tc:SAML:2.0:nameid-format:persistent + urn:oasis:names:tc:SAML:2.0:nameid-format:transient + urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified + urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress + + + +