NullPointer during OIDC logout client disabled (#13424)

closes #12624

services/src/main/java/org/keycloak/services/resources/LogoutSessionCodeChecks.java

@@ -42,7 +42,7 @@ import org.keycloak.sessions.RootAuthenticationSessionModel;
 public class LogoutSessionCodeChecks extends SessionCodeChecks {
 
     public LogoutSessionCodeChecks(RealmModel realm, UriInfo uriInfo, HttpRequest request, ClientConnection clientConnection, KeycloakSession session, EventBuilder event,
-                             String code, String clientId, String tabId) {
+                                   String code, String clientId, String tabId) {
         super(realm, uriInfo, request, clientConnection, session, event, null, code, null, clientId, tabId, null);
     }
 
@@ -67,4 +67,9 @@ public class LogoutSessionCodeChecks extends SessionCodeChecks {
         }
         return true;
     }
+
+    @Override
+    protected boolean checkClientDisabled(ClientModel client) {
+        return !client.isEnabled() && getClientCode() != null;
+    }
 }

services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java

@@ -18,6 +18,7 @@
 package org.keycloak.services.resources;
 
 import java.net.URI;
+
 import javax.ws.rs.core.Cookie;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriBuilder;
@@ -151,7 +152,8 @@ public class SessionCodeChecks {
         // object retrieve
         AuthenticationSessionManager authSessionManager = new AuthenticationSessionManager(session);
         AuthenticationSessionModel authSession = null;
-        if (authSessionId != null) authSession = authSessionManager.getAuthenticationSessionByIdAndClient(realm, authSessionId, client, tabId);
+        if (authSessionId != null)
+            authSession = authSessionManager.getAuthenticationSessionByIdAndClient(realm, authSessionId, client, tabId);
         AuthenticationSessionModel authSessionCookie = authSessionManager.getCurrentAuthenticationSession(realm, client, tabId);
 
         if (authSession != null && authSessionCookie != null && !authSession.getParentSession().getId().equals(authSessionCookie.getParentSession().getId())) {
@@ -222,9 +224,9 @@ public class SessionCodeChecks {
         setClientToEvent(client);
         session.getContext().setClient(client);
 
-        if (!client.isEnabled()) {
+        if (checkClientDisabled(client)) {
             event.error(Errors.CLIENT_DISABLED);
-            response = ErrorPage.error(session,authSession, Response.Status.BAD_REQUEST, Messages.LOGIN_REQUESTER_NOT_ENABLED);
+            response = ErrorPage.error(session, authSession, Response.Status.BAD_REQUEST, Messages.LOGIN_REQUESTER_NOT_ENABLED);
             clientCode.removeExpiredClientSession();
             return false;
         }
@@ -236,7 +238,7 @@ public class SessionCodeChecks {
             String lastFlow = authSession.getAuthNote(AuthenticationProcessor.CURRENT_FLOW_PATH);
 
             // Check if we transitted between flows (eg. clicking "register" on login screen)
-            if (execution==null && !flowPath.equals(lastFlow)) {
+            if (execution == null && !flowPath.equals(lastFlow)) {
                 logger.debugf("Transition between flows! Current flow: %s, Previous flow: %s", flowPath, lastFlow);
 
                 // Don't allow moving to different flow if I am on requiredActions already
@@ -378,7 +380,7 @@ public class SessionCodeChecks {
         AuthenticationSessionModel authSession = null;
 
         Cookie cook = RestartLoginCookie.getRestartCookie(session);
-        if(cook == null){
+        if (cook == null) {
             event.error(Errors.COOKIE_NOT_FOUND);
             return ErrorPage.error(session, authSession, Response.Status.BAD_REQUEST, Messages.COOKIE_NOT_FOUND);
         }
@@ -449,4 +451,8 @@ public class SessionCodeChecks {
     protected EventBuilder getEvent() {
         return event;
     }
+
+    protected boolean checkClientDisabled(ClientModel client) {
+        return !client.isEnabled();
+    }
 }

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

@@ -142,4 +142,9 @@ public class ClientAttributeUpdater extends ServerResourceUpdater<ClientAttribut
         rep.setDirectAccessGrantsEnabled(directAccessGranted);
         return this;
     }
+
+    public ClientAttributeUpdater setEnabled(Boolean enabled){
+        rep.setEnabled(enabled);
+        return this;
+    }
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/LegacyLogoutTest.java

@@ -18,10 +18,12 @@
 
 package org.keycloak.testsuite.oauth;
 
+import java.io.Closeable;
 import java.util.Collections;
 
 import javax.ws.rs.NotFoundException;
 
+import org.hamcrest.MatcherAssert;
 import org.jboss.arquillian.graphene.page.Page;
 import org.junit.After;
 import org.junit.Before;
@@ -30,6 +32,7 @@ import org.junit.Test;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.OAuthErrorException;
 import org.keycloak.admin.client.resource.ClientResource;
+import org.keycloak.admin.client.resource.ClientsResource;
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
 import org.keycloak.protocol.LoginProtocol;
@@ -48,6 +51,7 @@ import org.keycloak.testsuite.pages.InfoPage;
 import org.keycloak.testsuite.pages.LoginPage;
 import org.keycloak.testsuite.pages.LogoutConfirmPage;
 import org.keycloak.testsuite.pages.OAuthGrantPage;
+import org.keycloak.testsuite.updaters.ClientAttributeUpdater;
 import org.keycloak.testsuite.util.InfinispanTestTimeServiceRule;
 import org.keycloak.testsuite.util.OAuthClient;
 import org.keycloak.testsuite.util.ServerURLs;
@@ -206,7 +210,35 @@ public class LegacyLogoutTest extends AbstractTestRealmKeycloakTest {
         }
     }
 
+    // Test logout with deprecated "redirect_uri" and without "id_token_hint" and client disabled after login
+    @Test
+    public void logoutWithLegacyRedirectUriAndWithoutIdTokenHintClientDisabled() throws Exception {
+        OAuthClient.AccessTokenResponse tokenResponse = loginUser();
+        String sessionId = tokenResponse.getSessionState();
 
+        try (Closeable testAppClient = ClientAttributeUpdater.forClient(adminClient, "test", oauth.getClientId())
+                .setEnabled(false).update()) {
+
+            ClientsResource clients = adminClient.realm(oauth.getRealm()).clients();
+            ClientRepresentation rep = clients.findByClientId(oauth.getClientId()).get(0);
+            MatcherAssert.assertThat(false, is(rep.isEnabled()));
+
+            String logoutUrl = oauth.getLogoutUrl().redirectUri(APP_REDIRECT_URI).build();
+            driver.navigate().to(logoutUrl);
+
+            // Assert logout confirmation page. Session still exists. Assert default language on logout page (English)
+            logoutConfirmPage.assertCurrent();
+            MatcherAssert.assertThat(true, is(isSessionActive(sessionId)));
+            events.assertEmpty();
+            logoutConfirmPage.confirmLogout();
+
+            // Redirected back to the application with expected state
+            events.expectLogout(sessionId).removeDetail(Details.REDIRECT_URI).assertEvent();
+            MatcherAssert.assertThat(false, is(isSessionActive(sessionId)));
+            assertCurrentUrlEquals(APP_REDIRECT_URI);
+        }
+
+    }
 
     private OAuthClient.AccessTokenResponse loginUser() {
         oauth.doLogin("test-user@localhost", "password");

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/RPInitiatedLogoutTest.java

@@ -93,7 +93,7 @@ import org.openqa.selenium.NoSuchElementException;
 
 /**
  * Test for OIDC RP-Initiated Logout - https://openid.net/specs/openid-connect-rpinitiated-1_0.html
- *
+ * <p>
  * This is handled on server-side by the LogoutEndpoint.logout method
  *
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
@@ -141,6 +141,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
 
     @Test
     public void logoutRedirect() {
+
         OAuthClient.AccessTokenResponse tokenResponse = loginUser();
         String sessionId = tokenResponse.getSessionState();
 
@@ -506,8 +507,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
         try {
             logoutConfirmPage.clickBackToApplicationLink();
             fail();
-        }
+        } catch (NoSuchElementException ex) {
-        catch (NoSuchElementException ex) {
             // expected
         }
 
@@ -562,8 +562,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
         try {
             errorPage.clickBackToApplication();
             fail();
-        }
+        } catch (NoSuchElementException ex) {
-        catch (NoSuchElementException ex) {
             // expected
         }
     }
@@ -760,8 +759,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
         try {
             logoutConfirmPage.clickBackToApplicationLink();
             fail();
-        }
+        } catch (NoSuchElementException ex) {
-        catch (NoSuchElementException ex) {
             // expected
         }
 
@@ -880,8 +878,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
             try {
                 logoutConfirmPage.clickBackToApplicationLink();
                 fail();
-            }
+            } catch (NoSuchElementException ex) {
-            catch (NoSuchElementException ex) {
                 // expected
             }
 
@@ -1029,7 +1026,49 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
         }
     }
 
+    @Test
+    public void logoutWithIdTokenAndDisabledClientMustWork() throws Exception {
+        OAuthClient.AccessTokenResponse tokenResponse = loginUser();
 
+
+        try (Closeable accountClientUpdater = ClientAttributeUpdater.forClient(adminClient, "test", oauth.getClientId())
+                .setEnabled(false).update()) {
+
+            String logoutUrl = oauth.getLogoutUrl().postLogoutRedirectUri(APP_REDIRECT_URI).clientId("test-app").build();
+            driver.navigate().to(logoutUrl);
+            MatcherAssert.assertThat(true, is(isSessionActive(tokenResponse.getSessionState())));
+            events.assertEmpty();
+
+            logoutConfirmPage.confirmLogout();
+            events.expectLogout(tokenResponse.getSessionState()).assertEvent();
+            MatcherAssert.assertThat(false, is(isSessionActive(tokenResponse.getSessionState())));
+        }
+
+    }
+
+    //Login and logout with account client disabled after login
+    @Test
+    public void testLogoutWhenAccountClientIsDisabled() throws IOException {
+
+        OAuthClient.AccessTokenResponse tokenResponse = loginUser();
+        String sessionId = tokenResponse.getSessionState();
+
+        try (Closeable accountClientUpdater = ClientAttributeUpdater.forClient(adminClient, "test", Constants.ACCOUNT_MANAGEMENT_CLIENT_ID)
+                .setEnabled(false)
+                .update()) {
+            String logoutUrl = oauth.getLogoutUrl().build();
+            driver.navigate().to(logoutUrl);
+
+            events.assertEmpty();
+            logoutConfirmPage.assertCurrent();
+            logoutConfirmPage.confirmLogout();
+
+            MatcherAssert.assertThat(false, is(isSessionActive(sessionId)));
+            MatcherAssert.assertThat(false, is(isSessionActive(tokenResponse.getSessionState())));
+        }
+    }
+
+    // SUPPORT METHODS
     private OAuthClient.AccessTokenResponse loginUser() {
         return loginUser(false);
     }