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

Make JsonWebToken available in arbitrarily activated CDI request context in HTTP proxy interceptors #46715

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michalvavrik
Copy link
Member

This PR is motivated by user that uses io.vertx.httpproxy.HttpProxy#reverseProxy(io.vertx.core.http.HttpClient) and their io.vertx.httpproxy.ProxyInterceptor needs to inject org.eclipse.microprofile.jwt.JsonWebToken from CDI request context (reproducer is not publicly available, ask @sberyozkin for details if you require them). There is no RoutingContext available, only io.vertx.httpproxy.ProxyContext that doesn't expose underlying request's RoutingContext.

Why doesn't it work OOTB? Basically what is happening is that injecting org.eclipse.microprofile.jwt.JsonWebToken inside a Handler<RoutingContext> requires SecurityIdentity taken from the CDI request context. But when user activates CDI request context with the ActivateRequestContext annotation (or programmatically), where would the request scoped bean take the identity if it cannot access RoutingContext with the HTTP user?

Only way how to propagate RoutingContext is IMO local data of the Vert.x duplicated context. I know we don't want to do that by default because it relies on the ConcurrentHashMap which would be bad when used on the hoth path, but I think it can be justified in this situation when explicitly enabled.

Key part of the original reproducer looked like:

final HttpClient httpClient = vertx.createHttpClient(httpClientOptions);
final HttpProxy proxyHandler = HttpProxy.reverseProxy(httpClient);
proxyHandler.origin(uri.getPort(), uri.getHost());
proxyHandler.addInterceptor(myInterceptor);
router.route("my-path").handler(ctx -> {
proxyHandler.handle(ctx.request());
});

and myInterceptor is @ApplicationScoped bean that injects JsonWebToken.

@michalvavrik michalvavrik requested a review from sberyozkin March 10, 2025 23:49
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/security area/vertx labels Mar 10, 2025

This comment has been minimized.

Copy link

github-actions bot commented Mar 11, 2025

🎊 PR Preview 7843777 has been successfully built and deployed to https://quarkus-pr-main-46715-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

This comment has been minimized.

@sberyozkin
Copy link
Member

Thanks Michal, I'll have a look a bit later, quick question, how does it work for reactive routes?

@michalvavrik
Copy link
Member Author

michalvavrik commented Mar 11, 2025

Thanks Michal, I'll have a look a bit later, quick question, how does it work for reactive routes?

It works for everything based on Vert.x HTTP (including Reactive Routes), but IIRC in the Reactive Routes we activate CDI request context for endpoints created with annotations and if Quarkus activates the context itself, so we can add the identity to the context when we activate the context. This change shouldn't have negative impact in terms of functionality. I would actually do this by default if I could, it would make things more reliable, but I think we want to avoid adding stuff to local data when we don't have to because the concurrent hash map uses synchronized block when inserting stuff. But it is not like this PR does something unusual either (CDI request state is stored in local data, OpenTelemetry has context storage that relies on it etc.).

@sberyozkin
Copy link
Member

Thanks @michalvavrik, LGTM, definitely worth supporting the injection into arbitrary handlers registered directly with the router, all the suggestions are really about trying to make it very specific to a concrete combination (handlers registered directly on the router), I'd rather us expanding it with some more details later

@michalvavrik michalvavrik force-pushed the feature/propagate-routing-ctx-in-dupl-ctx-local-data branch from 5fb31a8 to b1443da Compare March 11, 2025 14:25

This comment has been minimized.

@sberyozkin sberyozkin self-requested a review March 11, 2025 15:00
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, very useful to have the identity injection working for handlers directly attached to the router.
Michal, if possible, please move the text to Security Tips and Tricks, may be we can consolidate there (later) all the tips related to the request scope management.

I'd also appreciate if @cescoffier could double check

@sberyozkin sberyozkin requested a review from cescoffier March 11, 2025 15:03
@michalvavrik michalvavrik force-pushed the feature/propagate-routing-ctx-in-dupl-ctx-local-data branch from b1443da to 94d4ac6 Compare March 11, 2025 15:28
Copy link

quarkus-bot bot commented Mar 11, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 94d4ac6.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Mar 11, 2025

Status for workflow Quarkus CI

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

✅ 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 Windows

📦 extensions/micrometer-opentelemetry/deployment

io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_Uni - History

  • Stream has no elements - java.lang.IllegalArgumentException
java.lang.IllegalArgumentException: Stream has no elements
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lambda$lastReading$2(MetricDataFilter.java:213)
	at java.base/java.util.Optional.orElseThrow(Optional.java:403)
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReading(MetricDataFilter.java:213)
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReadingDataPoint(MetricDataFilter.java:231)
	at io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_Uni(MicrometerTimedInterceptorTest.java:174)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)

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

Successfully merging this pull request may close these issues.

2 participants