KEYCLOAK-6474 Fix NPE on SAML logout

saml-core/src/main/java/org/keycloak/saml/SAML2LogoutResponseBuilder.java

@@ -67,32 +67,38 @@ public class SAML2LogoutResponseBuilder implements SamlProtocolExtensionsAwareBu
         return this;
     }
 
+    public StatusResponseType buildModel() throws ConfigurationException {
+        StatusResponseType statusResponse = new StatusResponseType(IDGenerator.create("ID_"), XMLTimeUtil.getIssueInstant());
+
+        // Status
+        StatusType statusType = new StatusType();
+        StatusCodeType statusCodeType = new StatusCodeType();
+        statusCodeType.setValue(URI.create(JBossSAMLURIConstants.STATUS_SUCCESS.get()));
+        statusType.setStatusCode(statusCodeType);
+
+        statusResponse.setStatus(statusType);
+        statusResponse.setInResponseTo(logoutRequestID);
+        NameIDType issuer = new NameIDType();
+        issuer.setValue(this.issuer);
+
+        statusResponse.setIssuer(issuer);
+        statusResponse.setDestination(destination);
+
+        if (! this.extensions.isEmpty()) {
+            ExtensionsType extensionsType = new ExtensionsType();
+            for (NodeGenerator extension : this.extensions) {
+                extensionsType.addExtension(extension);
+            }
+            statusResponse.setExtensions(extensionsType);
+        }
+
+        return statusResponse;
+    }
+
     public Document buildDocument() throws ProcessingException {
         Document samlResponse = null;
         try {
-            StatusResponseType statusResponse = new StatusResponseType(IDGenerator.create("ID_"), XMLTimeUtil.getIssueInstant());
+            StatusResponseType statusResponse = buildModel();
-
-            // Status
-            StatusType statusType = new StatusType();
-            StatusCodeType statusCodeType = new StatusCodeType();
-            statusCodeType.setValue(URI.create(JBossSAMLURIConstants.STATUS_SUCCESS.get()));
-            statusType.setStatusCode(statusCodeType);
-
-            statusResponse.setStatus(statusType);
-            statusResponse.setInResponseTo(logoutRequestID);
-            NameIDType issuer = new NameIDType();
-            issuer.setValue(this.issuer);
-
-            statusResponse.setIssuer(issuer);
-            statusResponse.setDestination(destination);
-
-            if (! this.extensions.isEmpty()) {
-                ExtensionsType extensionsType = new ExtensionsType();
-                for (NodeGenerator extension : this.extensions) {
-                    extensionsType.addExtension(extension);
-                }
-                statusResponse.setExtensions(extensionsType);
-            }
 
             SAML2Response saml2Response = new SAML2Response();
             samlResponse = saml2Response.convert(statusResponse);

server-spi-private/src/main/java/org/keycloak/events/EventBuilder.java

@@ -80,7 +80,7 @@ public class EventBuilder {
     }
 
     public EventBuilder realm(RealmModel realm) {
-        event.setRealmId(realm.getId());
+        event.setRealmId(realm == null ? null : realm.getId());
         return this;
     }
 
@@ -90,7 +90,7 @@ public class EventBuilder {
     }
 
     public EventBuilder client(ClientModel client) {
-        event.setClientId(client.getClientId());
+        event.setClientId(client == null ? null : client.getClientId());
         return this;
     }
 
@@ -100,7 +100,7 @@ public class EventBuilder {
     }
 
     public EventBuilder user(UserModel user) {
-        event.setUserId(user.getId());
+        event.setUserId(user == null ? null : user.getId());
         return this;
     }
 
@@ -110,7 +110,7 @@ public class EventBuilder {
     }
 
     public EventBuilder session(UserSessionModel session) {
-        event.setSessionId(session.getId());
+        event.setSessionId(session == null ? null : session.getId());
         return this;
     }
 

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

@@ -0,0 +1,59 @@
+/*
+ * Copyright 2018 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.keycloak.testsuite.updaters;
+
+import org.keycloak.admin.client.resource.IdentityProvidersResource;
+import java.io.Closeable;
+import javax.ws.rs.NotFoundException;
+import org.keycloak.admin.client.resource.RealmResource;
+import org.keycloak.representations.idm.IdentityProviderRepresentation;
+import java.io.IOException;
+import javax.ws.rs.core.Response;
+
+/**
+ *  Creates a temporary realm and makes sure it is removed.
+ */
+public class IdentityProviderCreator implements Closeable {
+
+    private final IdentityProvidersResource resource;
+    private final String alias;
+
+    public IdentityProviderCreator(RealmResource realmResource, IdentityProviderRepresentation rep) {
+        resource = realmResource.identityProviders();
+        alias = rep.getAlias();
+        Response response = null;
+        try {
+            response = resource.create(rep);
+        } finally {
+            if (response != null)
+                response.close();
+        }
+    }
+
+    public IdentityProvidersResource resource() {
+        return this.resource;
+    }
+
+    @Override
+    public void close() throws IOException {
+        try {
+            resource.get(alias).remove();
+        } catch (NotFoundException e) {
+            // ignore
+        }
+    }
+}

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

@@ -4,6 +4,7 @@ import org.keycloak.admin.client.resource.RealmResource;
 import org.keycloak.representations.idm.RealmRepresentation;
 import java.io.Closeable;
 import java.util.HashMap;
+import java.util.function.Consumer;
 
 /**
  *
@@ -25,6 +26,11 @@ public class RealmAttributeUpdater {
         }
     }
 
+    public RealmAttributeUpdater updateWith(Consumer<RealmRepresentation> updater) {
+        updater.accept(this.rep);
+        return this;
+    }
+
     public RealmAttributeUpdater setAttribute(String name, String value) {
         this.rep.getAttributes().put(name, value);
         return this;

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/saml/SamlDocumentStepBuilder.java

@@ -24,7 +24,9 @@ import org.keycloak.dom.saml.v2.protocol.AttributeQueryType;
 import org.keycloak.dom.saml.v2.protocol.AuthnRequestType;
 import org.keycloak.dom.saml.v2.protocol.LogoutRequestType;
 import org.keycloak.dom.saml.v2.protocol.ResponseType;
+import org.keycloak.dom.saml.v2.protocol.StatusResponseType;
 import org.keycloak.saml.common.constants.GeneralConstants;
+import org.keycloak.saml.common.constants.JBossSAMLConstants;
 import org.keycloak.saml.common.util.DocumentUtil;
 import org.keycloak.saml.common.util.StaxUtil;
 import org.keycloak.saml.processing.core.parsers.saml.SAMLParser;
@@ -33,9 +35,11 @@ import org.keycloak.saml.processing.core.saml.v2.writers.SAMLResponseWriter;
 import org.keycloak.testsuite.util.SamlClient.Step;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import javax.xml.namespace.QName;
 import javax.xml.stream.XMLStreamWriter;
 import org.junit.Assert;
 import org.w3c.dom.Document;
+import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.PROTOCOL_NSURI;
 
 /**
  *
@@ -87,21 +91,23 @@ public abstract class SamlDocumentStepBuilder<T extends SAML2Object, This extend
             ByteArrayOutputStream bos = new ByteArrayOutputStream();
             XMLStreamWriter xmlStreamWriter = StaxUtil.getXMLStreamWriter(bos);
 
-            if (saml2Object instanceof AuthnRequestType) {
+            if (transformed instanceof AuthnRequestType) {
-                new SAMLRequestWriter(xmlStreamWriter).write((AuthnRequestType) saml2Object);
+                new SAMLRequestWriter(xmlStreamWriter).write((AuthnRequestType) transformed);
-            } else if (saml2Object instanceof LogoutRequestType) {
+            } else if (transformed instanceof LogoutRequestType) {
-                new SAMLRequestWriter(xmlStreamWriter).write((LogoutRequestType) saml2Object);
+                new SAMLRequestWriter(xmlStreamWriter).write((LogoutRequestType) transformed);
-            } else if (saml2Object instanceof ArtifactResolveType) {
+            } else if (transformed instanceof ArtifactResolveType) {
-                new SAMLRequestWriter(xmlStreamWriter).write((ArtifactResolveType) saml2Object);
+                new SAMLRequestWriter(xmlStreamWriter).write((ArtifactResolveType) transformed);
-            } else if (saml2Object instanceof AttributeQueryType) {
+            } else if (transformed instanceof AttributeQueryType) {
-                new SAMLRequestWriter(xmlStreamWriter).write((AttributeQueryType) saml2Object);
+                new SAMLRequestWriter(xmlStreamWriter).write((AttributeQueryType) transformed);
-            } else if (saml2Object instanceof ResponseType) {
+            } else if (transformed instanceof ResponseType) {
-                new SAMLResponseWriter(xmlStreamWriter).write((ResponseType) saml2Object);
+                new SAMLResponseWriter(xmlStreamWriter).write((ResponseType) transformed);
-            } else if (saml2Object instanceof ArtifactResponseType) {
+            } else if (transformed instanceof ArtifactResponseType) {
-                new SAMLResponseWriter(xmlStreamWriter).write((ArtifactResponseType) saml2Object);
+                new SAMLResponseWriter(xmlStreamWriter).write((ArtifactResponseType) transformed);
+            } else if (transformed instanceof StatusResponseType) {
+                new SAMLResponseWriter(xmlStreamWriter).write((StatusResponseType) transformed, new QName(PROTOCOL_NSURI.get(), JBossSAMLConstants.LOGOUT_RESPONSE.get(), "samlp"));
             } else {
-                Assert.assertNotNull("Unknown type: <null>", saml2Object);
+                Assert.assertNotNull("Unknown type: <null>", transformed);
-                Assert.fail("Unknown type: " + saml2Object.getClass().getName());
+                Assert.fail("Unknown type: " + transformed.getClass().getName());
             }
             return new String(bos.toByteArray(), GeneralConstants.SAML_CHARSET);
         };

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

@@ -55,4 +55,10 @@ public abstract class AbstractSamlTest extends AbstractAuthTest {
                 .protocolUrl(UriBuilder.fromUri(getAuthServerRoot()))
                 .build(realm, SamlProtocol.LOGIN_PROTOCOL);
     }
+
+    protected URI getAuthServerRealmBase(String realm) throws IllegalArgumentException, UriBuilderException {
+        return RealmsResource
+                .realmBaseUrl(UriBuilder.fromUri(getAuthServerRoot()))
+                .build(realm);
+    }
 }

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

@@ -16,10 +16,15 @@
  */
 package org.keycloak.testsuite.saml;
 
+import org.keycloak.admin.client.resource.ClientsResource;
+import org.keycloak.admin.client.resource.RealmResource;
+import org.keycloak.broker.saml.SAMLIdentityProviderConfig;
+import org.keycloak.broker.saml.SAMLIdentityProviderFactory;
 import org.keycloak.dom.saml.v2.SAML2Object;
 import org.keycloak.dom.saml.v2.assertion.AssertionType;
 import org.keycloak.dom.saml.v2.assertion.AuthnStatementType;
 import org.keycloak.dom.saml.v2.assertion.NameIDType;
+import org.keycloak.dom.saml.v2.protocol.AuthnRequestType;
 import org.keycloak.dom.saml.v2.protocol.LogoutRequestType;
 import org.keycloak.dom.saml.v2.protocol.ResponseType;
 import org.keycloak.dom.saml.v2.protocol.StatusResponseType;
@@ -28,15 +33,26 @@ import org.keycloak.events.EventType;
 import org.keycloak.protocol.saml.SamlProtocol;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.EventRepresentation;
+import org.keycloak.representations.idm.IdentityProviderRepresentation;
+import org.keycloak.saml.SAML2LoginResponseBuilder;
 import org.keycloak.saml.SAML2LogoutResponseBuilder;
 import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
+import org.keycloak.saml.common.exceptions.ConfigurationException;
+import org.keycloak.saml.common.exceptions.ProcessingException;
 import org.keycloak.saml.processing.core.parsers.saml.SAMLParser;
 import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
+import org.keycloak.testsuite.updaters.ClientAttributeUpdater;
+import org.keycloak.testsuite.updaters.IdentityProviderCreator;
 import org.keycloak.testsuite.util.ClientBuilder;
+import org.keycloak.testsuite.util.IdentityProviderBuilder;
 import org.keycloak.testsuite.util.SamlClientBuilder;
 
+import java.io.Closeable;
+import java.io.IOException;
+import java.net.URI;
 import java.util.Arrays;
 import java.util.List;
+import java.util.UUID;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.xml.transform.dom.DOMSource;
 import org.junit.Before;
@@ -63,6 +79,8 @@ public class LogoutTest extends AbstractSamlTest {
     private final AtomicReference<NameIDType> nameIdRef = new AtomicReference<>();
     private final AtomicReference<String> sessionIndexRef = new AtomicReference<>();
 
+    private static final String SAML_BROKER_ALIAS = "saml-broker";
+
     @Before
     public void setup() {
         salesRep = adminClient.realm(REALM_NAME).clients().findByClientId(SAML_CLIENT_ID_SALES_POST).get(0);
@@ -86,25 +104,29 @@ public class LogoutTest extends AbstractSamlTest {
         return true;
     }
 
+    private SAML2Object extractNameIdAndSessionIndexAndTerminate(SAML2Object so) {
+        assertThat(so, isSamlResponse(JBossSAMLURIConstants.STATUS_SUCCESS));
+        ResponseType loginResp1 = (ResponseType) so;
+        final AssertionType firstAssertion = loginResp1.getAssertions().get(0).getAssertion();
+        assertThat(firstAssertion, org.hamcrest.Matchers.notNullValue());
+        assertThat(firstAssertion.getSubject().getSubType().getBaseID(), instanceOf(NameIDType.class));
+
+        NameIDType nameId = (NameIDType) firstAssertion.getSubject().getSubType().getBaseID();
+        AuthnStatementType firstAssertionStatement = (AuthnStatementType) firstAssertion.getStatements().iterator().next();
+
+        nameIdRef.set(nameId);
+        sessionIndexRef.set(firstAssertionStatement.getSessionIndex());
+
+        return null;
+    }
+
     private SamlClientBuilder prepareLogIntoTwoApps() {
         return new SamlClientBuilder()
           .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST).build()
           .login().user(bburkeUser).build()
-          .processSamlResponse(POST).transformObject(so -> {
+          .processSamlResponse(POST)
-            assertThat(so, isSamlResponse(JBossSAMLURIConstants.STATUS_SUCCESS));
+            .transformObject(this::extractNameIdAndSessionIndexAndTerminate)
-            ResponseType loginResp1 = (ResponseType) so;
+            .build()
-            final AssertionType firstAssertion = loginResp1.getAssertions().get(0).getAssertion();
-            assertThat(firstAssertion, org.hamcrest.Matchers.notNullValue());
-            assertThat(firstAssertion.getSubject().getSubType().getBaseID(), instanceOf(NameIDType.class));
-
-            NameIDType nameId = (NameIDType) firstAssertion.getSubject().getSubType().getBaseID();
-            AuthnStatementType firstAssertionStatement = (AuthnStatementType) firstAssertion.getStatements().iterator().next();
-
-            nameIdRef.set(nameId);
-            sessionIndexRef.set(firstAssertionStatement.getSessionIndex());
-            return null;    // Do not follow the redirect to the app from the returned response
-          }).build()
-
           .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST2, SAML_ASSERTION_CONSUMER_URL_SALES_POST2, POST).build()
           .login().sso(true).build()    // This is a formal step
           .processSamlResponse(POST).transformObject(so -> {
@@ -291,4 +313,109 @@ public class LogoutTest extends AbstractSamlTest {
         assertEquals("saml", logoutEvent.getDetails().get(Details.AUTH_METHOD));
         assertNotNull(logoutEvent.getDetails().get(SamlProtocol.SAML_LOGOUT_REQUEST_ID));
     }
+
+    private IdentityProviderRepresentation addIdentityProvider() {
+        IdentityProviderRepresentation identityProvider = IdentityProviderBuilder.create()
+          .providerId(SAMLIdentityProviderFactory.PROVIDER_ID)
+          .alias(SAML_BROKER_ALIAS)
+          .displayName("SAML")
+          .setAttribute(SAMLIdentityProviderConfig.SINGLE_SIGN_ON_SERVICE_URL, "http://saml.idp/saml")
+          .setAttribute(SAMLIdentityProviderConfig.SINGLE_LOGOUT_SERVICE_URL, "http://saml.idp/saml")
+          .setAttribute(SAMLIdentityProviderConfig.NAME_ID_POLICY_FORMAT, "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress")
+          .setAttribute(SAMLIdentityProviderConfig.POST_BINDING_RESPONSE, "false")
+          .setAttribute(SAMLIdentityProviderConfig.POST_BINDING_AUTHN_REQUEST, "false")
+          .setAttribute(SAMLIdentityProviderConfig.BACKCHANNEL_SUPPORTED, "false")
+          .build();
+        return identityProvider;
+    }
+
+    private SAML2Object createAuthnResponse(SAML2Object so) {
+        AuthnRequestType req = (AuthnRequestType) so;
+        try {
+            return new SAML2LoginResponseBuilder()
+              .requestID(req.getID())
+              .destination(req.getAssertionConsumerServiceURL().toString())
+              .issuer("http://saml.idp/saml")
+              .assertionExpiration(1000000)
+              .subjectExpiration(1000000)
+              .requestIssuer(getAuthServerRealmBase(REALM_NAME).toString())
+              .nameIdentifier(JBossSAMLURIConstants.NAMEID_FORMAT_EMAIL.get(), "a@b.c")
+              .authMethod(JBossSAMLURIConstants.AC_UNSPECIFIED.get())
+              .sessionIndex("idp:" + UUID.randomUUID())
+              .buildModel();
+        } catch (ConfigurationException | ProcessingException ex) {
+            throw new RuntimeException(ex);
+        }
+    }
+
+    private SAML2Object createIdPLogoutResponse(SAML2Object so) {
+        LogoutRequestType req = (LogoutRequestType) so;
+        try {
+            return new SAML2LogoutResponseBuilder()
+              .logoutRequestID(req.getID())
+              .destination(getSamlBrokerUrl(REALM_NAME).toString())
+              .issuer("http://saml.idp/saml")
+              .buildModel();
+        } catch (ConfigurationException ex) {
+            throw new RuntimeException(ex);
+        }
+    }
+
+    @Test
+    public void testLogoutPropagatesToSamlIdentityProvider() throws IOException {
+        final RealmResource realm = adminClient.realm(REALM_NAME);
+        final ClientsResource clients = realm.clients();
+
+        try (
+          Closeable sales = new ClientAttributeUpdater(clients.get(salesRep.getId()))
+          .setFrontchannelLogout(true)
+          .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE, "")
+          .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_REDIRECT_ATTRIBUTE, "http://url")
+          .update();
+
+          Closeable idp = new IdentityProviderCreator(realm, addIdentityProvider())
+          ) {
+            SAMLDocumentHolder samlResponse = new SamlClientBuilder()
+              .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST).build()
+
+              // Virtually perform login at IdP (return artificial SAML response)
+              .login().idp(SAML_BROKER_ALIAS).build()
+              .processSamlResponse(REDIRECT)
+                .transformObject(this::createAuthnResponse)
+                .targetAttributeSamlResponse()
+                .targetUri(getSamlBrokerUrl(REALM_NAME))
+                .build()
+              .updateProfile().username("a").email("a@b.c").firstName("A").lastName("B").build()
+              .followOneRedirect()
+
+              // Now returning back to the app
+              .processSamlResponse(POST)
+                .transformObject(this::extractNameIdAndSessionIndexAndTerminate)
+                .build()
+
+              // ----- Logout phase ------
+
+              // Logout initiated from the app
+              .logoutRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, REDIRECT)
+                .nameId(nameIdRef::get)
+                .sessionIndex(sessionIndexRef::get)
+                .build()
+
+              // Should redirect now to logout from IdP
+              .processSamlResponse(REDIRECT)
+                .transformObject(this::createIdPLogoutResponse)
+                .targetAttributeSamlResponse()
+                .targetUri(getSamlBrokerUrl(REALM_NAME))
+                .build()
+
+              .getSamlResponse(REDIRECT);
+
+            assertThat(samlResponse.getSamlObject(), isSamlStatusResponse(JBossSAMLURIConstants.STATUS_SUCCESS));
+        }
+    }
+
+    private URI getSamlBrokerUrl(String realmName) {
+        return URI.create(getAuthServerRealmBase(realmName).toString() + "/broker/" + SAML_BROKER_ALIAS + "/endpoint");
+    }
+
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/IdentityProviderBuilder.java

@@ -19,6 +19,7 @@ package org.keycloak.testsuite.util;
 
 import java.util.HashMap;
 import org.keycloak.representations.idm.IdentityProviderRepresentation;
+import java.util.Map;
 
 /**
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
@@ -51,11 +52,25 @@ public class IdentityProviderBuilder {
     }
     
     public IdentityProviderBuilder hideOnLoginPage() {
+        setAttribute("hideOnLoginPage", "true");
+        return this;
+    }
+
+    public IdentityProviderBuilder setAttribute(String name, String value) {
+        config().put(name, value);
+        return this;
+    }
+
+    public IdentityProviderBuilder removeAttribute(String name) {
+        config().put(name, null);
+        return this;
+    }
+
+    private Map<String, String> config() {
         if (rep.getConfig() == null) {
             rep.setConfig(new HashMap<>());
         }
-        rep.getConfig().put("hideOnLoginPage", "true");
+        return rep.getConfig();
-        return this;
     }
 
     public IdentityProviderRepresentation build() {