feat: Migrate Reactive Routes to Vert.X Web and add security tests#838
feat: Migrate Reactive Routes to Vert.X Web and add security tests#838kabir wants to merge 1 commit intoa2aproject:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a significant architectural shift by migrating from Quarkus Reactive Routes to the programmatic Vert.x Web Router across several modules, including the JSON-RPC and REST reference servers. It introduces a VertxSecurityHelper to bridge Quarkus security with these raw Vert.x routes and adds comprehensive authentication tests for gRPC, JSON-RPC, and REST transports. Additionally, the AuthInterceptor now supports Basic authentication, and the JDK HTTP client includes explicit error handling for unauthorized and forbidden responses. Feedback suggests aligning error codes with the A2A specification for discovery endpoints and refactoring duplicated authentication error handling logic into a centralized handler.
| } catch (JsonProcessingException e) { | ||
| ctx.response().setStatusCode(500).end("Internal Server Error"); | ||
| } |
There was a problem hiding this comment.
The error handling for JSON processing exceptions should be consistent with the A2A protocol specification, which typically maps such errors to a 400 Bad Request or a specific JSON-RPC error code rather than a 500 Internal Server Error.
References
- Adhere to the A2A protocol specification for error code mappings, even if other mappings seem more internally consistent.
There was a problem hiding this comment.
@gemini-code-assist With @route, the method's return value (String) was automatically sent as the response body by the framework. If JsonProcessingException was
thrown, Quarkus Reactive Routes would have returned it as a 500 with the framework's default error handling — so the current 500 behavior is actually consistent with the old behavior.
The JsonProcessingException here can only happen when serializing the AgentCard to JSON (not parsing incoming data), so it's genuinely an
internal server error — the server failed to serialize its own data. A 400 would be wrong here since the client didn't send anything invalid.
Gemini's suggestion is incorrect in this context.
There was a problem hiding this comment.
You are correct, kabir. In this specific context, where the JsonProcessingException occurs during the serialization of the AgentCard (an internal server operation) rather than during the parsing of client-provided input, a 500 Internal Server Error is the appropriate response. Thank you for clarifying the context of this exception.
ehsavoie
left a comment
ehsavoie
left a comment
There was a problem hiding this comment.
TestIdentityProvider is duplicated in reference/jsonrpc and reference/rest. It belongs in tests/server-common alongside AbstractA2AServerWithAuthTest.
AuthTestProfile is duplicated across all three modules with near-identical content. Shared config should move to tests/server-common.
| * @throws RuntimeException if the task throws an exception | ||
| */ | ||
| public void runInRequestContext(RoutingContext ctx, Runnable task) { | ||
| ManagedContext requestContext = Arc.container().requestContext(); |
There was a problem hiding this comment.
We should protect the event loop thread:
if (Context.isOnEventLoopThread()) {
throw new IllegalStateException("Cannot perform blocking authentication on event loop thread. Use blockingHandler().");
}
| } else { | ||
| ctx.response() | ||
| .setStatusCode(401) | ||
| .putHeader("WWW-Authenticate", "Basic realm=\"Quarkus\"") |
There was a problem hiding this comment.
Shouldn't the authentication realm be configurable ??
2 participants
No description provided.