feat!: add TaskAuthorizationProvider SPI for per-user task authorization#935
feat!: add TaskAuthorizationProvider SPI for per-user task authorization#935kabir wants to merge 4 commits into
Conversation
Add an opt-in SPI for per-user task authorization, enforced via a CDI decorator around RequestHandler. - TaskOperation enum identifies which operation triggered the check - TaskAuthorizationProvider interface defines checkRead/checkWrite/ checkCreate/recordOwnership/isTaskRecorded methods - AuthorizationRequestHandlerDecorator (@decorator @priority(50)) wraps RequestHandler, calling the provider before/after each method - For onListTasks, filtering is pushed to TaskStore.list() via a new ServerCallContext parameter - InMemoryTaskStore filters all tasks before pagination (exact results) - JpaDatabaseTaskStore uses an iterative fetch loop to accumulate pageSize authorized results across DB pages (correct page sizes) - Streaming ownership recording wraps the publisher to call recordOwnership on the first task event - QuarkusCallContextFactory propagates Quarkus SecurityIdentity into gRPC ServerCallContext, so authorization checks see the authenticated user. Uses Instance<SecurityIdentity> for optional injection in apps without security configured - Integration tests cover all 3 transports (JSON-RPC, REST, gRPC) across reference, multiversion, and compat-0.3 modules 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. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 ?
| } | ||
| 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.
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.