Repository navigation
Conversation
…been realmlimit, added unit-test, closes keycloak#46879 Signed-off-by: Marie Daly <marie.daly1@ibm.com>
Signed-off-by: Marie Daly <marie.daly1@ibm.com>
… correct excess calculation Signed-off-by: Marie Daly <marie.daly1@ibm.com>
…ith 2 tests to ensure comparator works corrfectly. Signed-off-by: Marie Daly <marie.daly1@ibm.com>
…ith 2 tests for session removal across multiple clients. Signed-off-by: Marie Daly <marie.daly1@ibm.com>
Signed-off-by: Marie Daly <marie.daly1@ibm.com>
There was a problem hiding this comment.
@msdaly200 Thanks for PR! I've just added a few comments.
Btw. the duplicated issue should be closed - as the issue should be resolved. If you'd like to do some improvements(like the comparator logic + tests), it'd be nice to create another issue(follow-up on the original issue) and reference it here. WDYT?
| "org.keycloak.tests.oid4vc", | ||
| "org.keycloak.tests.securityprofile", | ||
| "org.keycloak.tests.session", | ||
| "org.keycloak.tests.sessionlimits", |
There was a problem hiding this comment.
nitpick: Wouldn't it be better to have these session limits tests in the "org.keycloak.tests.session" package? As these tests might be relevant to every types of sessions + their characteristics (even the limits) WDYT?
There was a problem hiding this comment.
This is the folder the migrate.sh script created. As I broke down the large test into 3 smaller one, might be better to keep the folder name to reduce confusion. Also this folder tests only the UserSessionLimitsAuthenticator, other one is more session refresh and timeout tests.
| if (newUserSession && userRealmLimit > 0 && userSessionCountForRealm >= userRealmLimit) { | ||
| realmExcess = userSessionCountForRealm - userRealmLimit + 1; | ||
| } | ||
| if (newClientSession && userClientLimit > 0 && userSessionCountForClient >= userClientLimit) { | ||
| clientExcess = userSessionCountForClient - userClientLimit + 1; | ||
| } | ||
|
|
||
| boolean realmLimitHandled = false; | ||
| boolean clientLimitHandled = false; |
There was a problem hiding this comment.
Is there any benefit of introducing the new counters for realmExcess + clientExcess?
There was a problem hiding this comment.
Good point, I can tighten that up more.
| private List<UserSessionModel> getUserSessionsForClientIfEnabled(List<UserSessionModel> userSessionsForRealm, ClientModel currentClient, int userClientLimit) { | ||
| // Only count this users sessions for this client only in case a limit is configured, otherwise skip this costly operation. | ||
| if (userClientLimit <= 0) { | ||
| logger.debugf("total user sessions for this keycloak client will not be counted. Will be logged as 0 (zero)"); |
|
|
||
| // Prioritize current client's sessions for removal | ||
| Comparator<UserSessionModel> clientPriorityComparator = (s1, s2) -> { | ||
| boolean s1IsCurrent = s1.getAuthenticatedClientSessionByClient(currentClientId) != null; |
There was a problem hiding this comment.
Do we need to check if the user session is current for every comparison? AFAIK, this kind of check is done beforehand in the https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/authentication/authenticators/sessionlimits/UserSessionLimitsAuthenticator.java#L75.
I'd just go with the comparator of the getLastSessionRefresh + if we need some more fine-grained tie-braker, go with the UserSessionModel.getStarted()(I see it's also in seconds, but might not be the same, mainly due to the UserSessionEntity.mergeRemoteEntityWithLocalEntity().
@msdaly200 WDYT?
There was a problem hiding this comment.
Good catch, will change comparator.
Fix to UserSessionLimitsAuthenticator to correctly calculate no. of sessions to deny/terminate.
Problem: Users were encountering an HTTP 500 Server Error (IllegalArgumentException) when their total realm sessions exceeded the configured limit, but their specific client sessions did not. When the realm limit was hit, the code logic evaluated the client sessions against the client limit to figure out how many sessions to kill.
Fix:
[1] Code now calculates realmExcess and clientExcess independently and checks if (excess > 0) before attempting to terminate any sessions, so no negative numbers.
[2] Checks client limits before the realm limits, If a user has too many sessions and a limit must be enforced, the system will sacrifice the user's oldest session from the exact app they are currently trying to access before it ever touches their sessions in other, unrelated applications.
[3] Updated timestamp comparison logic as tests in new test framework were failing due to smaller time differences - reduce flakiness of migrated tests.
[4] Tests for UserSessionLimitsAuthenticator were migrated to new test framework and split into smaller files. New tests were added to ensure edge cases were handled correctly.
Closes #46879