Skip to content

Commit

Permalink
Invalidate user session when associated IdP is missing (previously re…
Browse files Browse the repository at this point in the history 
…moved)

Closes keycloak#31724

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
  • Loading branch information
@sguilhen @pedroigor
sguilhen authored and pedroigor committed Oct 17, 2024
1 parent 731274f commit 7d8ff71
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 6 deletions.
38 changes: 32 additions & 6 deletions services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.keycloak.services.managers;

import org.jboss.logging.Logger;
import org.keycloak.broker.provider.IdentityBrokerException;
import org.keycloak.cookie.CookieProvider;
import org.keycloak.cookie.CookieType;
import org.keycloak.http.HttpRequest;
Expand Down Expand Up @@ -178,6 +179,13 @@ public static boolean isSessionValid(RealmModel realm, UserSessionModel userSess
logger.debug("No user session");
return false;
}
if (userSession.getNote(Details.IDENTITY_PROVIDER) != null) {
String brokerAlias = userSession.getNote(Details.IDENTITY_PROVIDER);
if (realm.getIdentityProviderByAlias(brokerAlias) == null) {
// associated idp was removed, invalidate the session.
return false;
}
}
long currentTime = Time.currentTimeMillis();
long lifespan = SessionExpirationUtils.calculateUserSessionMaxLifespanTimestamp(userSession.isOffline(),
userSession.isRememberMe(), TimeUnit.SECONDS.toMillis(userSession.getStarted()), realm);
Expand Down Expand Up @@ -417,12 +425,19 @@ private static BackchannelLogoutResponse backchannelLogoutAll(KeycloakSession se
if (logoutBroker) {
String brokerId = userSession.getNote(Details.IDENTITY_PROVIDER);
if (brokerId != null) {
IdentityProvider identityProvider = IdentityBrokerService.getIdentityProvider(session, brokerId);
IdentityProvider identityProvider = null;
try {
identityProvider.backchannelLogout(session, userSession, uriInfo, realm);
} catch (Exception e) {
logger.warn("Exception at broker backchannel logout for broker " + brokerId, e);
backchannelLogoutResponse.setLocalLogoutSucceeded(false);
identityProvider = IdentityBrokerService.getIdentityProvider(session, brokerId);
} catch (IdentityBrokerException e) {
logger.warn("Skipping backchannel logout for broker " + brokerId + " - not found");
}
if (identityProvider != null) {
try {
identityProvider.backchannelLogout(session, userSession, uriInfo, realm);
} catch (Exception e) {
logger.warn("Exception at broker backchannel logout for broker " + brokerId, e);
backchannelLogoutResponse.setLocalLogoutSucceeded(false);
}
}
}
}
Expand Down Expand Up @@ -1516,7 +1531,18 @@ public static AuthResult verifyIdentityToken(KeycloakSession session, RealmModel
}
}

if (userSession != null) backchannelLogout(session, realm, userSession, uriInfo, connection, headers, true);

if (userSession != null) {
String userSessionId = userSession.getId();
KeycloakModelUtils.runJobInTransaction(session.getKeycloakSessionFactory(), session.getContext(), newSession -> {
RealmModel realmModel = newSession.realms().getRealm(realm.getId());
UserSessionModel userSessionModel = newSession.sessions().getUserSession(realmModel, userSessionId);
backchannelLogout(newSession, realmModel, userSessionModel, uriInfo, connection, headers, true);
});
// remove the user session here so that the external persistent session tx becomes aware of the removal that happened
// during the backchannel logout.
session.sessions().removeUserSession(realm, userSession);
}
logger.debug("User session not active");
return null;
}
Expand Down
45 changes: 45 additions & 0 deletions ...n-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,13 @@
import org.keycloak.broker.oidc.mappers.UserAttributeMapper;
import org.keycloak.broker.provider.util.SimpleHttp;
import org.keycloak.crypto.Algorithm;
import org.keycloak.models.ClientModel;
import org.keycloak.models.IdentityProviderMapperModel;
import org.keycloak.models.IdentityProviderMapperSyncMode;
import org.keycloak.models.IdentityProviderModel;
import org.keycloak.models.IdentityProviderSyncMode;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserSessionModel;
import org.keycloak.models.utils.TimeBasedOTP;
import org.keycloak.protocol.ProtocolMapperUtils;
import org.keycloak.protocol.oidc.OIDCConfigAttributes;
Expand All @@ -40,6 +43,7 @@
import org.keycloak.representations.idm.RealmRepresentation;
import org.keycloak.representations.idm.RoleRepresentation;
import org.keycloak.representations.idm.UserRepresentation;
import org.keycloak.services.managers.AuthenticationManager;
import org.keycloak.testsuite.Assert;
import org.keycloak.testsuite.admin.ApiUtil;
import org.keycloak.testsuite.broker.util.SimpleHttpDefault;
Expand All @@ -49,6 +53,7 @@
import org.keycloak.testsuite.util.WaitUtils;

import jakarta.ws.rs.core.Response;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
Expand All @@ -59,6 +64,7 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasItems;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
Expand Down Expand Up @@ -457,6 +463,45 @@ public void testInvalidIssuedFor() {
errorPage.assertCurrent();
}

@Test
public void testIdpRemovedAfterLoginInvalidatesUserSession() {
loginUser();
AccountHelper.logout(adminClient.realm(bc.consumerRealmName()), bc.getUserLogin());
AccountHelper.logout(adminClient.realm(bc.providerRealmName()), bc.getUserLogin());

oauth.clientId("broker-app");
loginPage.open(bc.consumerRealmName());
assertThat(loginPage.isSocialButtonPresent(bc.getIDPAlias()), is(true));
logInWithBroker(bc);

// remove the IDP while the user is logged in
adminClient.realm(bc.consumerRealmName()).identityProviders().get(bc.getIDPAlias()).remove();

// user session should still be active, but checking if it is valid should fail as the associated IDP was removed
testingClient.server(bc.consumerRealmName()).run(session -> {
RealmModel realm = session.getContext().getRealm();
ClientModel client = session.clients().getClientByClientId(realm, "broker-app");
List<UserSessionModel> userSessions = session.sessions().getUserSessionsStream(realm, client).toList();
assertThat(userSessions, hasSize(1));
UserSessionModel userSession = userSessions.get(0);
assertThat(AuthenticationManager.isSessionValid(realm, userSession), is(false));
});

// logout should work even after the IDP was removed
AccountHelper.logout(adminClient.realm(bc.consumerRealmName()), bc.getUserLogin());

// session should have been removed now
testingClient.server(bc.consumerRealmName()).run(session -> {
RealmModel realm = session.getContext().getRealm();
ClientModel client = session.clients().getClientByClientId(realm, "broker-app");
List<UserSessionModel> userSessions = session.sessions().getUserSessionsStream(realm, client).toList();
assertThat(userSessions, hasSize(0));
});

loginPage.open(bc.consumerRealmName());
assertThat(loginPage.isSocialButtonPresent(bc.getIDPAlias()), is(false));
}

@Test
public void testInvalidAudience() {
loginUser();
Expand Down

0 comments on commit 7d8ff71

Please sign in to comment.