xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3848969485
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -97,8 +127,10 @@ public static void validateFineGrainedAuth(Method
endpointMethod, UriInfo uriInf
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());
Review Comment:
Nit, non-blocking: the fail-closed outcome for an undeclared table query
parameter arrives as **500 INTERNAL_SERVER_ERROR** via the `Could not find
paramName` branch below, and the new
`testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam` pins that.
Failing closed is the right call, but 500 is the wrong status for an
authorization outcome — it will page on error-rate alerts and read as a
controller bug rather than a denied request. It is pre-existing behavior for a
missing target and only reachable through a misannotated endpoint, so I would
not hold the PR for it. Worth a `// TODO` or a follow-up issue to make the
"annotation names a parameter the endpoint never binds" case a 403 (or a
startup-time validation over the resource model, which would catch `copyTable`
too).
##########
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:
This is exactly the consolidation I was after — coarse and fine-grained now
resolve through one primitive, so they cannot disagree. Thanks for moving
`declaredQueryParams` down here rather than duplicating it.
Two follow-ups on the blast radius, both description-only:
**1. The PR's "Scope: Controller only" line is now stale.**
`FineGrainedAuthUtils` is shared with `pinot-broker`'s `AuthenticationFilter`,
so the broker's fine-grained target resolution is tightened by this commit too.
I checked every broker `@Authorize(targetType = TargetType.TABLE)` endpoint —
all 8 across `PinotBrokerDebug` and `PinotBrokerRouting` bind `tableName` as a
`@PathParam` inside their own `@Path` template, so none of them regress. But
the description should say the broker's resolution changed and was verified,
rather than "left for a follow-up" — a reader auditing this later will
otherwise assume `pinot-broker` was untouched.
**2. `findRawTargetId` is a public signature change.** It went from 3 args
to 4 (`Authorize, MultivaluedMap, MultivaluedMap` -> `Authorize, Method,
MultivaluedMap, MultivaluedMap`). Every in-tree caller is updated, and the
class carries no `@InterfaceAudience` marker, so I do not think it needs an
overload — but it belongs in the **Custom `AccessControl` implementations**
paragraph alongside the `null`-table-name note, since a plugin calling it will
fail to link.
Verified across controller + broker: 92 of the 93 `@Authorize(TABLE)`
endpoints resolve their `paramName` from either a `@Path` template variable or
a declared `@QueryParam`, so nothing newly falls into the "could not find
paramName" branch. The one exception is `PinotTableRestletResource#copyTable`,
which declares `@Authorize(targetType = TargetType.TABLE)` with **no**
`paramName` and therefore already 500s on master via the
`StringUtils.isEmpty(auth.paramName())` guard — pre-existing and untouched
here, worth a separate issue.
--
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]
