Skip to content
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.

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

Correct OIDC logout to properly support Keycloak #1656

Closed
wants to merge 2 commits into from

Conversation

teunis90
Copy link

Keycloak expects these two values:

post_logout_redirect_uri (and not redirect_uri)
id_token_hint (which is recommended by the spec: https://openid.net/specs/openid-connect-rpinitiated-1_0.html, but required by keycloak)

Tested on Keycloak version: 20.0.5

Fixes #1222

@AzorianMatt AzorianMatt added mod / accepted This request has been accepted mod / reviewed This request has been reviewed feature / update Existing feature modification labels Aug 29, 2023
@@ -652,10 +652,11 @@ def logout():

redirect_uri = url_for('index.login')
oidc_logout = Setting().get('oidc_oauth_logout_url')
id_token_hint = session.get('oidc_token').get('id_token')
Copy link
Collaborator

Choose a reason for hiding this comment

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

If Local DB access is still enabled and a user goes to logout, they will hit an error as this value is not defined for their session

Copy link
Author

Choose a reason for hiding this comment

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

@AgentTNT, can you reproduce the not-defined error? What do you suggest?

oidc_logout, url_for('index.login', _external=True))
if id_token_hint and oidc_logout:
redirect_uri = "{}?post_logout_redirect_uri={}&id_token_hint={}".format(
url_for('index.login', _external=True), oidc_logout, id_token_hint)

Copy link
Collaborator

Choose a reason for hiding this comment

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

if 'oidc_token' in session and oidc_logout: id_token_hint = session.get('oidc_token').get('id_token') redirect_uri = "{}?post_logout_redirect_uri={}&id_token_hint={}".format( oidc_logout, url_for('index.login', _external=True), id_token_hint)

This makes powerdns-admin redirect to the keycloak or other OIDC logout endpoint first, then get redirected back to the login page, fully logging out all OIDC session cookies within the browser whereas before the IDP was still staying signed in and not allowing users to login with new creds when clicking the OIDC login

Copy link
Author

Choose a reason for hiding this comment

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

I'll update the PR with your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@teunis90 the time is nearing quickly for me to make the final version release of this edition of PDA. If you would like to see this make it in, I would recommend getting that last change request implemented very soon. Thanks!

@AzorianMatt AzorianMatt added this to the V0.4.2 milestone Dec 8, 2023
@AzorianMatt AzorianMatt changed the title Fix OIDC logout Correct OIDC logout to properly support Keycloak Dec 8, 2023
@AzorianMatt AzorianMatt modified the milestones: v0.4.2, v0.4.3 Jan 31, 2024
Copy link

github-actions bot commented May 1, 2024

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken. Please see our Contribution Guide.

@github-actions github-actions bot added the mod / stale This request has gone stale label May 1, 2024
Copy link

This PR has been automatically closed due to lack of activity.

@github-actions github-actions bot closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / update Existing feature modification mod / accepted This request has been accepted mod / reviewed This request has been reviewed mod / stale This request has gone stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants