KEYCLOAK-14741 Minor SAML specs compliance improvements

saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLRequestWriter.java

@@ -39,6 +39,7 @@ import java.util.List;
 import org.keycloak.dom.saml.v2.protocol.ExtensionsType;
 
 import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.ASSERTION_NSURI;
+import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.NAMEID_FORMAT_TRANSIENT;
 import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.PROTOCOL_NSURI;
 
 /**
@@ -89,7 +90,10 @@ public class SAMLRequestWriter extends BaseWriter {
         }
 
         Boolean isPassive = request.isIsPassive();
-        if (isPassive != null) {
+        // The AuthnRequest IsPassive attribute is optional and if omitted its default value is false. 
+        // Some IdPs refuse requests if the IsPassive attribute is present and set to false, so to 
+        // maximize compatibility we emit it only if it is set to true
+        if (isPassive != null && isPassive == true) {
             StaxUtil.writeAttribute(writer, JBossSAMLConstants.IS_PASSIVE.get(), isPassive.toString());
         }
 
@@ -223,7 +227,8 @@ public class SAMLRequestWriter extends BaseWriter {
         }
 
         Boolean allowCreate = nameIDPolicy.isAllowCreate();
-        if (allowCreate != null) {
+        // The NameID AllowCreate attribute must not be used when using the transient NameID format.
+        if (allowCreate != null && (format == null || !NAMEID_FORMAT_TRANSIENT.get().equals(format.toASCIIString()))) {
             StaxUtil.writeAttribute(writer, JBossSAMLConstants.ALLOW_CREATE.get(), allowCreate.toString());
         }
 

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

@@ -9,6 +9,7 @@ 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;
+import org.keycloak.saml.common.util.DocumentUtil;
 import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request;
 import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
 import org.keycloak.saml.processing.web.util.RedirectBindingUtil;
@@ -39,11 +40,18 @@ import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.util.EntityUtils;
 import org.hamcrest.Matcher;
 import org.jboss.resteasy.util.Encode;
+import org.w3c.dom.Attr;
 import org.w3c.dom.Document;
+import org.w3c.dom.Element;
 
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.junit.Assert.assertThat;
+import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.NAMEID_FORMAT_TRANSIENT;
+import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.PROTOCOL_NSURI;
 import static org.keycloak.testsuite.util.ServerURLs.AUTH_SERVER_PORT;
 import static org.keycloak.testsuite.utils.io.IOUtil.documentToString;
 import static org.keycloak.testsuite.utils.io.IOUtil.setDocElementAttributeValue;
@@ -241,4 +249,62 @@ public class BasicSamlTest extends AbstractSamlTest {
 
         samlClient.execute(secondAuthn);
     }
+
+    @Test
+    public void testIsPassiveAttributeEmittedWhenTrue() throws Exception {
+        // Verifies that the IsPassive attribute is emitted in the authnRequest
+        // when it is set to true
+
+        // Build the login request document
+        AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, getAuthServerSamlEndpoint(REALM_NAME));
+        loginRep.setIsPassive(true);
+
+        Document document = SAML2Request.convert(loginRep);
+
+        // Find the AuthnRequest element
+        Element authnRequestElement = document.getDocumentElement();
+        Attr isPassiveAttribute = authnRequestElement.getAttributeNode("IsPassive");
+        assertThat("AuthnRequest element should contain the IsPassive attribute when isPassive is true, but it doesn't", isPassiveAttribute, notNullValue());
+        assertThat("AuthnRequest/IsPassive attribute should be true when isPassive is true, but it isn't", isPassiveAttribute.getNodeValue(), is("true"));
+    }
+
+    @Test
+    public void testIsPassiveAttributeOmittedWhenFalse() throws Exception {
+        // Verifies that the IsPassive attribute is not emitted in the authnRequest
+        // when it is set to false
+
+        // Build the login request document
+        AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, getAuthServerSamlEndpoint(REALM_NAME));
+        loginRep.setIsPassive(false);
+
+        Document document = SAML2Request.convert(loginRep);
+
+        // Find the AuthnRequest element
+        Element authnRequestElement = document.getDocumentElement();
+        Attr isPassiveAttribute = authnRequestElement.getAttributeNode("IsPassive");
+        assertThat("AuthnRequest element shouldn't contain the IsPassive attribute when isPassive is false, but it does", isPassiveAttribute, nullValue());
+    }
+
+    @Test
+    public void testAllowCreateAttributeOmittedWhenTransient() throws Exception {
+        // Verifies that the AllowCreate attribute is not emitted in the AuthnRequest
+        // when NameIDFormat is Transient
+
+        // Build the login request document
+        AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, getAuthServerSamlEndpoint(REALM_NAME));
+        loginRep.getNameIDPolicy().setFormat(NAMEID_FORMAT_TRANSIENT.getUri());
+        loginRep.getNameIDPolicy().setAllowCreate(true);
+
+        Document document = SAML2Request.convert(loginRep);
+
+        // Find the AuthnRequest element
+        Element authnRequestElement = document.getDocumentElement();
+        Element nameIdPolicyElement = DocumentUtil.getDirectChildElement(authnRequestElement, PROTOCOL_NSURI.get(), "NameIDPolicy");
+
+        Attr formatAttribute = nameIdPolicyElement.getAttributeNode("Format");
+        Attr allowCreateAttribute = nameIdPolicyElement.getAttributeNode("AllowCreate");
+        assertThat("AuthnRequest/NameIdPolicy Format should be present, but it is not", formatAttribute, notNullValue());
+        assertThat("AuthnRequest/NameIdPolicy Format should be Transient, but it is not", formatAttribute.getNodeValue(), is(NAMEID_FORMAT_TRANSIENT.get()));
+        assertThat("AuthnRequest/NameIdPolicy element shouldn't contain the AllowCreate attribute when Format is set to Transient, but it does", allowCreateAttribute, nullValue());
+    }
 }