Enforce access on program instances and live-info endpoints in ProgramRuntimeHttpHandler - #16172
Enforce access on program instances and live-info endpoints in ProgramRuntimeHttpHandler#16172adilburaksen wants to merge 2 commits into
Conversation
…mRuntimeHttpHandler The batch instances endpoint and the live-info endpoint read program instance counts and live runtime info using the namespace from the request path without access enforcement, while the single-program instances endpoint enforces via ensureProgramExists. Add accessEnforcer.enforce(programId, principal, StandardPermission.GET) before those reads (parity with the enforced paths), plus a regression test verifying an unauthorized principal is denied before runtime access.
![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 adds authorization checks using accessEnforcer.enforce to the /instances and /live-info endpoints in ProgramRuntimeHttpHandler, along with a new test class to verify these checks. The feedback points out that the authorization check within the batch /instances endpoint should catch SecurityException individually to prevent a single unauthorized item from failing the entire batch. Additionally, the reviewer suggests adding test coverage for the /instances endpoint and replacing deprecated Mockito Matchers with ArgumentMatchers in the test class.
| accessEnforcer.enforce(programId, authenticationContext.getPrincipal(), StandardPermission.GET); | ||
| output.add(getProgramInstances(runnable, spec, programId)); |
There was a problem hiding this comment.
The /instances endpoint is a batch API designed to process multiple runnables and return individual status codes (e.g., 200, 400, 404) for each item in the batch. By calling accessEnforcer.enforce(...) directly inside the loop without catching SecurityException (or UnauthorizedException), any single unauthorized program will throw an exception that propagates out of the method, failing the entire batch request with a 401/403.
To preserve the batch semantics, wrap the enforcement check in a try-catch block, catch SecurityException, and add a BatchRunnableInstances with HttpResponseStatus.UNAUTHORIZED.code() to the output list so that authorized programs can still be successfully processed and returned.
try {
accessEnforcer.enforce(programId, authenticationContext.getPrincipal(), StandardPermission.GET);
output.add(getProgramInstances(runnable, spec, programId));
} catch (SecurityException e) {
output.add(new BatchRunnableInstances(runnable, HttpResponseStatus.UNAUTHORIZED.code(), e.getMessage()));
}| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
| import org.mockito.Matchers; |
There was a problem hiding this comment.
org.mockito.Matchers is deprecated in Mockito 2.x and removed in newer versions. It should be replaced with org.mockito.ArgumentMatchers to avoid using deprecated APIs and ensure compatibility with future Mockito upgrades.
| import org.mockito.Matchers; | |
| import org.mockito.ArgumentMatchers; |
| import org.junit.Test; | ||
| import org.mockito.Matchers; | ||
|
|
||
| public class ProgramRuntimeHttpHandlerAuthorizationTest { |
There was a problem hiding this comment.
The test class only covers the /live-info endpoint (liveInfo). Since this pull request also adds access enforcement to the batch /instances endpoint (getInstances), please add corresponding unit tests to verify both authorized and unauthorized access scenarios for the batch endpoint.
| runtimeService = mock(ProgramRuntimeService.class); | ||
| NamespaceQueryAdmin namespaceQueryAdmin = mock(NamespaceQueryAdmin.class); | ||
|
|
||
| when(store.getLatestApp(Matchers.any(ApplicationReference.class))).thenReturn(APP_ID); |
There was a problem hiding this comment.
Replace the deprecated Matchers.any with ArgumentMatchers.any.
| when(store.getLatestApp(Matchers.any(ApplicationReference.class))).thenReturn(APP_ID); | |
| when(store.getLatestApp(ArgumentMatchers.any(ApplicationReference.class))).thenReturn(APP_ID); |
| exceptionThrown = e; | ||
| } | ||
| Assert.assertNotNull(exceptionThrown); | ||
| verify(runtimeService, never()).getLiveInfo(Matchers.any(ProgramId.class)); |
There was a problem hiding this comment.
Replace the deprecated Matchers.any with ArgumentMatchers.any.
| verify(runtimeService, never()).getLiveInfo(Matchers.any(ProgramId.class)); | |
| verify(runtimeService, never()).getLiveInfo(ArgumentMatchers.any(ProgramId.class)); |
Return a 403 entry for an unauthorized program in the batch /instances request instead of failing the whole batch, matching the endpoint's existing per-entry error handling, while still not reading instance data for the unauthorized program. Extends the test with a batch case.
|
@sahusanket mind giving this a review? It adds the missing authorization enforcement on the program-instance and live-info endpoints in ProgramRuntimeHttpHandler, matching how the other program handlers already gate access. Been open a few weeks now — glad to rebase if it's drifted from develop. |
1 participant
The batch
/instancesendpoint (getInstances) and the/live-infoendpoint inProgramRuntimeHttpHandlerread program instance counts and live runtime information using the namespace from the request path, without access enforcement. The single-program/instancesendpoint already enforcesStandardPermission.GETvialifecycleService.ensureProgramExists, but the batch variant andlive-infoskip it.This change adds
accessEnforcer.enforce(programId, authenticationContext.getPrincipal(), StandardPermission.GET)before those reads, matching the enforcement already applied on the equivalent single-program path and elsewhere in this handler, plus a regression test verifying an unauthorized principal is denied before any runtime-service access.