Skip to content

Enforce access on program run-record endpoints in ProgramLifecycleHttpHandler - #16171

Open
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:fix-programlifecycle-runrecord-bola
Open

Enforce access on program run-record endpoints in ProgramLifecycleHttpHandler#16171
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:fix-programlifecycle-runrecord-bola

Conversation

@adilburaksen

Copy link
Copy Markdown

The programRunRecord, programRunRecordVersioned, and getMapReduceInfo endpoints in ProgramLifecycleHttpHandler read run records directly from the store using the namespace from the request path, without the access enforcement that the equivalent ProgramLifecycleService methods already apply (e.g. getProgramTotalRunCount enforces StandardPermission.GET on the program reference).

This change adds contextAccessEnforcer.enforce(programRef/programId, StandardPermission.GET) before those direct store.getRun(...) reads, bringing them to parity with the already-enforced paths, and adds a regression test (ProgramLifecycleHttpHandlerAuthorizationTest) verifying that an unauthorized principal is denied before any store access and an authorized principal succeeds.

The injected ContextAccessEnforcer follows the same pattern used by the other app-fabric handlers.

…pHandler

The programRunRecord, programRunRecordVersioned, and getMapReduceInfo
endpoints read run records directly from the store using the namespace
from the request path, without the access enforcement that the equivalent
ProgramLifecycleService methods already apply. Add
contextAccessEnforcer.enforce(programRef/programId, StandardPermission.GET)
before those reads, plus a regression test verifying an unauthorized
principal is denied before any store access.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces authorization checks to the getMapReduceInfo, programRunRecord, and programRunRecordVersioned endpoints in ProgramLifecycleHttpHandler using ContextAccessEnforcer. It also adds a new test class, ProgramLifecycleHttpHandlerAuthorizationTest, to verify these permissions. Feedback on the changes includes updating deprecated Mockito imports to ArgumentMatchers, expanding test coverage to include the other two modified endpoints, and refactoring the exception assertions in the tests to use Assert.assertThrows for better readability and diagnostics.

Comment on lines +19 to +21
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The class org.mockito.Matchers is deprecated in Mockito 2.x and completely removed in Mockito 3.x. To ensure compatibility with newer Mockito versions and avoid deprecation warnings, please use org.mockito.ArgumentMatchers instead.

Suggested change
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;

private static final Principal BOB = new Principal("bob", Principal.PrincipalType.USER);

@Test
public void testProgramRunRecordRequiresProgramGetPermission() throws Exception {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The pull request adds authorization enforcement to three endpoints: programRunRecord, programRunRecordVersioned, and getMapReduceInfo. However, the test class only verifies authorization for programRunRecord. It is highly recommended to add corresponding unit tests for programRunRecordVersioned and getMapReduceInfo to ensure that the authorization checks for these endpoints are also covered and protected against future regressions.

Comment on lines +84 to +90
try {
AuthenticationTestContext.actAsPrincipal(BOB);
handler.programRunRecord(request, responder, namespaceId, appName, "workers", programName, runId);
Assert.fail();
} catch (UnauthorizedException e) {
// expected
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a try-catch block with Assert.fail() to assert that an exception is thrown is an older JUnit pattern. If an unexpected exception is thrown, it can make the test failure harder to diagnose. It is cleaner and more idiomatic to use Assert.assertThrows.

    AuthenticationTestContext.actAsPrincipal(BOB);
    Assert.assertThrows(UnauthorizedException.class, () ->
      handler.programRunRecord(request, responder, namespaceId, appName, "workers", programName, runId)
    );

@adilburaksen

Copy link
Copy Markdown
Author

@sahusanket this is the companion to the ProgramRuntime one — it enforces access on the run-record endpoints in ProgramLifecycleHttpHandler, which currently return run history without an authorization check. Small and self-contained. Let me know if you want the two split differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant