feat!: add TaskAuthorizationProvider SPI for per-user task authorization#935
feat!: add TaskAuthorizationProvider SPI for per-user task authorization#935kabir wants to merge 1 commit into
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 introduces an opt-in SPI for per-user task authorization (TaskAuthorizationProvider) along with a decorator to enforce read, write, and create permissions across request handlers and task stores. Feedback on these changes highlights a potential IllegalArgumentException in the gRPC call context factories when processing binary metadata headers, an inefficiency in JpaDatabaseTaskStore's iterative pagination query limit, and a security concern regarding information exposure via the pre-authorization task count.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces per-user task authorization to the SDK by adding a new TaskAuthorizationProvider SPI, an AuthorizationRequestHandlerDecorator to intercept requests, and updating TaskStore implementations to support authorization filtering during task listing. It also includes comprehensive integration tests and documentation updates. The review feedback highlights critical compilation errors in both InMemoryTaskStore and JpaDatabaseTaskStore due to unhandled checked exceptions (A2AError) thrown by checkRead inside the list methods. Additionally, it is recommended to annotate the ServerCallContext parameter with @nullable in TaskStore.list and its implementations to ensure null safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ehsavoie
left a comment
ehsavoie
left a comment
There was a problem hiding this comment.
The User interface only has isAuthenticated() and getUsername(). There's no way to get roles or claims. The TaskAuthorizationProvider receives a ServerCallContext so implementers can reach into state to get the raw SecurityIdentity, but this couples the provider to Quarkus internals. A getRoles(): Set method or getAttribute(String): Object would make the interface more useful for role-based authorization without requiring framework coupling. This isn't blocking, but it's a natural next step.
The anonymous User implementations in QuarkusCallContextFactory. Lines 39-49 create an anonymous User implementation inline. This works but makes it impossible to test equality or do instanceof checks. A named record like AuthenticatedUser(String username) would be cleaner and more testable.
| @Override | ||
| public <V> ServerCallContext create(StreamObserver<V> responseObserver) { | ||
| User user; | ||
| if (securityIdentityInstance.isResolvable()) { |
There was a problem hiding this comment.
This looks like a duplicate of QuarkusCallContextFactory maybe we should have a helper method for this
| .thenComparing(Task::id)) | ||
| .toList(); | ||
|
|
||
| if (authorizationProvider != null && context != null) { |
There was a problem hiding this comment.
Why not adding this in the stream filtering above ?
| @Inject | ||
| A2AConfigProvider configProvider; | ||
|
|
||
| private final @org.jspecify.annotations.Nullable TaskAuthorizationProvider authorizationProvider; |
| public TaskPushNotificationConfig onCreateTaskPushNotificationConfig(TaskPushNotificationConfig params, | ||
| ServerCallContext context) throws A2AError { | ||
| String taskId = params.taskId(); | ||
| if (taskId != null) { |
There was a problem hiding this comment.
If taskId is null the task can be created ? Is that correct ?
There was a problem hiding this comment.
No — if taskId is null, the delegate (DefaultRequestHandler) immediately throws InvalidParamsError("taskId is required") at line 793. The null guard in the decorator just avoids calling enforceWrite with a null argument; the request is never allowed through.
| } | ||
| EventKind result = delegate.onMessageSend(params, context); | ||
| String resultTaskId = extractTaskId(result); | ||
| recordOwnershipIfNeeded(context, resultTaskId, TaskOperation.MESSAGE_SEND); |
There was a problem hiding this comment.
TOCTOU race in onMessageSend ownership recording.
If two concurrent onMessageSend calls arrive for the same new task (no taskId in the message), both pass enforceCreate, the delegate creates the task, and both reach recordOwnershipIfNeeded. The isTaskRecorded + recordOwnership sequence isn't atomic. The javadoc says recordOwnership "must be idempotent," which handles the data integrity side, but there's a window where isTaskRecorded returns false for both threads. The TestTaskAuthorizationProvider correctly uses putIfAbsent, so this is safe in practice as long as implementers follow the
idempotency contract. This is documented but worth highlighting in a "common pitfall" section of the interface javadoc.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a per-user task authorization SPI (TaskAuthorizationProvider) along with an intercepting decorator (AuthorizationRequestHandlerDecorator) to enforce access control across gRPC, JSON-RPC, and REST transports, including compatibility layers. While the security architecture is well-designed, several critical issues must be addressed before merging. Specifically, there are compilation failures in both JpaDatabaseTaskStore and InMemoryTaskStore due to unhandled checked exceptions (A2AError) within stream filters and database queries. Additionally, the iterative fetching logic in JpaDatabaseTaskStore is inefficient and poses an infinite loop risk under heavy unauthorized task loads. Finally, potential NullPointerExceptions exist in the JSON-RPC route handlers if the user subject is null when instantiating AuthenticatedUser.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Add an opt-in SPI that lets multi-user deployments control which authenticated user may read, write, or create tasks. When a CDI bean implementing TaskAuthorizationProvider is present the SDK wraps the RequestHandler in an AuthorizationRequestHandlerDecorator that enforces per-operation checks and records task ownership on creation. Key additions: - TaskAuthorizationProvider interface with checkRead/checkWrite/ checkCreate/isTaskRecorded/recordOwnership contract - AuthorizationRequestHandlerDecorator that intercepts every RequestHandler method and delegates to the provider - AuthenticatedUser record replacing anonymous User implementations across all transports (gRPC, JSON-RPC, REST) and compat-0.3 - User.getAttribute() default method for extensible role/claim access - Authorization-aware filtering in InMemoryTaskStore.list() and JpaDatabaseTaskStore.list() with correct pagination - Comprehensive test suite covering all transports, multi-version dispatching, and the decorator itself - Security guidance in README and javadoc: fail-closed ownership policy, TOCTOU race pitfall, CDI injection requirement BREAKING CHANGE: TaskStore.list() signature now includes ServerCallContext to support authorization filtering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Add an opt-in SPI for per-user task authorization, enforced via a CDI decorator around RequestHandler.
BREAKING CHANGE: TaskStore.list() now requires a ServerCallContext parameter. Custom TaskStore implementations must update their list() signature and apply authorization filtering when a TaskAuthorizationProvider is present.