luigidemasi commented on code in PR #27435:
URL: https://github.com/apache/camel/pull/27435#discussion_r4194419752
##########
components/camel-undertow/src/main/java/org/apache/camel/component/undertow/DefaultUndertowHost.java:
##########
@@ -165,7 +175,7 @@ public HttpHandler registerHandler(
private Undertow registerHandler(UndertowConsumer consumer,
Undertow.Builder builder, HttpHandler handler) {
UndertowSecurityProvider securityProvider = consumer == null
- ? null
+ ? this.securityProvider
: consumer.getEndpoint().getComponent().getSecurityProvider()
!= null
?
consumer.getEndpoint().getComponent().getSecurityProvider()
: consumer.getEndpoint().getSecurityProvider();
Review Comment:
[P2] Include the producer endpoint's provider when deciding on servlet
context
When a WebSocket producer registers first, `consumer` is null and this
selects only the component's provider. A producer-only endpoint configured with
`securityProvider=#provider` (or endpoint `securityConfiguration`) can have a
provider whose `requireServletContext()` is true while the component provider
is null. The server then starts without a servlet context, but the new upgrade
path calls that endpoint provider's `authenticate()`. Providers such as
`SpringSecurityProvider` dereference `ServletRequestContext` and fail with HTTP
500. The same provider configured at component scope receives a servlet context
and succeeds.
Please pass the producer endpoint's servlet requirements through
registration before starting the host, and cover endpoint-scoped configuration
in the regression tests. `ProviderWithServletWebSocketTest` currently
configures the provider only on the component.
_AI-generated review by Codex on behalf of luigidemasi._
##########
components/camel-undertow/src/main/java/org/apache/camel/component/undertow/handlers/CamelWebSocketHandler.java:
##########
@@ -142,37 +169,127 @@ private static void send(
*/
@Override
public void handleRequest(HttpServerExchange exchange) throws Exception {
- UndertowConsumer currentConsumer = getConsumer();
- if (currentConsumer != null &&
currentConsumer.getEndpoint().getOauthHttpSecurity() != null) {
- if (exchange.isInIoThread()) {
- exchange.dispatch(this);
- return;
+ entryHandler.handleRequest(exchange);
+ }
+
+ /**
+ * The handler that applies the security settings of the path to an
upgrade request and then performs the WebSocket
+ * handshake. A consumer can run its own handlers, such as its access log,
before it.
+ */
+ public HttpHandler getUpgradeHandler() {
+ return upgradeHandler;
+ }
+
+ /**
+ * Lets the given security provider wrap this handler, as it wraps the
handlers of HTTP endpoints. The handler stays
+ * registered as is, so that it remains shared by the consumer and the
producers of the path.
+ */
+ public void wrapWith(UndertowSecurityProvider securityProvider) throws
Exception {
+ HttpHandler wrapped =
securityProvider.wrapHttpHandler(consumerRequestHandler);
+ // a provider that returns no handler disables the path, as it does
for HTTP endpoints
+ this.entryHandler = wrapped != null ? wrapped :
ResponseCodeHandler.HANDLE_405;
+ }
+
+ private void handleConsumerRequest(HttpServerExchange exchange) throws
Exception {
+ consumerHandler.handleRequest(exchange);
+ }
+
+ private void upgrade(HttpServerExchange exchange) throws Exception {
+ List<UndertowEndpoint> endpoints = securityEndpoints();
+ if
(endpoints.stream().noneMatch(CamelWebSocketHandler::hasSecurityChecks)) {
+ this.delegate.handleRequest(exchange);
+ return;
+ }
+ if (exchange.isInIoThread()) {
+ exchange.dispatch(upgradeHandler);
+ return;
+ }
+ Set<String> authenticatedEndpoints = new HashSet<>();
+ Map<String, Object> headers = new HashMap<>();
+ for (UndertowEndpoint endpoint : endpoints) {
+ OAuthHttpSecuritySupport oauthHttpSecurity =
endpoint.getOauthHttpSecurity();
+ if (oauthHttpSecurity != null) {
+ Validation validation =
oauthHttpSecurity.validate(endpoint.getCamelContext(),
authorizationHeaders(exchange));
+ exchange.getRequestHeaders().remove(Headers.AUTHORIZATION);
+ if (!validation.isAuthenticated()) {
+ reject(exchange, validation);
+ return;
+ }
+
exchange.putAttachment(OAUTH_TOKEN_VALIDATION_RESULT_ATTACHMENT,
validation.getValidationResult());
}
- OAuthHttpSecuritySupport oauthHttpSecurity =
currentConsumer.getEndpoint().getOauthHttpSecurity();
- Validation validation =
oauthHttpSecurity.validate(currentConsumer.getEndpoint().getCamelContext(),
- authorizationHeaders(exchange));
- exchange.getRequestHeaders().remove(Headers.AUTHORIZATION);
- if (!validation.isAuthenticated()) {
- reject(exchange, validation);
- return;
+ if (endpoint.requiresAuthentication()) {
+ int statusCode = endpoint.authenticate(exchange);
+ if (statusCode != StatusCodes.OK) {
+ exchange.setStatusCode(statusCode);
+ exchange.endExchange();
+ return;
+ }
+ authenticatedEndpoints.add(endpoint.getEndpointUri());
+ if (endpoint.getSecurityProvider() != null) {
+ endpoint.getSecurityProvider().addHeader(headers::put,
exchange);
+ }
}
- exchange.putAttachment(OAUTH_TOKEN_VALIDATION_RESULT_ATTACHMENT,
validation.getValidationResult());
+ }
+ if (!authenticatedEndpoints.isEmpty()) {
+ exchange.putAttachment(SECURITY_PROVIDER_RESULT_ATTACHMENT,
+ new SecurityProviderResult(authenticatedEndpoints,
headers));
}
this.delegate.handleRequest(exchange);
}
- private UndertowConsumer getConsumer() {
+ /**
+ * The endpoints whose security settings apply to the path: the
consumer's, also while it is stopped, otherwise the
+ * producers'. All the Camel endpoints of a path share its WebSocket
connections.
+ */
+ private List<UndertowEndpoint> securityEndpoints() {
consumerLock.lock();
try {
- return consumer;
+ if (consumerEndpoint != null) {
+ return List.of(consumerEndpoint);
+ }
} finally {
consumerLock.unlock();
}
+ return producerEndpoints.stream().distinct().toList();
}
- private boolean requiresOAuth() {
- UndertowConsumer currentConsumer = getConsumer();
- return currentConsumer != null &&
currentConsumer.getEndpoint().getOauthHttpSecurity() != null;
+ private static boolean hasSecurityChecks(UndertowEndpoint endpoint) {
+ return endpoint.getOauthHttpSecurity() != null ||
endpoint.requiresAuthentication();
+ }
+
+ private static boolean isAuthenticated(WebSocketChannel channel,
List<UndertowEndpoint> endpoints) {
+ for (UndertowEndpoint endpoint : endpoints) {
+ if (!isAuthenticated(channel, endpoint)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Whether the handshake of the given channel passed the security checks
of the given endpoint: its OAuth
+ * validation, its security provider and its allowed roles. Always {@code
true} for an endpoint without such checks.
+ */
+ public static boolean isAuthenticated(WebSocketChannel channel,
UndertowEndpoint endpoint) {
+ if (endpoint.getOauthHttpSecurity() != null && (channel == null
+ || !(channel.getAttribute(
+
OAuthHttpSecuritySupport.OAUTH_TOKEN_VALIDATION_RESULT) instanceof
OAuthTokenValidationResult))) {
+ return false;
+ }
+ if (endpoint.requiresAuthentication()) {
+ return channel != null
+ && channel.getAttribute(SECURITY_PROVIDER_RESULT)
instanceof SecurityProviderResult result
+ && result.endpointUris.contains(endpoint.getEndpointUri());
+ }
+ return true;
Review Comment:
[P1] Account for custom authentication handlers when checking existing
channels
This returns `true` when the endpoint has neither `oauthProfile` nor a
provider/allowed roles, even if its `handlers` chain performs authentication. A
client can connect while only an unsecured producer owns the path, then
continue sending messages to the route and receiving broadcasts after a
consumer with `handlers=#basicAuth` starts. New unauthenticated upgrade
requests correctly receive 401, but the existing connection never passed that
handler chain and is still accepted here.
Please record whether a channel passed the applicable consumer handler chain
and reject channels that did not when that consumer starts, covering both
inbound delivery and outbound sends. Add a regression case alongside the
existing late-consumer test using `handlers=#basicAuth`. This is a gap in the
new handler enforcement; previously WebSocket endpoints ignored these handlers
entirely.
_AI-generated review by Codex on behalf of luigidemasi._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
