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

Adjust 'challenge' selection so that custom auth mechanism is called #27921

Conversation

michalvavrik
Copy link
Contributor

@michalvavrik michalvavrik commented Sep 14, 2022

fixes: #27180

This PR makes sure io.quarkus.vertx.http.runtime.security.HttpAuthenticationMechanism#send/getChallenge of a custom auth mechanism get called. There is an example of HttpAuthenticationMechanism Customization that don't work for me as built-in auth mechanisms put into routing context themselves and this information is used when selecting an auth mechanism for challenge, I've explained actual behavior in depth in linked issue. This solution needs check from an expert (@sberyozkin ) as I only inferred "expected behavior" from PRs submitted in this area.

@sberyozkin
Copy link
Member

I'm not sure it will work - for example, we have both Basic and OIDC mechanisms enabled at the same time, with this PR the challenge caused by a failing basic auth can be picked by OIDC or the other way around, depending on the sorting order.
Perhaps the condition should be: if (mechanisms.size() > 1) ?

@sberyozkin
Copy link
Member

sberyozkin commented Sep 14, 2022

And also say in the docs - that your custom authentication mechanism has to add itself to RoutingContext (routingContext.put(HttpAuthenticationMechanism.class.getName(), this);) if more than one authentication mechanism is available to ensure a correct challenge is returned - something along these lines

@michalvavrik michalvavrik force-pushed the feature/custom-auth-mechanism-should-challenge branch from 607e2a2 to 28f6f49 Compare September 14, 2022 12:34
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Sep 14, 2022
@michalvavrik
Copy link
Contributor Author

michalvavrik commented Sep 14, 2022

if (mechanisms.size() > 1) fixes the reproducer only in combination with @Priority(1) @Alternative, that is not a problem in regards of #27180, but I wonder if the auth mechanism selection don't work differently to your expecatation:

Please consider CustomFormAuthenticator without @Priority(1) @Alternative and with routingContext.put(HttpAuthenticationMechanism.class.getName(), this);, if you run such a test, the result depends on the order of mechanisms -> it's random. Sometimes I get 8 times green and then red, sometimes I get couple of failures etc. I think that's because both mechanisms are getting called in io.quarkus.vertx.http.runtime.security.HttpAuthenticator#createSecurityIdentity and who is in the routing context depends on the order.

For that reason, I left out setting the context and adjusted docs so that examples will work. I also fixed some invalid method signatures.

@michalvavrik michalvavrik force-pushed the feature/custom-auth-mechanism-should-challenge branch 2 times, most recently from 052ee77 to b00b1ef Compare September 14, 2022 12:48
@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

Please consider CustomFormAuthenticator without @priority(1) @Alternative and with routingContext.put(HttpAuthenticationMechanism.class.getName(), this);, if you run such a test, the result depends on the order of mechanisms -> it's random

Sure, I think we have 2 variations here
1: a mechanism such as CustomFormAuthenticator needs to replace the default built in form one. - this is the reproducer case.
(there is also HttpAuthenticationMechanism.getPriority which I guess can be used instead)
2. a totally new custom mechanism one, not conflicting with any of the existing ones, is used alongside for ex OIDC - in this case I think this new mechanism needs to set this attribute.

Quarkus Documentation automation moved this from To do to Reviewer approved Sep 14, 2022
@michalvavrik michalvavrik force-pushed the feature/custom-auth-mechanism-should-challenge branch from b00b1ef to 91ab317 Compare September 14, 2022 12:55
@michalvavrik
Copy link
Contributor Author

FYI Now just fixed indentation for docs as originally I had it wrong.

@michalvavrik
Copy link
Contributor Author

I see, it makes sense.

@michalvavrik michalvavrik force-pushed the feature/custom-auth-mechanism-should-challenge branch from 91ab317 to 48d0c87 Compare September 14, 2022 13:50
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 14, 2022

Failing Jobs - Building 48d0c87

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
JVM Tests - JDK 18 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/smallrye-reactive-messaging-amqp/deployment 
! Skipped: integration-tests/reactive-messaging-amqp 

📦 extensions/smallrye-reactive-messaging-amqp/deployment

io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest.test line 30 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

⚙️ JVM Tests - JDK 18 #

- Failing: integration-tests/oidc-code-flow 

📦 integration-tests/oidc-code-flow

io.quarkus.it.keycloak.CodeFlowTest.testTokenAutoRefresh line 519 - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/tenant-autorefresh
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:461)

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Sep 14, 2022

io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest clearly not related (vertx disconnected stuff), io.quarkus.it.keycloak.CodeFlowTest.testTokenAutoRefresh I think goes down to missing PublicKey, but I'll run tests locally.

@michalvavrik
Copy link
Contributor Author

Ah, I can see that's a known issue #27900

@sberyozkin sberyozkin merged commit 4a2a5cb into quarkusio:main Sep 14, 2022
Quarkus Documentation automation moved this from Reviewer approved to Done Sep 14, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 14, 2022
@michalvavrik michalvavrik deleted the feature/custom-auth-mechanism-should-challenge branch September 14, 2022 22:28
@gsmet gsmet removed this from the 2.14 - main milestone Sep 20, 2022
@gsmet gsmet added this to the 2.13.0.Final milestone Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Unable to Overwrite Response using Security-JPA & RestEasy-Classic
3 participants