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

Fix token 'aud' verification logic #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AliMickey
Copy link

The audience parameter that can be set during initialisation is not passed correctly during token verification and logout.

The audience parameter that can be set during initialisation is not passed correctly during token verification and logout.
@kroketio
Copy link
Owner

Hey, thanks for the PR.

I have not dealt with OpenID for a couple of years now, and I have lost my ability to determine if this PR should go in or not.

In general, this plugin was made for my simple use-case of login/logout, as noted in the README:

The OpenID specification is rather large (and confusing) and this extension tries to abstract the complicated parts away and makes the fair assumption that your web application wants some basic OIDC features, mostly: login and logout. Undoubtedly you may use Keycloak in various other exotic ways but this limited scope ensures the extension stays maintainable. Please keep that in mind when submitting a pull-request.

Merging code I do not fully understand is tricky, as it has security implications. For now, ill just leave this open.

@AliMickey
Copy link
Author

Thanks for the reply,

I don't understand how this is a tricky or complicated change? It's simply allowing for the correct audience parameter to be used when the plugin is initialised.

The default behaviour for OIDC clients is to use client_id for the audience parameter, so in most cases this bug hasn't affected workload (or it has but no one has bothered to raise this issue, let alone try and fix it).

I have tested locally, and it works as expected, feel free to test it on your end before you merge, but I don't believe leaving this open is the best way forward for the community.

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 this pull request may close these issues.

2 participants