Fix registration of backchannel logout route #43000

AB-xdev · 2024-09-04T06:21:33Z

Disclaimer I couldn't build it locally / had no time to setup the 22 DIN A4 pages of the contribution guide ;)

Fixes: logout.backchannel.path fails when http.root-path is present - Again #42990

Currently the backchannel logout route is registered with the root-path, resulting in a path like this: ``/backend/backend/back-channel-logout`` instead of ``/backend/back-channel-logout`` See also #42990

sberyozkin · 2024-09-04T07:53:35Z

Thanks @AB-xdev, I'll look now at confirming with the test that this route is accessible with the root path configured, I avoided it previous time trying to minimize impact on all the surrounding tests in the test suite, but I'll isolate it now to a dedicated test

sberyozkin · 2024-09-04T09:32:00Z

@AB-xdev

had no time to setup the 22 DIN A4 pages of the contribution guide

What that is about ? And how is it relevant to the issue at hand ?

gsmet · 2024-09-04T09:48:10Z

I think it was just a gripe about our contributing guide being too long :).

That's actually very accurate and we started some work to have a smaller first contribution guide. Looks like I need to get back to it :).

sberyozkin · 2024-09-04T09:52:46Z

I'm starting looking at adding a back channel test with the http root set

sberyozkin · 2024-09-04T16:07:53Z

After spending a lot of time trying to add a dedicated test in extensions/oidc-deployment only, I'm resorting to adding integration-tests/oidc-wiremock-logout. It appears there are some test class loading issues which are not visible because all the tests there now use the same realm.
When a wiremock test is added, the properties like quarkus.oidc.auth-server-url registered by other tests impact the wiremock test...

sberyozkin · 2024-09-04T17:26:25Z

Hi @gsmet @cescoffier I added a test. FYI, if I the route path is relative, example, quarkus.oidc.logout.backchannel.path=back-channel-logout, then, even with quarkus.http.root-path=/service, the route registration fails.

Only having both quarkus.http.root-path=/service and quarkus.oidc.logout.backchannel.path=/service works.

It also took me a bit of time to realize RestAuured integration will prepend the root path itself for non-absolute URLs :-)

cescoffier · 2024-09-04T17:34:34Z

Why do you need to repeat the root path? Is the route registered correctly?

cescoffier · 2024-09-04T17:36:09Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/BackChannelLogoutHandler.java

@@ -46,7 +46,7 @@ public void setup(@Observes Router router) {

    private void addRoute(Router router, OidcTenantConfig oidcTenantConfig) {
        if (oidcTenantConfig.isTenantEnabled() && oidcTenantConfig.logout.backchannel.path.isPresent()) {
-            router.route(getRootPath() + oidcTenantConfig.logout.backchannel.path.get())
+            router.route(oidcTenantConfig.logout.backchannel.path.get())


Who is calling this method? Which router is it? Why it's not a route registered in the processor?

Hi @cescoffier

Who is calling this method?

It is called from just above from setup(@Observes Router router) , here.

Which router is it?

The main application one, since it works when accessed from the tests and for example, from Keycloak.

Why it's not a route registered in the processor?

Do you mean the recorder, similar to how it is done WebAuthnRecorder ?

I don't recall now, probably I did not know how to do it at a time, and chose setup(@Observers Router router), or may be because the way it is done now is easier to handle in a multi-tenant setup. I can certainly rework it and move the registration to the recorder. I guess, to have this PR backportable, I should do it as a follow up PR a bit later, does it sound reasonable ?

Yeah, please let's try to get the simple version of this PR verified, tested and merged.

I need a backportable fix very soon.

quarkus-bot · 2024-09-04T20:02:46Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 2288830.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.testOTelInjections - History

Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.reset(OpenTelemetryInjectionsTest.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.testOTelInjections - History

Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.reset(OpenTelemetryInjectionsTest.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

sberyozkin · 2024-09-05T09:34:25Z

@cescoffier

Why do you need to repeat the root path? Is the route registered correctly?

Do you mean why the route was registered with the explicit root path ? It was a mistake, the PR from @AB-xdev fixes it, or do you mean something else ?

cescoffier · 2024-09-05T11:21:21Z

Ok, so it's an applicatoin router receiving the application router. For sure you must not prefix the root path.

I think we would need to switch to a route declared in the extension instead of this, but as you said, for backport reason, let's go with this.

sberyozkin · 2024-09-05T11:37:19Z

Sure, @cescoffier, I'll open a follow up request, it might take a bit of time to handle that, but I'll optimize, thanks. By the way, integration-tests/oidc-wiremock has exactly the same test, but without the quarkus.http.root-path configured, where / is a default, so both cases are covered

sberyozkin · 2024-09-05T11:39:27Z

Merging now, @AB-xdev, thanks again for initiating this PR 👍 , we look forward for more :-)

Fix registration of backchannel logout route

f92fc56

Currently the backchannel logout route is registered with the root-path, resulting in a path like this: ``/backend/backend/back-channel-logout`` instead of ``/backend/back-channel-logout`` See also #42990

quarkus-bot bot added the area/oidc label Sep 4, 2024

AB-xdev mentioned this pull request Sep 4, 2024

logout.backchannel.path fails when http.root-path is present - Again #42990

Closed

gsmet added the triage/backport-3.14 label Sep 4, 2024

sberyozkin force-pushed the 42990-logout-backchannel-with-root-path branch from c8a7a4f to d58dc45 Compare September 4, 2024 17:21

sberyozkin requested review from gsmet and cescoffier September 4, 2024 17:26

sberyozkin force-pushed the 42990-logout-backchannel-with-root-path branch from d58dc45 to 9f32092 Compare September 4, 2024 17:27

Add Back channel logout test with quarkus.http.root-path

2288830

sberyozkin force-pushed the 42990-logout-backchannel-with-root-path branch from 9f32092 to 2288830 Compare September 4, 2024 17:28

cescoffier reviewed Sep 4, 2024

View reviewed changes

quarkus-bot bot added the triage/flaky-test label Sep 4, 2024

cescoffier approved these changes Sep 5, 2024

View reviewed changes

sberyozkin merged commit ffded0b into quarkusio:main Sep 5, 2024
50 checks passed

quarkus-bot bot added this to the 3.16 - main milestone Sep 5, 2024

quarkus-bot bot added the kind/bugfix label Sep 5, 2024

AB-xdev deleted the 42990-logout-backchannel-with-root-path branch September 5, 2024 14:32

gsmet removed this from the 3.16 - main milestone Sep 6, 2024

gsmet added this to the 3.14.3 milestone Sep 6, 2024

gsmet removed the triage/backport-3.14 label Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix registration of backchannel logout route #43000

Fix registration of backchannel logout route #43000

AB-xdev commented Sep 4, 2024 •

edited by geoand

Loading

sberyozkin commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

gsmet commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

cescoffier commented Sep 4, 2024

cescoffier Sep 4, 2024

sberyozkin Sep 5, 2024

gsmet Sep 5, 2024

quarkus-bot bot commented Sep 4, 2024 •

edited by github-actions bot

Loading

sberyozkin commented Sep 5, 2024

cescoffier commented Sep 5, 2024

sberyozkin commented Sep 5, 2024 •

edited

Loading

sberyozkin commented Sep 5, 2024

Fix registration of backchannel logout route #43000

Fix registration of backchannel logout route #43000

Conversation

AB-xdev commented Sep 4, 2024 • edited by geoand Loading

sberyozkin commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

gsmet commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

sberyozkin commented Sep 4, 2024

cescoffier commented Sep 4, 2024

cescoffier Sep 4, 2024

Choose a reason for hiding this comment

sberyozkin Sep 5, 2024

Choose a reason for hiding this comment

gsmet Sep 5, 2024

Choose a reason for hiding this comment

quarkus-bot bot commented Sep 4, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/opentelemetry

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

sberyozkin commented Sep 5, 2024

cescoffier commented Sep 5, 2024

sberyozkin commented Sep 5, 2024 • edited Loading

sberyozkin commented Sep 5, 2024

AB-xdev commented Sep 4, 2024 •

edited by geoand

Loading

quarkus-bot bot commented Sep 4, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

sberyozkin commented Sep 5, 2024 •

edited

Loading