Require authorization on PreferencesHttpHandlerInternal endpoints - #16192
Require authorization on PreferencesHttpHandlerInternal endpoints#16192herdiyana256 wants to merge 2 commits into
Conversation
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.
![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 secures the internal preferences HTTP handler by introducing authorization checks via AccessEnforcer across instance, namespace, application, and program preference endpoints, and adds a corresponding authorization test suite. The review feedback correctly identifies compilation errors due to unhandled checked exceptions (AccessException) thrown by the new enforcement calls, requiring throws declarations in both the handler and test methods. Additionally, the feedback suggests expanding test coverage to include authorized application preferences and program-level preference checks.
| accessEnforcer.enforce(new InstanceId(""), authenticationContext.getPrincipal(), | ||
| StandardPermission.GET); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fixed in b5f64c9, added throws Exception to the method signature.
| @Test | ||
| public void testGetInstancePreferencesUnauthorized() { |
There was a problem hiding this comment.
Since getInstancePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.
| @Test | |
| public void testGetInstancePreferencesUnauthorized() { | |
| @Test | |
| public void testGetInstancePreferencesUnauthorized() throws Exception { |
There was a problem hiding this comment.
Fixed in b5f64c9, added throws Exception to the method signature.
| @Test | ||
| public void testGetInstancePreferencesAuthorized() { |
There was a problem hiding this comment.
Since getInstancePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.
| @Test | |
| public void testGetInstancePreferencesAuthorized() { | |
| @Test | |
| public void testGetInstancePreferencesAuthorized() throws Exception { |
There was a problem hiding this comment.
Fixed in b5f64c9, added throws Exception to the method signature.
| @Test | ||
| public void testGetNamespacePreferencesUnauthorized() { |
There was a problem hiding this comment.
Since getNamespacePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.
| @Test | |
| public void testGetNamespacePreferencesUnauthorized() { | |
| @Test | |
| public void testGetNamespacePreferencesUnauthorized() throws Exception { |
There was a problem hiding this comment.
Fixed in b5f64c9, added throws Exception to the method signature.
| @Test | ||
| public void testGetNamespacePreferencesAuthorized() { |
There was a problem hiding this comment.
Since getNamespacePreferences throws a checked exception, this test method must declare throws Exception to compile successfully.
| @Test | |
| public void testGetNamespacePreferencesAuthorized() { | |
| @Test | |
| public void testGetNamespacePreferencesAuthorized() throws Exception { |
There was a problem hiding this comment.
Fixed in b5f64c9, added throws Exception to the method signature.
| @Test | ||
| public void testGetApplicationPreferencesUnauthorized() { |
There was a problem hiding this comment.
Since getApplicationPreferences throws a checked exception, this test method must declare throws Exception to compile successfully.
| @Test | |
| public void testGetApplicationPreferencesUnauthorized() { | |
| @Test | |
| public void testGetApplicationPreferencesUnauthorized() throws Exception { |
There was a problem hiding this comment.
Fixed in b5f64c9, added throws Exception to the method signature.
| } | ||
| } |
There was a problem hiding this comment.
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);
}
}There was a problem hiding this comment.
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.
…est coverage Per gemini-code-assist review on cdapio#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.
1 participant
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 #16191, 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.