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

RR - use exception mappers on auth failure exceptions for proactive auth #29590

Merged

Conversation

michalvavrik
Copy link
Contributor

@michalvavrik michalvavrik commented Nov 30, 2022

fixes: #29896

This is proposal to enable RESTEasy Reactive exception mappers to handle AuthenticationCompletionException, AuthenticationFailedException, AuthenticationRedirectException, ForbiddenException when quarkus.http.auth.proactive=true. I suggest we weight the arguments and decide whether the change is needed in some form or the PR should be closed.

Pros:

  • user can use exception mappers when quarkus.http.auth.proactive=false
  • it's JAX-RS way that user asked for in the past
  • HTTP path matching policy may lead to 403 that will be customizable via exception mappers (after this PR, not down to proactive=true), currently it's only customizable via failure handlers
  • by accident, above-mentioned already works for RESTEasy Classic since 2.14 (however we can regress it) and classic/reactive behavior should be same

Cons:

  • IMO in reported issues, users seemed complacent about using proactive=false (once Sergey suggested it)
  • custom exception mapper that catches AuthenticationFailedException thrown by OIDC is unlikely to send challenge
  • currenlty users can customize response via failure handlers after the default auth failure handler has send challenge and failed event (iff auth mechanism didn't end response), therefore it's safer

@sberyozkin
Copy link
Member

@michalvavrik Sorry for the delay.
Can you please add a test showing that fault handlers can still be used with the proactive auth enabled if preferred unless we already have the existing test ? I'm not sure why you remove the doc section explaining how to use the fault handlers.
Thanks

@michalvavrik
Copy link
Contributor Author

@sberyozkin sure, I'll think about it and may add tests next week. I removed the docs section as it says with proactive=true the only way to customize response is to use failure handler, which would no more be true. My issue with user defined failure handler when proactive=true is that with RESTEasy Reactive/Classic they would need negative priority and I don't want to advice that to users. I think primary way for RESTEasy Reactive/Classic should be exception mappers and failure handlers are available for special cases or for non-JAX-RS users (Reactive Routes, pure lambdas?).

@michalvavrik
Copy link
Contributor Author

Maybe there is another way, let me think about it, I'm currently on sick leave.

@michalvavrik michalvavrik marked this pull request as draft December 4, 2022 12:08
@sberyozkin
Copy link
Member

sberyozkin commented Dec 4, 2022

@michalvavrik Sorry, take care, no rush at all, lets talk in a few days (I'd consider keeping the doc section but adjusting the advice only when/how to use it - ex, if both basic and code flow are enabled, etc)

@sberyozkin
Copy link
Member

@michalvavrik Hey, so the only question on my part here, is the fault route path still working with this PR (can you link to the test please), and if yes, then my only change request is to keep the info about using fault routes in the docs (with some minor updates - it won't longer be the only option)

@michalvavrik
Copy link
Contributor Author

@michalvavrik Hey, so the only question on my part here, is the fault route path still working with this PR (can you link to the test please), and if yes, then my only change request is to keep the info about using fault routes in the docs (with some minor updates - it won't longer be the only option)

Simply priority adjustment will make failure handlers working again and then I will return docs section & add all relevant tests. I'll do that.

@michalvavrik michalvavrik force-pushed the feature/rr-proactive-auth-ex-mappers branch from 3ff5df2 to 8721761 Compare December 15, 2022 21:17
@michalvavrik michalvavrik marked this pull request as ready for review December 15, 2022 21:19
@michalvavrik
Copy link
Contributor Author

michalvavrik commented Dec 15, 2022

I've added an example and caution note to docs regarding auth mechanism challenge. Together existing test coverage and added tests covers both exception mappers and failure handlers in RESTEasy Reactive and Classic.

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

@michalvavrik Great work, thanks, left a few minor doc suggestions only

@michalvavrik michalvavrik force-pushed the feature/rr-proactive-auth-ex-mappers branch from 8721761 to 8ed165f Compare December 16, 2022 11:06
@michalvavrik
Copy link
Contributor Author

Docs looks better with your comments, thank you.

@sberyozkin sberyozkin self-requested a review December 16, 2022 12:05
Quarkus Documentation automation moved this from To do to Reviewer approved Dec 16, 2022
@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 16, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 16, 2022

Failing Jobs - Building 8ed165f

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.JandexMultiModuleProjectDevModeTest.main line 21 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@sberyozkin sberyozkin merged commit 4083da5 into quarkusio:main Dec 16, 2022
Quarkus Documentation automation moved this from Reviewer approved to Done Dec 16, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 16, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Dec 16, 2022
@michalvavrik michalvavrik deleted the feature/rr-proactive-auth-ex-mappers branch December 16, 2022 17:36
@gsmet gsmet modified the milestones: 2.16 - main, 2.15.1.Final Dec 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.

smallrye jwt 401 cannot be intercepted by ExceptionMapper
3 participants