Included client_assertion_type and client_assertion when other auth m…#714
Conversation
…ethods are not set
|
@pmlopes any chance for you to take a look? |
tsegismont
left a comment
tsegismont
left a comment
There was a problem hiding this comment.
Thank you @pendula95
Is there any possibility to add a test with Keycloak?
…d-together-with-clientassertion-clientassertiontype
pendula95
left a comment
pendula95
left a comment
There was a problem hiding this comment.
I am 100% confident about this change. Looks like client assertion as client authentication method was missed and previous code would not pass needed tests.
I have tried to work around current code to not introduce breaking changes or new api features but as result there are some maybe unexpected behaviors. It might be helpful to have discussion about this changes.
|
|
||
| public OAuth2Options setClientAssertionType(String clientAssertionType) { | ||
| this.clientAssertionType = clientAssertionType; | ||
| this.useBasicAuthorization = false; |
There was a problem hiding this comment.
This is a shortcut that might introduce unexpected behaviors for some users
There was a problem hiding this comment.
I'm sorry but I'm not an expert in that area. Can you elaborate about why this might come unexpected for some users? Especially as it seems you say the current behavior is broken in the other comment
There was a problem hiding this comment.
There are 2 ways how to use assertion. Both defined: https://datatracker.ietf.org/doc/html/rfc7521
This PR fixes incorrect implementation for Using Assertions for Client Authentication
As test were missing that would catch this, it was overlooked. This has been fixed by the PR and I am quite familiar with this.
On the other hand most of the code that is present its goal was to facilitate Using Assertions as Authorization Grants which I have limited knowledge. And looking at the test non of them validate the functionality so I am not 100% sure that something was not broken by this PR.
pmlopes
left a comment
pmlopes
left a comment
There was a problem hiding this comment.
I think this PR is correct and makes sense. Indeed as @pendula95 noticed, the assertion implementation was not considering client authentication. The original code was mainly focused on the authorization grants, so we could support service accounts (how Google like to call it) and On-Behalf-Of (like how Azure likes to call it).
If I remember correctly the extra null checks where there to ensure a smooth interop with Vert.x Web. If this PR passes the existing + new tests and does not introduce any side-effects/regressions in Vertx-Web, it gets my approval 👍
3 participants
Fixes #713