search

kroketio / quart-keycloak

Add Keycloak OpenID Connect support to your Quart application.
BSD 3-Clause "New" or "Revised" License
12 stars 4 forks source link

Fix token 'aud' verification logic #9

Open AliMickey opened 7 months ago

AliMickey commented 7 months ago

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

kroketio commented 6 months ago

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 commented 6 months ago

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.