From f950cf3dd68db27727e93bdc2f71640def4bcdbc Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Thu, 6 Aug 2026 21:15:05 +0700 Subject: [PATCH 1/2] Require authorization on PreferencesHttpHandlerInternal endpoints PreferencesHttpHandlerInternal exposes instance, namespace, application, and program preferences with no authorization check anywhere in its call chain: PreferencesService (its only dependency for reads) has no ContextAccessEnforcer/AccessEnforcer of its own. Its public sibling, PreferencesHttpHandler, calls accessEnforcer.enforce(entity, principal, StandardPermission.GET) before every equivalent read at every one of those same scopes, so this is a direct authorization gap between the two, not an intentional design difference. Since this handler is reachable the same way as the rest of the appfabric.http.handler set (see the FileFetcherHttpHandlerInternal fix for the full reachability argument: same HTTP service, same discoverable service, same Router path-matching as public v3 handlers, no v3Internal-specific authorization anywhere in RouterPathLookup or AuthenticationHandler), any principal with a valid CDAP access token can read another namespace's, application's, or program's preferences, which frequently carry pipeline runtime arguments and connection configuration. This adds the same enforce() calls PreferencesHttpHandler already makes at each scope, plus a dedicated authorization test mirroring ConfigHandlerAuthorizationTest. --- .../PreferencesHttpHandlerInternal.java | 19 ++- ...sHttpHandlerInternalAuthorizationTest.java | 156 ++++++++++++++++++ 2 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java index ea967b1d2bfa..0c192dfb718b 100644 --- a/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java +++ b/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java @@ -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; @@ -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; } /** @@ -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); PreferencesDetail detail = preferencesService.getPreferences(); responder.sendJson(HttpResponseStatus.OK, GSON.toJson(detail, PreferencesDetail.class)); } @@ -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) { @@ -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) { @@ -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) { diff --git a/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java b/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java new file mode 100644 index 000000000000..c3c2c480d68b --- /dev/null +++ b/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java @@ -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() { + 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() { + 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() { + 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() { + 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() { + 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); + } +} From b5f64c9eaae20c7e28df8055135baea107b5341d Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Fri, 7 Aug 2026 08:38:34 +0700 Subject: [PATCH 2/2] Declare throws Exception where enforce() is now called, add missing test coverage Per gemini-code-assist review on #16192: accessEnforcer.enforce() throws a checked AccessException, so getInstancePreferences/getNamespacePreferences/ getApplicationPreferences needed throws Exception on the method signature (matching the existing pattern in PreferencesHttpHandler and the untouched getProgramPreferences), and the corresponding test methods needed it too. Also added the application- and program-level authorized-access test cases Gemini suggested, with grants scoped to the specific ApplicationId/ProgramId since InMemoryAccessController does exact-entity matching, not hierarchical. --- .../PreferencesHttpHandlerInternal.java | 6 +- ...sHttpHandlerInternalAuthorizationTest.java | 61 +++++++++++++++++-- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java index 0c192dfb718b..e905edb1a85f 100644 --- a/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java +++ b/cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternal.java @@ -76,7 +76,7 @@ public class PreferencesHttpHandlerInternal extends AbstractAppFabricHttpHandler */ @Path("/preferences") @GET - public void getInstancePreferences(HttpRequest request, HttpResponder responder) { + public void getInstancePreferences(HttpRequest request, HttpResponder responder) throws Exception { accessEnforcer.enforce(new InstanceId(""), authenticationContext.getPrincipal(), StandardPermission.GET); PreferencesDetail detail = preferencesService.getPreferences(); @@ -100,7 +100,7 @@ public void getInstancePreferences(HttpRequest request, HttpResponder responder) @GET public void getNamespacePreferences(HttpRequest request, HttpResponder responder, @PathParam("namespace-id") String namespace, - @QueryParam("resolved") boolean resolved) { + @QueryParam("resolved") boolean resolved) throws Exception { NamespaceId namespaceId = new NamespaceId(namespace); accessEnforcer.enforce(namespaceId, authenticationContext.getPrincipal(), StandardPermission.GET); @@ -133,7 +133,7 @@ public void getNamespacePreferences(HttpRequest request, HttpResponder responder public void getApplicationPreferences(HttpRequest request, HttpResponder responder, @PathParam("namespace-id") String namespace, @PathParam("application-id") String appId, - @QueryParam("resolved") boolean resolved) { + @QueryParam("resolved") boolean resolved) throws Exception { ApplicationId applicationId = new ApplicationId(namespace, appId); accessEnforcer.enforce(applicationId, authenticationContext.getPrincipal(), StandardPermission.GET); diff --git a/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java b/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java index c3c2c480d68b..2abdd01c6946 100644 --- a/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java +++ b/cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/PreferencesHttpHandlerInternalAuthorizationTest.java @@ -24,8 +24,11 @@ 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.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.Authorizable; import io.cdap.cdap.proto.security.Principal; import io.cdap.cdap.proto.security.StandardPermission; @@ -58,6 +61,9 @@ public class PreferencesHttpHandlerInternalAuthorizationTest { 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 final ApplicationId OTHER_APP = new ApplicationId("other-tenant-namespace", "some-app"); + private static final ProgramId OTHER_PROGRAM = new ProgramId("other-tenant-namespace", "some-app", + ProgramType.WORKFLOW, "some-program"); private static PreferencesHttpHandlerInternal preferencesHandler; @@ -74,6 +80,10 @@ public static void setup() { Collections.unmodifiableSet(new HashSet<>(Arrays.asList(requiredPermissions)))); inMemoryAccessController.grant(Authorizable.fromEntityId(OTHER_NAMESPACE), MASTER_PRINCIPAL, Collections.unmodifiableSet(new HashSet<>(Arrays.asList(requiredPermissions)))); + inMemoryAccessController.grant(Authorizable.fromEntityId(OTHER_APP), MASTER_PRINCIPAL, + Collections.unmodifiableSet(new HashSet<>(Arrays.asList(requiredPermissions)))); + inMemoryAccessController.grant(Authorizable.fromEntityId(OTHER_PROGRAM), MASTER_PRINCIPAL, + Collections.unmodifiableSet(new HashSet<>(Arrays.asList(requiredPermissions)))); AuthenticationContext authenticationContext = new AuthenticationTestContext(); PreferencesService mockPreferencesService = mock(PreferencesService.class); @@ -81,6 +91,10 @@ public static void setup() { .thenReturn(new PreferencesDetail(Collections.emptyMap(), 0, false)); when(mockPreferencesService.getPreferences(any(NamespaceId.class))) .thenReturn(new PreferencesDetail(Collections.emptyMap(), 0, false)); + when(mockPreferencesService.getPreferences(any(ApplicationId.class))) + .thenReturn(new PreferencesDetail(Collections.emptyMap(), 0, false)); + when(mockPreferencesService.getPreferences(any(ProgramId.class))) + .thenReturn(new PreferencesDetail(Collections.emptyMap(), 0, false)); ApplicationLifecycleService mockAppLifecycleService = mock(ApplicationLifecycleService.class); NamespaceQueryAdmin mockNamespaceQueryAdmin = mock(NamespaceQueryAdmin.class); @@ -96,7 +110,7 @@ public void initializeVariables() { } @Test - public void testGetInstancePreferencesUnauthorized() { + public void testGetInstancePreferencesUnauthorized() throws Exception { AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL); try { preferencesHandler.getInstancePreferences(request, responder); @@ -108,7 +122,7 @@ public void testGetInstancePreferencesUnauthorized() { } @Test - public void testGetInstancePreferencesAuthorized() { + public void testGetInstancePreferencesAuthorized() throws Exception { AuthenticationTestContext.actAsPrincipal(MASTER_PRINCIPAL); try { preferencesHandler.getInstancePreferences(request, responder); @@ -119,7 +133,7 @@ public void testGetInstancePreferencesAuthorized() { } @Test - public void testGetNamespacePreferencesUnauthorized() { + public void testGetNamespacePreferencesUnauthorized() throws Exception { AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL); try { preferencesHandler.getNamespacePreferences(request, responder, OTHER_NAMESPACE.getNamespace(), false); @@ -131,7 +145,7 @@ public void testGetNamespacePreferencesUnauthorized() { } @Test - public void testGetNamespacePreferencesAuthorized() { + public void testGetNamespacePreferencesAuthorized() throws Exception { AuthenticationTestContext.actAsPrincipal(MASTER_PRINCIPAL); try { preferencesHandler.getNamespacePreferences(request, responder, OTHER_NAMESPACE.getNamespace(), false); @@ -142,7 +156,7 @@ public void testGetNamespacePreferencesAuthorized() { } @Test - public void testGetApplicationPreferencesUnauthorized() { + public void testGetApplicationPreferencesUnauthorized() throws Exception { AuthenticationTestContext.actAsPrincipal(UNPRIVILEGED_PRINCIPAL); try { preferencesHandler.getApplicationPreferences(request, responder, OTHER_NAMESPACE.getNamespace(), @@ -153,4 +167,41 @@ public void testGetApplicationPreferencesUnauthorized() { Assert.assertNotNull("an unprivileged caller must not be able to read another " + "namespace's application preferences via the internal endpoint", exceptionThrown); } + + @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 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 must not be rejected", exceptionThrown); + } }