Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -27,8 +27,12 @@
import io.cdap.cdap.proto.PreferencesDetail;
import io.cdap.cdap.proto.ProgramType;
import io.cdap.cdap.proto.id.ApplicationId;
import io.cdap.cdap.proto.id.InstanceId;
import io.cdap.cdap.proto.id.NamespaceId;
import io.cdap.cdap.proto.id.ProgramId;
import io.cdap.cdap.proto.security.StandardPermission;
import io.cdap.cdap.security.spi.authentication.AuthenticationContext;
import io.cdap.cdap.security.spi.authorization.AccessEnforcer;
import io.cdap.http.HttpResponder;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpResponseStatus;
Expand All @@ -48,14 +52,20 @@ public class PreferencesHttpHandlerInternal extends AbstractAppFabricHttpHandler
private final PreferencesService preferencesService;
private final ApplicationLifecycleService applicationLifecycleService;
private final NamespaceQueryAdmin namespaceQueryAdmin;
private final AccessEnforcer accessEnforcer;
private final AuthenticationContext authenticationContext;

@Inject
PreferencesHttpHandlerInternal(PreferencesService preferencesService,
ApplicationLifecycleService applicationLifecycleService,
NamespaceQueryAdmin namespaceQueryAdmin) {
NamespaceQueryAdmin namespaceQueryAdmin,
AccessEnforcer accessEnforcer,
AuthenticationContext authenticationContext) {
this.preferencesService = preferencesService;
this.applicationLifecycleService = applicationLifecycleService;
this.namespaceQueryAdmin = namespaceQueryAdmin;
this.accessEnforcer = accessEnforcer;
this.authenticationContext = authenticationContext;
}

/**
Expand All @@ -67,6 +77,8 @@ public class PreferencesHttpHandlerInternal extends AbstractAppFabricHttpHandler
@Path("/preferences")
@GET
public void getInstancePreferences(HttpRequest request, HttpResponder responder) {
accessEnforcer.enforce(new InstanceId(""), authenticationContext.getPrincipal(),
StandardPermission.GET);
Comment on lines +80 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The accessEnforcer.enforce(...) call throws AccessException, which is a checked exception. Since this exception is not caught, the enclosing method getInstancePreferences (and similarly getNamespacePreferences and getApplicationPreferences) must declare throws Exception (or throws AccessException) to avoid compilation errors. This matches the pattern used in PreferencesHttpHandler.java.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b5f64c9, added throws Exception to the method signature.

PreferencesDetail detail = preferencesService.getPreferences();
responder.sendJson(HttpResponseStatus.OK, GSON.toJson(detail, PreferencesDetail.class));
}
Expand All @@ -90,6 +102,8 @@ public void getNamespacePreferences(HttpRequest request, HttpResponder responder
@PathParam("namespace-id") String namespace,
@QueryParam("resolved") boolean resolved) {
NamespaceId namespaceId = new NamespaceId(namespace);
accessEnforcer.enforce(namespaceId, authenticationContext.getPrincipal(),
StandardPermission.GET);
// No need to check if namespace exists. PreferencesService returns an empty PreferencesDetail when that happens.
PreferencesDetail detail;
if (resolved) {
Expand Down Expand Up @@ -121,6 +135,8 @@ public void getApplicationPreferences(HttpRequest request, HttpResponder respond
@PathParam("application-id") String appId,
@QueryParam("resolved") boolean resolved) {
ApplicationId applicationId = new ApplicationId(namespace, appId);
accessEnforcer.enforce(applicationId, authenticationContext.getPrincipal(),
StandardPermission.GET);
// No need to check if application exists. PreferencesService returns an empty PreferencesDetail when that happens.
PreferencesDetail detail;
if (resolved) {
Expand Down Expand Up @@ -156,6 +172,7 @@ public void getProgramPreferences(HttpRequest request, HttpResponder responder,
@PathParam("program-id") String programId,
@QueryParam("resolved") boolean resolved) throws Exception {
ProgramId program = new ProgramId(namespace, appId, getProgramType(programType), programId);
accessEnforcer.enforce(program, authenticationContext.getPrincipal(), StandardPermission.GET);
// No need to check if program exists. PreferencesService returns an empty PreferencesDetail when that happens.
PreferencesDetail detail;
if (resolved) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* 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.Mockito.mock;
import static org.mockito.Mockito.when;

import io.cdap.cdap.common.namespace.NamespaceQueryAdmin;
import io.cdap.cdap.config.PreferencesService;
import io.cdap.cdap.internal.app.services.ApplicationLifecycleService;
import io.cdap.cdap.proto.PreferencesDetail;
import io.cdap.cdap.proto.id.InstanceId;
import io.cdap.cdap.proto.id.NamespaceId;
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.InMemoryAccessController;
import io.cdap.cdap.security.spi.authentication.AuthenticationContext;
import io.cdap.cdap.security.spi.authorization.UnauthorizedException;
import io.cdap.http.HttpResponder;
import io.netty.handler.codec.http.HttpRequest;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;

/**
* FileFetcherHttpHandlerInternal's sibling gap: PreferencesHttpHandlerInternal exposed
* instance/namespace/application preferences with no authorization check at all,
* unlike PreferencesHttpHandler (the public equivalent), which calls
* accessEnforcer.enforce(..., StandardPermission.GET) at every one of those same scopes.
* Mirrors ConfigHandlerAuthorizationTest. InMemoryAccessController implements
* AccessController, which itself extends AccessEnforcer, so it can be passed directly
* as the handler's AccessEnforcer dependency (this handler, unlike ConfigHandler, takes
* the plain AccessEnforcer interface rather than ContextAccessEnforcer).
*/
public class PreferencesHttpHandlerInternalAuthorizationTest {
private static final Principal MASTER_PRINCIPAL = new Principal("master", Principal.PrincipalType.USER);
private static final Principal UNPRIVILEGED_PRINCIPAL = new Principal("unprivileged",
Principal.PrincipalType.USER);
private static final NamespaceId OTHER_NAMESPACE = new NamespaceId("other-tenant-namespace");

private static PreferencesHttpHandlerInternal preferencesHandler;

HttpRequest request;
HttpResponder responder;
Exception exceptionThrown;

@BeforeClass
public static void setup() {
StandardPermission[] requiredPermissions = new StandardPermission[] {StandardPermission.GET};

InMemoryAccessController inMemoryAccessController = new InMemoryAccessController();
inMemoryAccessController.grant(Authorizable.fromEntityId(InstanceId.SELF), MASTER_PRINCIPAL,
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(requiredPermissions))));
inMemoryAccessController.grant(Authorizable.fromEntityId(OTHER_NAMESPACE), MASTER_PRINCIPAL,
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(requiredPermissions))));
AuthenticationContext authenticationContext = new AuthenticationTestContext();

PreferencesService mockPreferencesService = mock(PreferencesService.class);
when(mockPreferencesService.getPreferences())
.thenReturn(new PreferencesDetail(Collections.emptyMap(), 0, false));
when(mockPreferencesService.getPreferences(any(NamespaceId.class)))
.thenReturn(new PreferencesDetail(Collections.emptyMap(), 0, false));
ApplicationLifecycleService mockAppLifecycleService = mock(ApplicationLifecycleService.class);
NamespaceQueryAdmin mockNamespaceQueryAdmin = mock(NamespaceQueryAdmin.class);

preferencesHandler = new PreferencesHttpHandlerInternal(mockPreferencesService, mockAppLifecycleService,
mockNamespaceQueryAdmin, inMemoryAccessController, authenticationContext);
}

@Before
public void initializeVariables() {
request = mock(HttpRequest.class);
responder = mock(HttpResponder.class);
exceptionThrown = null;
}

@Test
public void testGetInstancePreferencesUnauthorized() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since getInstancePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.

Suggested change
@Test
public void testGetInstancePreferencesUnauthorized() {
@Test
public void testGetInstancePreferencesUnauthorized() throws Exception {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b5f64c9, added throws Exception to the method signature.

AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL);
try {
preferencesHandler.getInstancePreferences(request, responder);
} catch (UnauthorizedException e) {
exceptionThrown = e;
}
Assert.assertNotNull("an unprivileged caller must not be able to read instance-level "
+ "preferences via the internal endpoint", exceptionThrown);
}

@Test
public void testGetInstancePreferencesAuthorized() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since getInstancePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.

Suggested change
@Test
public void testGetInstancePreferencesAuthorized() {
@Test
public void testGetInstancePreferencesAuthorized() throws Exception {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b5f64c9, added throws Exception to the method signature.

AuthenticationTestContext.actAsPrincipal(MASTER_PRINCIPAL);
try {
preferencesHandler.getInstancePreferences(request, responder);
} catch (UnauthorizedException e) {
exceptionThrown = e;
}
Assert.assertNull("a caller with instance-level access must not be rejected", exceptionThrown);
}

@Test
public void testGetNamespacePreferencesUnauthorized() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since getNamespacePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.

Suggested change
@Test
public void testGetNamespacePreferencesUnauthorized() {
@Test
public void testGetNamespacePreferencesUnauthorized() throws Exception {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b5f64c9, added throws Exception to the method signature.

AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL);
try {
preferencesHandler.getNamespacePreferences(request, responder, OTHER_NAMESPACE.getNamespace(), false);
} catch (UnauthorizedException e) {
exceptionThrown = e;
}
Assert.assertNotNull("an unprivileged caller must not be able to read another "
+ "namespace's preferences via the internal endpoint", exceptionThrown);
}

@Test
public void testGetNamespacePreferencesAuthorized() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since getNamespacePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.

Suggested change
@Test
public void testGetNamespacePreferencesAuthorized() {
@Test
public void testGetNamespacePreferencesAuthorized() throws Exception {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b5f64c9, added throws Exception to the method signature.

AuthenticationTestContext.actAsPrincipal(MASTER_PRINCIPAL);
try {
preferencesHandler.getNamespacePreferences(request, responder, OTHER_NAMESPACE.getNamespace(), false);
} catch (UnauthorizedException e) {
exceptionThrown = e;
}
Assert.assertNull("a caller with access to the namespace must not be rejected", exceptionThrown);
}

@Test
public void testGetApplicationPreferencesUnauthorized() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since getApplicationPreferences throws a checked exception, this test method must declare throws Exception to compile successfully.

Suggested change
@Test
public void testGetApplicationPreferencesUnauthorized() {
@Test
public void testGetApplicationPreferencesUnauthorized() throws Exception {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b5f64c9, added throws Exception to the method signature.

AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL);
try {
preferencesHandler.getApplicationPreferences(request, responder, OTHER_NAMESPACE.getNamespace(),
"some-app", false);
} catch (UnauthorizedException e) {
exceptionThrown = e;
}
Assert.assertNotNull("an unprivileged caller must not be able to read another "
+ "namespace's application preferences via the internal endpoint", exceptionThrown);
}
}
Comment on lines +169 to +207

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

To ensure comprehensive test coverage of the new authorization checks, consider adding tests for authorized application preferences and both authorized/unauthorized program preferences.

  }

  @Test
  public void testGetApplicationPreferencesAuthorized() throws Exception {
    AuthenticationTestContext.actAsPrincipal(MASTER_PRINCIPAL);
    try {
      preferencesHandler.getApplicationPreferences(request, responder, OTHER_NAMESPACE.getNamespace(),
          "some-app", false);
    } catch (UnauthorizedException e) {
      exceptionThrown = e;
    }
    Assert.assertNull("a caller with access to the application's namespace must not be rejected", exceptionThrown);
  }

  @Test
  public void testGetProgramPreferencesUnauthorized() throws Exception {
    AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL);
    try {
      preferencesHandler.getProgramPreferences(request, responder, OTHER_NAMESPACE.getNamespace(),
          "some-app", "workflows", "some-program", false);
    } catch (UnauthorizedException e) {
      exceptionThrown = e;
    }
    Assert.assertNotNull("an unprivileged caller must not be able to read another "
        + "namespace's program preferences via the internal endpoint", exceptionThrown);
  }

  @Test
  public void testGetProgramPreferencesAuthorized() throws Exception {
    AuthenticationTestContext.actAsPrincipal(MASTER_PRINCIPAL);
    try {
      preferencesHandler.getProgramPreferences(request, responder, OTHER_NAMESPACE.getNamespace(),
          "some-app", "workflows", "some-program", false);
    } catch (UnauthorizedException e) {
      exceptionThrown = e;
    }
    Assert.assertNull("a caller with access to the program's namespace must not be rejected", exceptionThrown);
  }
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added in b5f64c9: testGetApplicationPreferencesAuthorized, testGetProgramPreferencesUnauthorized, testGetProgramPreferencesAuthorized. The authorized cases needed grants scoped to the specific ApplicationId/ProgramId (not just the namespace), since InMemoryAccessController does exact-entity matching rather than hierarchical, so I added those grants and the corresponding PreferencesService mock stubs too.