security: add authorization checks to OperationHttpHandler#16157
security: add authorization checks to OperationHttpHandler#16157adilburaksen wants to merge 1 commit into
Conversation
The operation endpoints under /v3/namespaces/{namespace-id}/operations
listed operation runs, fetched an operation run by id and stopped an
operation run for the namespace supplied in the request path without
enforcing any access control on that namespace (the handler carried an
explicit TODO to add an RBAC check). A caller could therefore list or
read operation runs of, or stop operations in, namespaces it is not
authorized to access.
Enforce access on the requested NamespaceId via the injected
ContextAccessEnforcer before each operation: StandardPermission.GET for
the scan and get endpoints and StandardPermission.UPDATE for the stop
endpoint, mirroring the namespace-level enforcement performed by the
source control management service.
![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 integrates role-based access control (RBAC) checks into the OperationHttpHandler by injecting ContextAccessEnforcer and enforcing standard permissions (GET and UPDATE) across several endpoints. However, the enforce method throws a checked AccessException which is currently unhandled. The review comments correctly point out that AccessException must be imported and added to the throws signature of the scanOperations, getOperationRun, and failOperation methods to avoid compilation failures.
| import io.cdap.cdap.proto.security.StandardPermission; | ||
| import io.cdap.cdap.security.spi.authorization.ContextAccessEnforcer; |
There was a problem hiding this comment.
The ContextAccessEnforcer.enforce method throws AccessException, which is a checked exception. To use it, we need to import io.cdap.cdap.api.security.AccessException.
| import io.cdap.cdap.proto.security.StandardPermission; | |
| import io.cdap.cdap.security.spi.authorization.ContextAccessEnforcer; | |
| import io.cdap.cdap.api.security.AccessException; | |
| import io.cdap.cdap.proto.security.StandardPermission; | |
| import io.cdap.cdap.security.spi.authorization.ContextAccessEnforcer; |
| @@ -100,6 +102,7 @@ public void scanOperations( | |||
| throws BadRequestException, IOException, ForbiddenException { | |||
There was a problem hiding this comment.
The contextAccessEnforcer.enforce call throws AccessException, which is a checked exception. Since it is not caught, it must be declared in the throws clause of the method.
| throws BadRequestException, IOException, ForbiddenException { | |
| throws BadRequestException, IOException, ForbiddenException, AccessException { |
| @@ -144,7 +147,8 @@ public void getOperationRun( | |||
| @PathParam("id") String runId) | |||
| throws BadRequestException, OperationRunNotFoundException, IOException, ForbiddenException { | |||
There was a problem hiding this comment.
The contextAccessEnforcer.enforce call throws AccessException, which is a checked exception. Since it is not caught, it must be declared in the throws clause of the method.
| throws BadRequestException, OperationRunNotFoundException, IOException, ForbiddenException { | |
| throws BadRequestException, OperationRunNotFoundException, IOException, ForbiddenException, AccessException { |
| @@ -169,7 +173,8 @@ public void failOperation(FullHttpRequest request, HttpResponder responder, | |||
| @PathParam("id") String runId) | |||
| throws OperationRunNotFoundException, IOException, BadRequestException, ForbiddenException { | |||
There was a problem hiding this comment.
The contextAccessEnforcer.enforce call throws AccessException, which is a checked exception. Since it is not caught, it must be declared in the throws clause of the method.
| throws OperationRunNotFoundException, IOException, BadRequestException, ForbiddenException { | |
| throws OperationRunNotFoundException, IOException, BadRequestException, ForbiddenException, AccessException { |
1 participant
The operation endpoints under
/v3/namespaces/{namespace-id}/operationslisted operation runs, fetched an operation run by id, and stopped an operation run for the namespace in the request path without enforcing any access control on that namespace. The handler carried an explicit// TODO(CDAP-20881) : Add RBAC check.This change injects
ContextAccessEnforcerinto the handler and enforces access on the requestedNamespaceId:scanOperationsandgetOperationRunenforceStandardPermission.GET.failOperation(stop) enforcesStandardPermission.UPDATE.This mirrors the namespace-level enforcement performed by the source control management service and the other authorization PRs in this area. Hardening only; no functional change for authorized callers.