8000 Handle missing Single Logout Service URL during SAML IdP-initiated logout (closes #53242) by atiqur-rahman-pro · Pull Request #53281 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Handle missing Single Logout Service URL during SAML IdP-initiated logout (closes #53242) - #53281

Merged
pedroigor merged 3 commits into
keycloak:mainfrom
atiqur-rahman-pro:fix/saml-broker-logout-empty-slo
Sep 30, 2026
Merged

Handle missing Single Logout Service URL during SAML IdP-initiated logout (closes #53242)#53281
pedroigor merged 3 commits into
keycloak:mainfrom
atiqur-rahman-pro:fix/saml-broker-logout-empty-slo

Conversation

@atiqur-rahman-pro
@atiqur-rahman-pro atiqur-rahma 8000 n-pro commented Sep 29, 2026
Copy link
Copy Markdown
Contributor

Description

When an Identity Provider initiates SAML logout by sending a LogoutRequest to Keycloak's broker endpoint, Keycloak completes the backchannel logout for the session, but then attempts to construct and send a LogoutResponse. If the SAML Identity Provider configuration has an empty or blank Single Logout Service URL, Keycloak throws an uncaught HTTP 500 error (IllegalArgumentException: uri parameter is null).

This PR:

  • Checks if singleLogoutServiceUrl is blank in SAMLEndpoint.
  • If blank, skips building the LogoutResponse, records a success event, and returns the successful logout info page.
  • Successfully completes the logout process and cleans up consumer user sessions without a 500 server error.
  • Adds integration test coverage in KcSamlLogoutTest.

Closes #53242

@atiqur-rahman-pro
Handle missing Single Logout Service URL during SAML IdP logout (keyc…
0bafec9 
…loak#53242)

Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 06:59
@atiqur-rahman-pro
atiqur-rahman-pro requested review from a team as code owners September 29, 2026 06:59
Copilot AI reviewed Sep 29, 2026
View reviewed changes
Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The integration test does not verify session removal or successful logout-event delivery.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Handles SAML IdP-initiated logout when no Single Logout Service URL is configured.

Changes:

  • Returns a successful logout page instead of constructing an invalid response.
  • Records logout success and avoids transaction rollback.
  • Adds integration coverage for the missing-URL scenario.
File Description
services/​src/​main/​java/​org/​keycloak/​broker/​saml/​SAMLEndpoint.java Handles blank logout service URLs safely.
testsuite/​integration-arquillian/​tests/​base/​src/​test/​java/​org/​keycloak/​testsuite/​broker/​KcSamlLogoutTest.java Tests the missing-URL logout flow.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ...tion-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlLogoutTest.java Resolved
Comment thread ...tion-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlLogoutTest.java Resolved
@atiqur-rahman-pro
Assert consumer user session is removed after logout
8000
c0cf11d 
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Copilot AI review requested due to automatic review settings September 29, 2026 07:32
Copilot AI reviewed Sep 29, 2026
View reviewed changes
Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The regression test must be migrated from the deprecated Arquillian testsuite to the current test framework.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
Resolved since last review (1)

Comment thread ...tion-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlLogoutTest.java Resolved
@atiqur-rahman-pro
atiqur-rahman-pro commented Sep 29, 2026
Copy link
Copy Markdown
Contributor Author

The SAML broker logout test coverage currently resides in KcSamlLogoutTest within the Arquillian testsuite (consistent with recent SAML logout changes such as #52592). Once SAML broker logout tests are migrated to the new test framework, this will follow along.

@pedroigor pedroigor mentioned this pull request Sep 29, 2026
Closed
1 of 2 tasks
pedroigor
pedroigor previously approved these changes Sep 30, 2026
View reviewed changes
@pedroigor
pedroigor enabled auto-merge (rebase) September 30, 2026 10:12
@pedroigor
pedroigor commented Sep 30, 2026
Copy link
Copy Markdown
Contributor

Thanks, @atiqur-rahman-pro.

@atiqur-rahman-pro
Compact SAML broker logout test to comply with deprecation check thre…
350cd88 
…shold

Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 12:00
auto-merge was automatically disabled September 30, 2026 12:00

Head branch was pushed to by a user without write access

Copilot AI reviewed Sep 30, 2026
View reviewed changes
Copilot AI 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.

Copilot review overview

🟢 Approval recommended

The focused fix prevents transaction rollback and is covered by an end-to-end regression test.

Review effort: Balanced
Findings: None

Resolved since last review (2)

@atiqur-rahman-pro
atiqur-rahman-pro commented Sep 30, 2026
Copy link
Copy Markdown
Contributor Author

Hi @pedroigor, all CI checks have completed successfully. The PR is clean and ready to merge. Thank you!

@pedroigor
pedroigor commented Sep 30, 2026
Copy link
Copy Markdown
Contributor

Thanks, @atiqur-rahman-pro.

@pedroigor
pedroigor merged commit 8eab1ae into keycloak:main Sep 30, 2026
94 of 164 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@pedroigor pedroigor pedroigor approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SAML broker: IdP-initiated logout fails with HTTP 500 and is rolled back when the IdP has no single logout service URL

3 participants

@atiqur-rahman-pro @pedroigor
0