Fix AuthenticationRedirectException handling with disabled proactive security

[michalvavrik]

fixes io.quarkus.it.keycloak.CodeFlowTest.testRPInitiatedLogout; follow-up for #28539 and #29228

Code flow fails at testRPInitiatedLogout as event is failed on HTTP route not created by RESTEasy classic (reactive would fail for same reason). I checked all usages of default auth failure handler and it's not used from failure route (invoked from within failure handler), therefore if event is failed, we can't call next (it would skip one failure handler) neither we can't call end() (as then failed event is never passed to failure handler). For that reason I removed ending event in default auth failure handler, which is de facto same as when default auth failure handler is null (nothing happens); the handler is nullable and always called like this:

@Override
public void onFailure(Throwable failure) {
    BiConsumer<RoutingContext, Throwable> handler = routingContext
            .get(QuarkusHttpUser.AUTH_FAILURE_HANDLER);
    if (handler != null) {
        handler.accept(routingContext, failure);
    }
}

This PR:

• ensures failure handlers that handles auth security exceptions is applied on all routes failures
• takes into consideration that event could be failed already

[michalvavrik]

@sberyozkin I am truly sorry I missed this scenario before; even now I have very hard time to reproduce this without the test mentioned above. Fix needs to be backported to 2.14 because of RR.

[sberyozkin]

@michalvavrik Np, thanks for the PR, is CodeFlowTest#testRPInitiatedLogout is failing now ? Other PRs are not seemed to be affected ?

[michalvavrik]

@sberyozkin CodeFlowTest#testRPInitiatedLogout is failing locally without this PR, this PR should be correct way (as spent as much time investigating it as necessary). If other PRs are not affected by this, only explanation would be they are not rebased on the latest main.

[sberyozkin]

@michalvavrik Hi Michal, there is some test instability in this PR, they do not look like to be related but please rebase just in case

[michalvavrik]

@sberyozkin I was just doing it (literally :-D), 1.Windows is not related, JDK 17 should be re-run by someone who has right to re-trigger it (all I could do is push re-base or smth)

[sberyozkin]

It looks good to me, but Michal, lets just wait till tomorrow, Stuart, @stuartwdouglas, please double check, thanks

[quarkus-bot]

Failing Jobs - Building f5a6fa4

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 17	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 17 MacOS M1	Build	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 18
✔️	Maven Tests - JDK 11
✖	Maven Tests - JDK 11 Windows	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: integration-tests/kubernetes/maven-invoker-way integration-tests/mailer integration-tests/smallrye-opentracing and 1 more

📦 integration-tests/kubernetes/maven-invoker-way

✖ Failed to execute goal org.apache.maven.plugins:maven-invoker-plugin:3.2.2:run (integration-tests) on project quarkus-integration-test-kubernetes-invoker: Could not create temporary file for invoker settings.xml

📦 integration-tests/mailer

✖ io.quarkus.it.mailer.MailerTest.sendHtmlEmail - More details - Source on GitHub

java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:625)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:696)

📦 integration-tests/smallrye-opentracing

✖ io.quarkus.it.opentracing.OpenTracingTestCase. line 35 - More details - Source on GitHub

java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration
	at org.testcontainers.dockerclient.DockerClientProviderStrategy.lambda$getFirstValidStrategy$7(DockerClientProviderStrategy.java:256)
	at java.base/java.util.Optional.orElseThrow(Optional.java:403)

📦 integration-tests/spring-web

✖ io.quarkus.it.spring.web.openapi.SwaggerAndOpenAPIWithCommonPrefixPMT.shouldWorkEvenWithCommonPrefix line 28 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

---

⚙️ Maven Tests - JDK 11 Windows #

- Failing: integration-tests/maven 

📦 integration-tests/maven

✖ io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError line 102 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testCustomManifestAttributes line 162 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testModulesInProfiles line 145 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testModuleWithOverriddenBuildProfile line 121 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testModuleWithBuildProfileInProperty line 114 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testMultiBuildMode line 128 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testConditionalDependencies line 63 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError line 102 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testCustomManifestAttributes line 162 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testModulesInProfiles line 145 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testModuleWithOverriddenBuildProfile line 121 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testModuleWithBuildProfileInProperty line 114 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testMultiBuildMode line 128 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testConditionalDependencies line 63 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

[michalvavrik]

Hey Sergey,

I inspected failures and It looks like issue with concrete tests on MacOS and Windows to me, can't see any relation to the PR, but if you have any doubt at all, I can rebase PR on current main (it has been a day, maybe someone fixed it...).

[sberyozkin]

Hi @michalvavrik Thanks, and now that we have 2 approvals, lets merge :-)

[michalvavrik]

I'll add backport label as #29228 got backported and this fix is needed.

[gsmet]

This does not apply cleanly due to various conflicts. @michalvavrik could you check if we need to backport something else or if we need a specific PR for the backport?

I plan to release 2.14.3.Final on Tuesday so we would need the situation sorted out (and merged in 2.14) by Monday evening.

[michalvavrik]

@gsmet conflict is there because #28967 wasn't backported, but that also means the issue with RP initiated logout is not there etc. I also verified it with the test, therefore the backport is not needed. Thanks

[michalvavrik]

now that I enabled disabled test, I can see different issue (circular reference in RP initiated logout). I don't think it's good idea to have backported #29228 without #28967 as #28967 preceded #29228. Please @gsmet backport both and behavior will be as intended.