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

No OIDC frontchannel logout when session_state is missing in the ID token #184

Open
djbgeodan opened this issue Oct 3, 2023 · 5 comments · Fixed by #185
Open

No OIDC frontchannel logout when session_state is missing in the ID token #184

djbgeodan opened this issue Oct 3, 2023 · 5 comments · Fixed by #185

Comments

@djbgeodan
Copy link
Contributor

Hi Travis,

We use the OIDC plugin with a external IdD. This works great, except the the logout at the IdP does not take place, when the logout handler is called. I think it is caused by the fact that the ID token does not contain the session_state claim. https://github.com/travisghansen/external-auth-server/blob/master/src/plugin/oauth/index.js#L1515C39-L1515C39

In the code there is the comment TODO: this check may not be entirely needed/wanted . So my question is, can this condition be removed?

Regards, Dirk-Jan

@travisghansen
Copy link
Owner

Reading through some docs and rfcs I think it can be removed. It appears to me that field in the id token is not necessarily common (may be a keycloak-centric behavior) and has no direct tie to the logout functionality. It may be a hint the provider actually supports logout but otherwise seems to have no bearing on the logout process.

I can make the change when I get a moment or you are welcome to submit a PR I can merge.

@travisghansen
Copy link
Owner

Can you test using the next image tag? It’s a mutable tag so make sure your cluster pulls the newest revision.

@djbgeodan
Copy link
Contributor Author

Thanks, I've tested it. It's solves our logout issue.

@travisghansen
Copy link
Owner

I’ve just committed a small change in that same area of code. Can you pull the most recent next image and ensure everything still works as needed?

@djbgeodan
Copy link
Contributor Author

I've tested it. All seems to work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants