-
Notifications
You must be signed in to change notification settings - Fork 36
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
Implement test for OIDC filtered client #1513
Conversation
@mocenas I think failures are related, can you check please? |
You are right, failure was related. Should be fixed now. |
|
||
@RegisterRestClient | ||
@AccessToken | ||
@RegisterProvider(DefaultTokenRequestFilter.class) |
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 don't like this, because we are loosing only test of @AcessToken
in our all TS / FW. Why can't you add your own client, is it because of what you are explaining on the DefaultTokenRequestFilter
? I will comment there.
* {@link io.quarkus.ts.security.keycloak.oidcclient.extended.restclient.ping.clients.TokenPropagationPongClient} | ||
* It would not be required normally, but having {@link CustomTokenRequestFilter} causes AmbiguousResolutionException when | ||
* getting a default filter. | ||
* So this class is necessary to have unambiguous filter for TokenPropagatingPongClient. |
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.
Sounds like something that could be fixed with qualifiers since we know this at build time. Can you please open issue? IMO this is not expected behavior, because Quarkus knows there is need to use AccessTokenRequestFilter
. The worst case scenario, Quarkus can use annotation transformer and produce this class for you.
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.
Please ping me there, I'd like to have a look at it.
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 agree this behaviour is not OK. I've created an issue quarkusio/quarkus#36994
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.
Thank you
@mocenas I'd prefer to have TODO: LINK_TO_CREATED_ISSUE on DefaultTokenRequestFilter.java, or you can let me know why you disagree. Anyway this PR can be merged, lgtm, thank you. |
I've added link to the issue, also I've ported the test to the reactive module. |
good idea |
@michalvavrik Is it OK to merge this? (can you merge it?, don't forget to squash :-) |
* Implement test for OIDC filtered client Test for quarkusio/quarkus#36459 and quarkusio/quarkus#36501 * Resolve ambiguous accessTokenRequest * Add OIDC FilteredToken test to reactive
* Implement test for OIDC filtered client Test for quarkusio/quarkus#36459 and quarkusio/quarkus#36501 * Resolve ambiguous accessTokenRequest * Add OIDC FilteredToken test to reactive
* Implement test for OIDC filtered client Test for quarkusio/quarkus#36459 and quarkusio/quarkus#36501 * Resolve ambiguous accessTokenRequest * Add OIDC FilteredToken test to reactive
Test for quarkusio/quarkus#36459 and quarkusio/quarkus#36501
Summary
Implement test for selection of exchange OIDC client from multiple clients setup in one app.
Please select the relevant options.
run tests
phrase in comment)Checklist: