Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3859966243
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -92,18 +144,21 @@ public static void validateFineGrainedAuth(Method
endpointMethod, UriInfo uriInf
if (auth.targetType() == TargetType.TABLE) {
// paramName is mandatory for table level authorization
if (StringUtils.isEmpty(auth.paramName())) {
+ // TODO: PinotTableRestletResource#copyTable declares
@Authorize(TABLE) with no paramName
+ // and already 500s here. Prefer startup-time validation of the
resource model (or 403)
+ // so a misannotation is not an error-rate page.
throw new WebApplicationException(
"paramName not found for table level authorization in API: " +
uriInfo.getRequestUri(),
Response.Status.INTERNAL_SERVER_ERROR);
}
- // find the paramName in the path or query params
- targetId = findRawTargetId(auth, uriInfo.getPathParameters(),
uriInfo.getQueryParameters());
+ // Path params are part of the endpoint declaration. Query params are
caller-supplied, so only a name
+ // the method binds as @QueryParam may identify the table. Otherwise a
caller could append
+ // ?tableName=<a table it is scoped to> and have a cluster-wide
request authorized as table-scoped.
+ targetId = findRawTargetId(auth, endpointMethod,
uriInfo.getPathParameters(), uriInfo.getQueryParameters());
if (StringUtils.isEmpty(targetId)) {
- throw new WebApplicationException(
- "Could not find paramName " + auth.paramName() + " in path or
query params of the API: "
- + uriInfo.getRequestUri(),
Response.Status.INTERNAL_SERVER_ERROR);
+ throw unboundTableParamException(auth.paramName(),
uriInfo.getRequestUri());
Review Comment:
Reverted 33fd79729a. Tree matches 159bac2bfe. 500 stays on both unbound and
missing table paramName, so the two misannotation paths agree again. Optional
tableName uploads keep 500 as well. 403 is follow-up if we still want it.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -33,38 +40,83 @@
import org.slf4j.LoggerFactory;
-/// Utility methods to share in Broker and Controller request filters related
to fine grain authorization.
+/// Shared broker and controller helpers for fine-grained authorization.
+///
+/// The broker request filter calls [#validateFineGrainedAuth], so tightening
target resolution
+/// here applies to both roles. Every in-tree broker `@Authorize(targetType =
TargetType.TABLE)`
+/// endpoint (`PinotBrokerDebug`, `PinotBrokerRouting`) binds `tableName` as a
`@PathParam` inside
+/// its own `@Path` template, so the declared-query-param filter does not
change broker resolution.
+///
+/// [#findRawTargetId] is public. It previously took 3 arguments (`Authorize`,
path map, query
+/// map) and now takes 4 (`Authorize`, `Method`, path map, query map). There
is no overload: the
+/// `Method` is required to apply the declared-parameter filter. A plugin
calling the old
+/// signature will fail to link.
public class FineGrainedAuthUtils {
private static final Logger LOGGER =
LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of
endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new
ConcurrentHashMap<>();
+
+ /// Status when `@Authorize(TABLE)` names a parameter the endpoint never
binds.
+ ///
+ /// Historically [Response.Status#INTERNAL_SERVER_ERROR], which pages on
error-rate alerts and
+ /// reads as a controller bug. [Response.Status#FORBIDDEN] is the
authorization outcome.
+ /// Restore `INTERNAL_SERVER_ERROR` here — or pass it to
[#unboundTableParamException] — to
+ /// revert to the previous status. Tests pin both directions.
+ static final Response.Status UNBOUND_TABLE_PARAM_STATUS =
Response.Status.FORBIDDEN;
Review Comment:
Dropped the constant, both unboundTableParamException helpers, and the
revert test with that commit. Nothing left to inline as 403. The throw is 500
again.
##########
pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java:
##########
@@ -36,22 +38,113 @@
public class FineGrainedAuthUtilsTest {
@Test
- public void testFindRawTargetId() throws Exception {
+ public void testFindRawTargetId()
+ throws Exception {
MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
- Authorize tableAuth =
TestResource.class.getDeclaredMethod("getTable").getAnnotation(Authorize.class);
+ Method tableMethod = TestResource.class.getDeclaredMethod("getTable");
+ Method tableQueryMethod =
TestResource.class.getDeclaredMethod("getTableByQuery", String.class);
+ Authorize tableAuth = tableMethod.getAnnotation(Authorize.class);
+ Authorize tableQueryAuth = tableQueryMethod.getAnnotation(Authorize.class);
Authorize clusterAuth =
getAnnotatedMethod().getAnnotation(Authorize.class);
pathParams.putSingle("tableName", "pathTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams,
queryParams), "pathTable");
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod,
pathParams, queryParams), "pathTable");
pathParams.clear();
queryParams.putSingle("tableName", "queryTable");
- assertEquals(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams,
queryParams), "queryTable");
- assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth, pathParams,
queryParams));
+ // The annotation names tableName, but getTable never binds it, so the
query value is not trusted.
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod,
pathParams, queryParams));
+ assertEquals(FineGrainedAuthUtils.findRawTargetId(tableQueryAuth,
tableQueryMethod, pathParams, queryParams),
+ "queryTable");
+ assertNull(FineGrainedAuthUtils.findRawTargetId(clusterAuth,
getAnnotatedMethod(), pathParams, queryParams));
queryParams.clear();
- assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, pathParams,
queryParams));
+ assertNull(FineGrainedAuthUtils.findRawTargetId(tableAuth, tableMethod,
pathParams, queryParams));
+ }
+
+ @Test
+ public void testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam()
+ throws Exception {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(),
Mockito.any(), Mockito.any()))
+ .thenReturn(true);
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
+ MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+ queryParams.putSingle("tableName", "callerPicked");
+ Mockito.when(mockUriInfo.getPathParameters()).thenReturn(pathParams);
+ Mockito.when(mockUriInfo.getQueryParameters()).thenReturn(queryParams);
+
Mockito.when(mockUriInfo.getRequestUri()).thenReturn(URI.create("http://localhost/tables";));
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Method unboundTableMethod =
TestResource.class.getDeclaredMethod("getTable");
+ try {
+ FineGrainedAuthUtils.validateFineGrainedAuth(unboundTableMethod,
mockUriInfo, mockHttpHeaders, ac);
+ Assert.fail("Expected WebApplicationException");
+ } catch (WebApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Could not find paramName"));
+ Assert.assertEquals(e.getResponse().getStatus(),
+ FineGrainedAuthUtils.UNBOUND_TABLE_PARAM_STATUS.getStatusCode());
+ Assert.assertEquals(e.getResponse().getStatus(),
Response.Status.FORBIDDEN.getStatusCode());
+ }
+ Mockito.verify(ac, Mockito.never())
+ .hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(),
Mockito.any(), Mockito.any());
+ }
+
+ @Test
+ public void testUnboundTableParamExceptionRevertsToInternalServerError() {
+ URI requestUri = URI.create("http://localhost/tables";);
+ WebApplicationException current =
FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri);
+ Assert.assertEquals(current.getResponse().getStatus(),
Response.Status.FORBIDDEN.getStatusCode());
+
+ // Revert path: passing the previous status restores the 500 that this
case used to pin.
+ WebApplicationException reverted =
FineGrainedAuthUtils.unboundTableParamException("tableName", requestUri,
+ Response.Status.INTERNAL_SERVER_ERROR);
+ Assert.assertTrue(reverted.getMessage().contains("Could not find
paramName"));
+ Assert.assertEquals(reverted.getResponse().getStatus(),
Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ Assert.assertEquals(reverted.getMessage(), current.getMessage());
+ }
Review Comment:
Deleted. The pin that matters is still
testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam (500, hasAccess
never called).
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java:
##########
@@ -25,6 +25,15 @@
import org.apache.pinot.spi.annotations.InterfaceStability;
+/// Controller access-control SPI.
+///
+/// Custom implementations should audit two resolution changes from
apache/pinot#18975:
+/// 1. A table name appended as an undeclared query parameter no longer reaches
+/// [#hasAccess(String, AccessType, HttpHeaders, String)]; the request
arrives with a `null`
+/// table name and must be treated as cluster-wide.
+/// 2. [org.apache.pinot.core.auth.FineGrainedAuthUtils#findRawTargetId]
changed from 3 arguments
+/// (`Authorize`, path map, query map) to 4 by adding the resource
`Method`. There is no
+/// overload; a plugin calling the old signature will fail to link.
Review Comment:
Class-level migration note is gone with the revert. Per-method javadoc still
has the durable contract (null table name = cluster-wide). findRawTargetId 3 to
4 and the broker-resolution notes are in the PR description now, not on the
AccessControl SPI.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -37,34 +43,58 @@
public class FineGrainedAuthUtils {
private static final Logger LOGGER =
LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+ /// Memoizes [#declaredQueryParams(Method)]; bounded by the number of
endpoints.
+ private static final Map<Method, Set<String>> DECLARED_QUERY_PARAMS = new
ConcurrentHashMap<>();
private FineGrainedAuthUtils() {
}
- /// Returns the parameter from the path or query params.
- /// @param paramName to look for
- /// @param pathParams path params
- /// @param queryParams query params
- /// @return the value of the parameter
- private static String findParam(String paramName, MultivaluedMap<String,
String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- String name = pathParams.getFirst(paramName);
- if (name == null) {
- name = queryParams.getFirst(paramName);
+ /// Returns the names `endpointMethod` binds as [QueryParam]s.
+ ///
+ /// Only method-level `@QueryParam` binding is recognized; an endpoint
binding parameters through `@BeanParam` or
+ /// resource-class field injection is treated as declaring none. That
direction denies table scope rather than
+ /// granting it, so it fails closed. No in-tree controller or broker
resource uses either form today.
+ ///
+ /// Results are memoized because [Method#getParameterAnnotations()]
re-parses the class-file annotation bytes on
+ /// every call, and this runs on every request before authentication. The
key set is bounded by the number of
+ /// endpoints.
+ public static Set<String> declaredQueryParams(Method endpointMethod) {
+ return DECLARED_QUERY_PARAMS.computeIfAbsent(endpointMethod,
FineGrainedAuthUtils::findDeclaredQueryParams);
+ }
+
+ private static Set<String> findDeclaredQueryParams(Method endpointMethod) {
+ Set<String> declared = new HashSet<>();
+ for (Annotation[] parameterAnnotations :
endpointMethod.getParameterAnnotations()) {
+ for (Annotation parameterAnnotation : parameterAnnotations) {
+ if (parameterAnnotation instanceof QueryParam queryParam) {
+ declared.add(queryParam.value());
+ }
+ }
}
- return name;
+ return Set.copyOf(declared);
}
/// Finds the raw target parameter identified by an [Authorize] annotation.
///
+ /// Path parameters are template variables of the endpoint's own `@Path` and
are always trusted. Query parameters
+ /// are caller-supplied, so only a name the method binds as `@QueryParam`
may identify the table.
+ ///
/// @param auth annotation identifying the authorization target
+ /// @param endpointMethod the resource method, used to decide which query
parameters are declared
/// @param pathParams request path parameters
/// @param queryParams request query parameters
/// @return the unnormalized table parameter value, or `null` for a cluster
target or missing table parameter
@Nullable
- public static String findRawTargetId(Authorize auth, MultivaluedMap<String,
String> pathParams,
- MultivaluedMap<String, String> queryParams) {
- return auth.targetType() == TargetType.TABLE ? findParam(auth.paramName(),
pathParams, queryParams) : null;
+ public static String findRawTargetId(Authorize auth, Method endpointMethod,
+ MultivaluedMap<String, String> pathParams, MultivaluedMap<String,
String> queryParams) {
+ if (auth.targetType() != TargetType.TABLE) {
+ return null;
+ }
+ String targetId = pathParams.getFirst(auth.paramName());
+ if (targetId == null &&
declaredQueryParams(endpointMethod).contains(auth.paramName())) {
+ targetId = queryParams.getFirst(auth.paramName());
+ }
+ return targetId;
}
Review Comment:
Scope and Custom AccessControl sections are updated. FineGrainedAuthUtils is
shared with the broker filter, so undeclared ?tableName= is ignored there too.
The 8 in-tree broker TABLE endpoints use @PathParam, so behavior is unchanged.
Broker BasicAuth cluster-scope gap stays follow-up. findRawTargetId went 3 to 4
args (Method added), no overload; old plugins fail to link. Also restored the
LLC completion and GET /auth/verify back-compat bullets that helm already had.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
