Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@
import io.cdap.cdap.proto.id.ProgramId;
import io.cdap.cdap.proto.id.ProgramReference;
import io.cdap.cdap.proto.id.ProgramRunId;
import io.cdap.cdap.proto.security.StandardPermission;
import io.cdap.cdap.security.spi.authorization.ContextAccessEnforcer;
import io.cdap.cdap.security.spi.authorization.UnauthorizedException;
import io.cdap.http.HttpResponder;
import io.netty.handler.codec.http.FullHttpRequest;
Expand Down Expand Up @@ -107,18 +109,21 @@ public class ProgramLifecycleHttpHandler extends AbstractAppFabricHttpHandler {
private final MRJobInfoFetcher mrJobInfoFetcher;
private final NamespaceQueryAdmin namespaceQueryAdmin;
private final Store store;
private final ContextAccessEnforcer contextAccessEnforcer;

@Inject
ProgramLifecycleHttpHandler(Store store,
DiscoveryServiceClient discoveryServiceClient,
ProgramLifecycleService lifecycleService,
MRJobInfoFetcher mrJobInfoFetcher,
NamespaceQueryAdmin namespaceQueryAdmin) {
NamespaceQueryAdmin namespaceQueryAdmin,
ContextAccessEnforcer contextAccessEnforcer) {
this.store = store;
this.discoveryServiceClient = discoveryServiceClient;
this.lifecycleService = lifecycleService;
this.mrJobInfoFetcher = mrJobInfoFetcher;
this.namespaceQueryAdmin = namespaceQueryAdmin;
this.contextAccessEnforcer = contextAccessEnforcer;
}

/**
Expand All @@ -133,6 +138,7 @@ public void getMapReduceInfo(HttpRequest request, HttpResponder responder,
@PathParam("run-id") String runId) throws IOException, NotFoundException {
ApplicationReference appRef = new ApplicationReference(namespaceId, appId);
ProgramReference programRef = appRef.program(ProgramType.MAPREDUCE, mapreduceId);
contextAccessEnforcer.enforce(programRef, StandardPermission.GET);

// runId is uuid, can be retrieved ignoring version
RunRecordDetail runRecordMeta = store.getRun(programRef, runId);
Expand Down Expand Up @@ -461,6 +467,7 @@ public void programRunRecord(HttpRequest request, HttpResponder responder,
ProgramType programType = ProgramType.valueOfCategoryName(type, BadRequestException::new);
ProgramReference programRef = new ApplicationReference(namespaceId, appName).program(programType,
programName);
contextAccessEnforcer.enforce(programRef, StandardPermission.GET);
RunRecordDetail runRecordMeta = store.getRun(programRef, runId);
if (runRecordMeta == null) {
throw new NotFoundException(
Expand Down Expand Up @@ -494,6 +501,7 @@ public void programRunRecordVersioned(HttpRequest request,
ProgramType programType = ProgramType.valueOfCategoryName(type, BadRequestException::new);
ProgramId progId = new ApplicationId(namespaceId, appName, appVersion).program(programType,
programName);
contextAccessEnforcer.enforce(progId, StandardPermission.GET);
RunRecordDetail runRecordMeta = store.getRun(progId.run(runid));
if (runRecordMeta != null && !isTetheredRunRecord(runRecordMeta)) {
RunRecord runRecord = RunRecord.builder(runRecordMeta).build();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/*
* Copyright © 2026 Cask Data, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/

package io.cdap.cdap.gateway.handlers;

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

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;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import io.cdap.cdap.app.mapreduce.MRJobInfoFetcher;
import io.cdap.cdap.app.store.Store;
import io.cdap.cdap.common.namespace.NamespaceQueryAdmin;
import io.cdap.cdap.internal.app.services.ProgramLifecycleService;
import io.cdap.cdap.internal.app.store.RunRecordDetail;
import io.cdap.cdap.proto.ProgramRunCluster;
import io.cdap.cdap.proto.ProgramRunClusterStatus;
import io.cdap.cdap.proto.ProgramRunStatus;
import io.cdap.cdap.proto.ProgramType;
import io.cdap.cdap.proto.id.ApplicationReference;
import io.cdap.cdap.proto.id.NamespaceId;
import io.cdap.cdap.proto.id.ProfileId;
import io.cdap.cdap.proto.id.ProgramReference;
import io.cdap.cdap.proto.security.Authorizable;
import io.cdap.cdap.proto.security.Principal;
import io.cdap.cdap.proto.security.StandardPermission;
import io.cdap.cdap.security.auth.context.AuthenticationTestContext;
import io.cdap.cdap.security.authorization.DefaultContextAccessEnforcer;
import io.cdap.cdap.security.authorization.InMemoryAccessController;
import io.cdap.cdap.security.spi.authorization.ContextAccessEnforcer;
import io.cdap.cdap.security.spi.authorization.UnauthorizedException;
import io.cdap.http.HttpResponder;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpResponseStatus;
import java.util.Collections;
import org.apache.twill.discovery.DiscoveryServiceClient;
import org.junit.Assert;
import org.junit.Test;

/**
* Authorization tests for {@link ProgramLifecycleHttpHandler}.
*/
public class ProgramLifecycleHttpHandlerAuthorizationTest {

private static final Principal ALICE = new Principal("alice", Principal.PrincipalType.USER);
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.

String namespaceId = NamespaceId.DEFAULT.getNamespace();
String appName = "app";
String programName = "worker";
String runId = "run";
ProgramReference programRef = new ApplicationReference(namespaceId, appName)
.program(ProgramType.WORKER, programName);
Authorizable programAuthorizable = Authorizable.fromEntityId(programRef);
RunRecordDetail runRecord = createRunRecord(namespaceId, appName, programName, runId);

Store store = mock(Store.class);
when(store.getRun(eq(programRef), eq(runId))).thenReturn(runRecord);
InMemoryAccessController accessController = new InMemoryAccessController();
accessController.revoke(programAuthorizable);
ProgramLifecycleHttpHandler handler = createHandler(store,
new DefaultContextAccessEnforcer(new AuthenticationTestContext(), accessController));
HttpRequest request = mock(HttpRequest.class);
HttpResponder responder = mock(HttpResponder.class);

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

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)
    );

verify(store, never()).getRun(any(ProgramReference.class), anyString());

try {
accessController.grant(programAuthorizable, ALICE, Collections.singleton(StandardPermission.GET));
AuthenticationTestContext.actAsPrincipal(ALICE);
handler.programRunRecord(request, responder, namespaceId, appName, "workers", programName, runId);
} finally {
accessController.revoke(programAuthorizable);
}

verify(store).getRun(eq(programRef), eq(runId));
verify(responder).sendJson(eq(HttpResponseStatus.OK), anyString());
}

private static ProgramLifecycleHttpHandler createHandler(Store store,
ContextAccessEnforcer contextAccessEnforcer) {
return new ProgramLifecycleHttpHandler(
store,
mock(DiscoveryServiceClient.class),
mock(ProgramLifecycleService.class),
mock(MRJobInfoFetcher.class),
mock(NamespaceQueryAdmin.class),
contextAccessEnforcer);
}

private static RunRecordDetail createRunRecord(String namespaceId, String appName,
String programName, String runId) {
return RunRecordDetail.builder()
.setProgramRunId(new NamespaceId(namespaceId).app(appName).worker(programName).run(runId))
.setStartTime(1L)
.setRunTime(1L)
.setStatus(ProgramRunStatus.RUNNING)
.setCluster(new ProgramRunCluster(ProgramRunClusterStatus.PROVISIONED, null, null))
.setProfileId(ProfileId.NATIVE)
.setSourceId(new byte[] { 0 })
.build();
}
}