8000 Improve handling for backchannel logout requests by thomasdarimont · Pull Request #34105 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Improve handling for backchannel logout requests - #34105

Merged
rmartinc merged 1 commit into
keycloak:mainfrom
thomasdarimont:issue/gh-34104-backchannel-logout-logging
May 20, 2026
Merged

Improve handling for backchannel logout requests#34105
rmartinc merged 1 commit into
keycloak:mainfrom
thomasdarimont:issue/gh-34104-backchannel-logout-logging

Conversation

@thomasdarimont
@thomasdarimont thomasdarimont commented Oct 18, 2024
Copy link
Copy Markdown
Contributor
  • Added guards for empty backchannelLogoutUrl and host in ResourceAdminManager#logoutClientSessionWithBackchannelLogoutUrl(..) to prevent NPEs.
  • Added additional logging to backchannel logout components to make it easier to analyze the logout request.

Fixes #34104

@thomasdarimont
thomasdarimont requested a review from a team as a code owner October 18, 2024 14:52
@jonkoops jonkoops changed the title Improve handling for backchannel logout requests (#34104) Improve handling for backchannel logout requests Oct 18, 2024
This was referenced Oct 18, 2024
Closed
Closed
@ahus1
ahus1 commented Oct 21, 2024
Copy link
Copy Markdown
Member

This seems to be @keycloak/core-clients related.

@msdaly200 msdaly200 self-assigned this May 6, 2026
@msdaly200
msdaly200 commented May 7, 2026
Copy link
Copy Markdown
Contributor

Hi @thomasdarimont, sorry for the long delay in getting back to you on this. So this PR is still relevant and was wondering if you still wish to work on this. Your fix does get rid of the NPE, but there are a couple of minor changes recommended,

[1] throw exception so logout doesn't fail silently, and
[2] The warning on line 187 in the ResourceAdminManager probably needs a bit more info as the host doesn't need to be empty for this warning to show, it can also be because the 'client_session_state' is missing while the 'client_session_host' is present.

Many thanks.

@thomasdarimont
thomasdarimont force-pushed the issue/gh-34104-backchannel-logout-logging branch from a5a8d49 to 53efcdf Compare May 12, 2026 16:20
@thomasdarimont
thomasdarimont commented May 12, 2026
Copy link
Copy Markdown
Contributor Author

@msdaly200 Thank you very much for the feedback! I updated the PR according to your suggestions.

@thomasdarimont
thomasdarimont force-pushed the issue/gh-34104-backchannel-logout-logging branch 2 times, most recently from 11d7b1e to aef08cb Compare May 12, 2026 22:05
@keycloak-github-bot
keycloak-github-bot Bot commented May 12, 2026
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.cli.registration.KcRegTest#testCRUDWithOnTheFlyUserAuthWithSignedJwtClient_PKCS12Keystore

Keycloak CI - FIPS IT (non-strict)

org.opentest4j.AssertionFailedError: login message ==> expected: <Logging into http://localhost:8180/auth as user user1 of realm test> but was: <May 12, 2026 10:35:31 PM org.keycloak.crypto.fips.FIPS1402Provider <init>>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
...

Report flaky test

org.keycloak.testsuite.cli.registration.KcRegTest#testCRUDWithOnTheFlyUserAuthWithClientSecret

Keycloak CI - FIPS IT (non-strict)

org.opentest4j.AssertionFailedError: exitCode == 0 ==> expected: <0> but was: <1>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
...

Report flaky test

org.keycloak.testsuite.cli.registration.KcRegTest#testCRUDWithOnTheFlyServiceAccountWithClientSecret

Keycloak CI - FIPS IT (non-strict)

org.opentest4j.AssertionFailedError: exitCode == 0 ==> expected: <0> but was: <1>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
...

Report flaky test

org.keycloak.testsuite.cli.registration.KcRegTest#testCRUDWithOnTheFlyUserAuth

Keycloak CI - FIPS IT (non-strict)

org.opentest4j.AssertionFailedError: exitCode == 0 ==> expected: <0> but was: <1>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
...

Report flaky test

org.keycloak.testsuite.cli.registration.KcRegTest#testCreateWithAllowedHostsWithoutAuthentication

Keycloak CI - FIPS IT (non-strict)

org.opentest4j.AssertionFailedError: exitCode == 0 ==> expected: <0> but was: <1>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed May 12, 2026
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

msdaly200
msdaly200 approved these changes May 15, 2026
View reviewed changes
@msdaly200 msdaly200 left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Thomas, LGTM. Sending null back and the new exception are both dealt with ok in the AuthenticationManager->backChennelLogoutClientSession method so flow looks good.

@thomasdarimont @rmartinc
Improve handling for backchannel logout requests (keycloak#34104)
211873d 
- Guard against a null backchannel logout URL in
ResourceAdminManager#logoutClientSessionWithBackchannelLogoutUrl.
- When the host placeholder cannot be resolved, throw an IllegalStateException
with diagnostic context (clientId, clientSessionId, both note values) instead
of silently returning null. The exception is handled by
AuthenticationManager#backchannelLogoutClientSession, which logs it and
returns a 5xx response so the failure is visible to operators.
- Expand debug logging around the backchannel logout flow to make it easier
to correlate token requests with logout attempts.

Fixes keycloak#34104

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
@rmartinc
rmartinc force-pushed the issue/gh-34104-backchannel-logout-logging branch from aef08cb to 211873d Compare May 19, 2026 14:13
@rmartinc
rmartinc commented May 19, 2026
Copy link
Copy Markdown
Contributor

I have rebased the branch to fix the issues we had the previous week with lint. Waiting for the tests.

rmartinc
rmartinc approved these changes May 20, 2026
View reviewed changes
@rmartinc rmartinc left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thomasdarimont and @msdaly200! LGTM too!

@rmartinc
rmartinc merged commit 3119efd into keycloak:main May 20, 2026
140 of 142 checks passed
@stianst stianst mentioned this pull request Jun 1, 2026
Closed
sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026
@thomasdarimont @sashyo
Improve handling for backchannel logout requests (keycloak#34104) (ke…
c9c427d 
…ycloak#34105)

- Guard against a null backchannel logout URL in
ResourceAdminManager#logoutClientSessionWithBackchannelLogoutUrl.
- When the host placeholder cannot be resolved, throw an IllegalStateException
with diagnostic context (clientId, clientSessionId, both note values) instead
of silently returning null. The exception is handled by
AuthenticationManager#backchannelLogoutClientSession, which logs it and
returns a 5xx response so the failure is visible to operators.
- Expand debug logging around the backchannel logout flow to make it easier
to correlate token requests with logout attempts.

Fixes keycloak#34104

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmartinc rmartinc rmartinc approved these changes
+2 more reviewers
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
@msdaly200 msdaly200 msdaly200 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@msdaly200 msdaly200

Labels

flaky-test team/core-authn

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve robustness of backchannel logout with client sessions

5 participants

@thomasdarimont @ahus1 @msdaly200 @rmartinc @stianst
0