Skip to content

Add post_logout_redirect_uri support and add test - #22130

Open
nuwang wants to merge 5 commits into
galaxyproject:devfrom
nuwang:add_idp_logout_test
Open

Add post_logout_redirect_uri support and add test#22130
nuwang wants to merge 5 commits into
galaxyproject:devfrom
nuwang:add_idp_logout_test

Conversation

@nuwang

@nuwang nuwang commented Mar 16, 2026

Copy link
Copy Markdown
Member

closes: #21275

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@nuwang
nuwang requested a review from ahmedhamidawan March 17, 2026 03:28

@ahmedhamidawan ahmedhamidawan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just added a little commit that reuses the existing addSearchParams utility for constructing the logout URL.

As for the rest, looks fine to me, however, I haven't been able to test out the entire flow locally with an example.

@dannon dannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The old client flow called /user/logout first, so the Galaxy session was already dead before attempting /authnz/logout. Now when OIDC is enabled only /authnz/logout is called — if that request fails, the server-side session survives. Acceptable tradeoff or worth a fallback?

The redirect_uripost_logout_redirect_uri rename is correct per spec — do you know if any non-standard OIDC providers out there relied on the old param name? Might be worth a release note if so.

@mvdbeek mvdbeek moved this from Needs Review to In Progress in Galaxy Dev - weeklies Mar 24, 2026
@nuwang

nuwang commented Mar 26, 2026

Copy link
Copy Markdown
Member Author

@dannon The intent was indeed to invalidate the server side session as well. Isn't this call sufficient? trans.handle_user_logout(logout_all=asbool(logout_all))

Not aware of anyone relying on redirect_uri, but no harm in a release note I suppose.

@mvdbeek

mvdbeek commented May 19, 2026

Copy link
Copy Markdown
Member

There's a conflict here

@mvdbeek

mvdbeek commented May 19, 2026

Copy link
Copy Markdown
Member

Pushing the milestone, freeze is tomorrow

@mvdbeek mvdbeek modified the milestones: 26.1, 26.2 May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Keycloak/OIDC logout should no longer use redirect_uri in favour of post_logout_redirect_uri

4 participants