-
Notifications
You must be signed in to change notification settings - Fork 3k
Simplify OIDC client exchange grant configuration #48914
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
Simplify OIDC client exchange grant configuration #48914
Conversation
Status for workflow
|
} | ||
if (oidcConfig.grant().type() == Grant.Type.EXCHANGE | ||
&& !tokenGrantParams.contains(OidcConstants.EXCHANGE_GRANT_SUBJECT_TOKEN_TYPE)) { | ||
tokenGrantParams.add(OidcConstants.EXCHANGE_GRANT_SUBJECT_TOKEN_TYPE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 questions:
- So you preset
subject_token_type
for theexchange
grant type. Should you check grant params that there issubject_token
? (likeio.quarkus.oidc.client.OidcClient#getTokens(java.util.Map<java.lang.String,java.lang.String>)
is called with it fromio.quarkus.oidc.token.propagation.reactive.AccessTokenRequestReactiveFilter#exchangeTokenProperty
- It is not documented as a default (not sure where would we document though), are there scenarios when this configuration was set and are broken now:
quarkus.oidc-client.exchange-grant.auth-server-url=${keycloak.url}
quarkus.oidc-client.exchange-grant.discovery-enabled=false
quarkus.oidc-client.exchange-grant.token-path=/tokens-exchange
quarkus.oidc-client.exchange-grant.grant.type=exchange
quarkus.oidc-client.exchange-grant.client-id=quarkus-app
quarkus.oidc-client.exchange-grant.credentials.secret=secret
I just wonder if there were scenarios when quarkus.oidc-client.grant-options.exchange.subject_token_type
previously wasn't set and are now affected by having this preset, can this have negative impact as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michalvavrik, sure,
So you preset subject_token_type for the exchange grant type. Should you check grant params that there is subject_token? (like io.quarkus.oidc.client.OidcClient#getTokens(java.util.Map<java.lang.String,java.lang.String>) is called with it from io.quarkus.oidc.token.propagation.reactive.AccessTokenRequestReactiveFilter#exchangeTokenProperty
This is a required parameter, irrespectively of what the expected token type is set to, I can add a check that subject_token
is set if the grant is exchange
?
I just wonder if there were scenarios when quarkus.oidc-client.grant-options.exchange.subject_token_type previously wasn't set and are now affected by having this preset, can this have negative impact as well?
I believe it is highly unlikely, this is a required parameter and this grant has not been tested properly as it was not possible to enable it with Keycloak admin api and as far as I recall it was not even possible to save a custom realm with the grant enabled directly in Keycloak - it might have changed with the very latest 26.3.0...
So, users had to set this property manually, and this code respects the user's choice - perhaps I can test that if the user sets some other type - that type is preserved ? What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if users set some other type, this code would detect it. After reading the https://datatracker.ietf.org/doc/html/rfc8693#name-request that you linked, I think this PR is not dangerous and it won't replace users value. Let's do it, it is useful. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @michalvavrik, I'll do another iteration at some point with this grant type and do some more testing and polishing.
Indeed, here, the goal is not to ask users to type this property, it is a rather advanced case, I've never heard myself of any other cases but where the incoming access token is exchanged, it should be not even 90% but 95%+ case :-), so letting users not to type the technical property is the least we can do for them :-)
I've been working on a demo that requires a use of the OIDC client exchange grant, and I realized I had to configure client like this:
Here,
quarkus.oidc-client.grant-options.exchange.subject_token_type=urn:ietf:params:oauth:token-type:access_token
can definitely be avoided, it is very technical, users will still be able to set this property if they want some different token type, but in 90% cases it will be this specific type, and users should not need to worry about having to add this property