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

Use failure handlers for security exceptions before JAX-RS chain starts #28967

Conversation

michalvavrik
Copy link
Contributor

@michalvavrik michalvavrik commented Oct 31, 2022

fixes: #28489
fixes: #28488

Make it possible to customize response when Quarkus Security authentication exceptions are thrown before JAX-RS chain started. That is done by failing event when proactive security is enabled and ensuring default failure handler QuarkusErrorHandler sends response if it wasn't sent downstream. We should provide a way to customize response as users were asking for it and currently it's not possible f.e. when smallrye-jwt receives invalid token to customize response.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik marked this pull request as draft November 1, 2022 06:06
@michalvavrik michalvavrik force-pushed the feature/auth-failure-handling-with-proactive-auth branch from 8678a5e to 1a299fc Compare November 1, 2022 06:24
@michalvavrik michalvavrik marked this pull request as ready for review November 1, 2022 06:25
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 1, 2022

Failing Jobs - Building 1a299fc

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up job ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/opentelemetry/deployment 
! Skipped: integration-tests/micrometer-prometheus integration-tests/opentelemetry integration-tests/opentelemetry-grpc and 5 more

📦 extensions/opentelemetry/deployment

io.quarkus.opentelemetry.deployment.instrumentation.RestClientOpenTelemetryTest.path line 132 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <SERVER> but was: <CLIENT>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @michalvavrik.

Next, we can revisit your idea of allowing to get into the application code to map the error even if the proactive authentication is enabled, and combine it with the generic failure handler approach

Quarkus Documentation automation moved this from To do to Reviewer approved Nov 1, 2022
@sberyozkin sberyozkin merged commit d7ff583 into quarkusio:main Nov 1, 2022
Quarkus Documentation automation moved this from Reviewer approved to Done Nov 1, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 1, 2022
@michalvavrik michalvavrik deleted the feature/auth-failure-handling-with-proactive-auth branch November 1, 2022 20:58
@michalvavrik
Copy link
Contributor Author

michalvavrik commented Nov 1, 2022

Next, we can revisit your idea of allowing to get into the application code to map the error even if the proactive authentication is enabled, and combine it with the generic failure handler approach

For RESTEasy Reactive that would be as simple as removing this boolean https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/ResteasyReactiveRecorder.java#L228. It has no real value except of stopping exception mappers from receiving auth failures when proactive auth is enabled :-) For classic it might be harder, don't know yet. We can wait with next step as there have been a lot of changes in that area (whatever you want); now I have #5751 to deal with and it's strongly related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment