Skip to content

Keycloak 12624 nullpointer during OIDC logout client disabled #13424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mposolda merged 5 commits into from
Aug 8, 2022
Merged

Keycloak 12624 nullpointer during OIDC logout client disabled #13424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57742cf
fixes #12624
marcelomrwin Jul 29, 2022
c32bd6f
Merge branch 'keycloak:main' into KEYCLOAK-12624-nullpointer-during-o…
marcelomrwin Jul 29, 2022
16946db
fixes #12624
marcelomrwin Aug 3, 2022
a436f4f
Merge branch 'keycloak:main' into KEYCLOAK-12624-nullpointer-during-o…
marcelomrwin Aug 3, 2022
72f0260
Merge branch 'keycloak:main' into KEYCLOAK-12624-nullpointer-during-o…
marcelomrwin Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion services/src/main/java/org/keycloak/services/resources/LogoutSessionCodeChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
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);
}

Expand All @@ -67,4 +67,9 @@ protected boolean isActionActive(ClientSessionCode.ActionType actionType) {
}
return true;
}

@Override
protected boolean checkClientDisabled(ClientModel client) {
return !client.isEnabled() && getClientCode() != null;
}
}
16 changes: 11 additions & 5 deletions services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -151,7 +152,8 @@ public AuthenticationSessionModel initialVerifyAuthSession() {
// 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())) {
Expand Down Expand Up @@ -222,9 +224,9 @@ public boolean initialVerify() {
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;
}
Expand All @@ -236,7 +238,7 @@ public boolean initialVerify() {
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
Expand Down Expand Up @@ -378,7 +380,7 @@ protected Response restartAuthenticationSessionFromCookie(RootAuthenticationSess
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);
}
Expand Down Expand Up @@ -449,4 +451,8 @@ protected KeycloakSession getSession() {
protected EventBuilder getEvent() {
return event;
}

protected boolean checkClientDisabled(ClientModel client) {
return !client.isEnabled();
}
}
5 changes: 5 additions & 0 deletions ...lian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,9 @@ public ClientAttributeUpdater setDirectAccessGrantsEnabled(Boolean directAccessG
rep.setDirectAccessGrantsEnabled(directAccessGranted);
return this;
}

public ClientAttributeUpdater setEnabled(Boolean enabled){
rep.setEnabled(enabled);
return this;
}
}
32 changes: 32 additions & 0 deletions ...on-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/LegacyLogoutTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -30,6 +32,7 @@
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;
Expand All @@ -48,6 +51,7 @@
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;
Expand Down Expand Up @@ -206,7 +210,35 @@ public void logoutRedirectWithStarRedirectUriForDirectGrantClient() {
}
}

// 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");
Expand Down
61 changes: 50 additions & 11 deletions ...quillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/RPInitiatedLogoutTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@

/**
* 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>
Expand Down Expand Up @@ -141,6 +141,7 @@ public void clientConfiguration() {

@Test
public void logoutRedirect() {

OAuthClient.AccessTokenResponse tokenResponse = loginUser();
String sessionId = tokenResponse.getSessionState();

Expand Down Expand Up @@ -292,7 +293,7 @@ public void logoutWithRememberMe() throws IOException {
assertEquals("test-user@localhost", loginPage.getUsername());

loginPage.login("test-user@localhost", "password");

//log out
appPage.openAccount();
accountManagementPage.signOut();
Expand Down Expand Up @@ -506,8 +507,7 @@ public void logoutWithoutIdTokenHintWithoutPostLogoutRedirectUri() {
try {
logoutConfirmPage.clickBackToApplicationLink();
fail();
}
catch (NoSuchElementException ex) {
} catch (NoSuchElementException ex) {
// expected
}

Expand Down Expand Up @@ -562,8 +562,7 @@ public void logoutExpiredConfirmationAction() {
try {
errorPage.clickBackToApplication();
fail();
}
catch (NoSuchElementException ex) {
} catch (NoSuchElementException ex) {
// expected
}
}
Expand Down Expand Up @@ -760,8 +759,7 @@ public void logoutWithBadClientId() {
try {
logoutConfirmPage.clickBackToApplicationLink();
fail();
}
catch (NoSuchElementException ex) {
} catch (NoSuchElementException ex) {
// expected
}

Expand Down Expand Up @@ -880,8 +878,7 @@ public void testLocalizationDuringLogout() throws IOException {
try {
logoutConfirmPage.clickBackToApplicationLink();
fail();
}
catch (NoSuchElementException ex) {
} catch (NoSuchElementException ex) {
// expected
}

Expand Down Expand Up @@ -944,7 +941,7 @@ public void testFrontChannelLogoutWithPostLogoutRedirectUri() throws Exception {
ClientRepresentation rep = clients.findByClientId(oauth.getClientId()).get(0);
rep.setFrontchannelLogout(true);
rep.getAttributes().put(OIDCConfigAttributes.FRONT_CHANNEL_LOGOUT_URI, oauth.APP_ROOT + "/admin/frontchannelLogout");
clients.get(rep.getId()).update(rep);
clients.get(rep.getId()).update(rep);
try {
oauth.clientSessionState("client-session");
oauth.doLogin("test-user@localhost", "password");
Expand Down Expand Up @@ -1029,7 +1026,49 @@ public void testFrontChannelLogout() throws Exception {
}
}

@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);
}
Expand Down