8000 Use POST binding for logout when REDIRECT url is not set and forced POST by rmartinc · Pull Request #40757 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use POST binding for logout when REDIRECT url is not set and forced POST #40757

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

Jump to bottom
Merged
merged 1 commit into from
Jun 30, 2025
Merged

Use POST binding for logout when REDIRECT url is not set and forced POST #40757

merged 1 commit into from
Jun 30, 2025

Conversation

rmartinc
Copy link
Contributor
@rmartinc rmartinc commented Jun 27, 2025 •
edited
Loading

Closes #40637

A little regression created in 26.2 for SAML by #32262. Previously if just the admin URL (master SAML processing URL) was set, POST was used for frontchannel logout (see this line in 26.1.5). After that issue the default is REDIRECT for that configuration, even if force POST is configured to ON in the client. I also changed that method to do a short-circuit evaluation (reversing the order and returning immediately). The meaningful change is if (postUrlSet && !redirectUrlSet) to if (!redirectUrlSet && (postUrlSet || samlClient.forcePostBinding())). Created one integration test and modified the unitary test too.

Note that I didn't revert the functionality completely because IMHO it makes more sense like it is in the PR. The issue is only when there is only master saml processing URL, in that case previously it was always POST, currently is always REDIRECT, and with the PR its POST or REDIRECT depending the configuration of the Force POST binding switch. Normally the logout URLs are filled if you use a SP descriptor to create the client, and the Force POST binding switch is by default set to true.

@rmartinc rmartinc requested review from a team as code owners June 27, 2025 13:00
keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Jun 27, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

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

@keycloak-github-bot
Copy link

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.cluster.ComponentInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

@mposolda mposolda self-assigned this Jun 30, 2025
mposolda
mposolda approved these changes Jun 30, 2025
View reviewed changes
Copy link
Contributor
@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@rmartinc Thanks!

@mposolda mposolda merged commit 2db98b6 into keycloak:main Jun 30, 2025
76 checks passed
@ahus1
Copy link
Contributor
ahus1 commented Jun 30, 2025

Pinging @nxpthx as this fix was related to the PR created for #32262.

I'm sorry that I've contributed to this issue by approving the PR.

@rmartinc rmartinc mentioned this pull request Jul 1, 2025
Merged
@nxpthx
Copy link
Contributor
nxpthx commented Jul 1, 2025

@rmartinc While I agree, the the "default fallback" changed, which is fixed by your update, I am not sure if we still behave like users would expect.
So yes it makes sense to use the "force Options" and afterwards the same Binding type as used for Login IF there is only the master-saml-url configured.
If a user explicitly configured a URL for logging out via a specific binding, he might expect, that this URL is taken for logout.

@rmartinc
Copy link
Contributor Author
rmartinc commented Jul 1, 2025

@nxpthx Yes, the only important change is that if, and it is just executed in the last case when there is no redirect logout Url (only adminUrl). So I think we are good. It's a bit of a corner case when there is no specific logout URL configured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@mposolda mposolda mposolda approved these changes

Assignees

@mposolda mposolda

Labels
flaky-test team/cloud-native team/core-clients team/core-iam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Front logout channel broken in 26.2.5 for saml
4 participants
@rmartinc @ahus1 @nxpthx @mposolda
0