xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244764417
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/Authorize.java:
##########
@@ -37,6 +37,11 @@
// If the target type is the Pinot cluster, then this field is not required
String paramName() default "";
+ /// Optional path or query parameter that narrows a cluster operation to one
table for coarse access control.
+ /// Use only when the entire operation is limited to that table, rather than
filtering part of the response.
+ /// Omitting the parameter retains cluster-wide scope. Fine-grained
authorization still uses targetType.
+ String tableScopeParam() default "";
Review Comment:
Documented the fail-closed behavior on tableScopeParam: missing input, or a
name absent from the endpoint path parameters and declared query parameters,
resolves no table and retains cluster-wide authorization. A table-scoped
principal is denied.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus
affected-module formatting, Checkstyle, and license checks.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AuthenticationFilterTest.java:
##########
@@ -179,15 +293,93 @@ public void testReadOnlyEndpointsDeclareReadAccess() {
@Test
public void testSegmentCompletionGetEndpointsDeclareCreateAccess() {
+ MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
+ MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+ queryParams.putSingle("tableName", "someTable");
for (String methodName : new String[]{"extendBuildTime",
"segmentConsumed", "segmentStoppedConsuming",
"segmentCommitStart", "reduceSegmentSize"}) {
Method method =
Arrays.stream(LLCSegmentCompletionHandlers.class.getDeclaredMethods())
.filter(candidate ->
candidate.getName().equals(methodName)).findFirst().orElseThrow();
assertEquals(_authFilter.extractAccessType(method), AccessType.CREATE);
assertEquals(method.getAnnotation(Authorize.class).action(),
Actions.Cluster.COMMIT_SEGMENT);
+ // These callbacks name no table, so they take the cluster-wide check.
That is why a table-scoped server token
+ // is denied and realtime commit stalls — the consequence documented on
BaseBasicAuthAccessControl.
+ assertNull(AuthenticationFilter.extractTableName(method, pathParams,
queryParams),
+ methodName + " must not resolve a table name");
}
}
+ /// Guards the assumption stated on
[org.apache.pinot.core.auth.FineGrainedAuthUtils#getDeclaredQueryParams]: a
+ /// table target's parameter must be bound as a `@PathParam` or a
method-level `@QueryParam`. An endpoint binding it
+ /// via `@BeanParam` or field injection would resolve no table and hard-fail
with 400 for every caller, admin
+ /// included.
+ @Test
+ public void testTableAuthorizeParamsAreBoundOnTheMethod()
+ throws Exception {
+ List<Class<?>> resourceClasses = controllerResourceClasses();
+ assertTrue(resourceClasses.size() > 20, "expected to discover the
controller resource classes, found "
+ + resourceClasses.size());
+ List<String> violations = new ArrayList<>();
+ for (Class<?> resourceClass : resourceClasses) {
+ for (Method method : resourceClass.getDeclaredMethods()) {
+ Authorize authorize = method.getAnnotation(Authorize.class);
+ if (authorize == null || authorize.targetType() != TargetType.TABLE) {
Review Comment:
Extended the existing resource binding guard to validate non-empty
tableScopeParam declarations on CLUSTER endpoints as well as paramName on TABLE
endpoints. The renamed testAuthorizeTableParamsAreBoundOnTheMethod scans the
controller resources and reports unbound table parameters.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus
affected-module formatting, Checkstyle, and license checks.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -140,35 +145,55 @@ AccessType extractAccessType(Method endpointMethod) {
return AccessType.READ;
}
+ /// Resolves the table `endpointMethod` acts on, or `null` when the request
addresses the cluster rather than a
+ /// table. `AccessControlUtils.validatePermission` picks the table-scoped or
the cluster-wide check on that answer,
+ /// so it must not be steerable by the caller.
+ ///
+ /// Path parameters are template variables of the endpoint's own `@Path`,
hence part of its declaration. Query
+ /// parameters are caller-supplied and JAX-RS surfaces every one present on
the URI, so only those the endpoint
+ /// declares may name the table: otherwise a caller could append
`?tableName=<a table it is scoped to>` to a cluster
+ /// endpoint and have it authorized as a table-scoped request.
+ ///
+ @Nullable
@VisibleForTesting
static String extractTableName(Method endpointMethod, MultivaluedMap<String,
String> pathParameters,
MultivaluedMap<String, String> queryParameters) {
Authorize authorize = endpointMethod.getAnnotation(Authorize.class);
if (authorize != null && authorize.targetType() == TargetType.TABLE) {
- return FineGrainedAuthUtils.findRawTargetId(authorize, pathParameters,
queryParameters);
+ // Same primitive as FineGrainedAuthUtils.validateFineGrainedAuth: path
first, then a declared query param.
+ return FineGrainedAuthUtils.findRawTargetId(authorize, endpointMethod,
pathParameters, queryParameters);
+ }
+ if (authorize != null && authorize.targetType() == TargetType.CLUSTER) {
+ String parameter = authorize.tableScopeParam();
Review Comment:
Confirmed the CLUSTER branch remains authoritative and returns before the
generic table-name heuristic. The extended resource binding guard now verifies
that every explicit cluster tableScopeParam is bound by the method, covering
the missing enforcement noted here.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus
affected-module formatting, Checkstyle, and license checks.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -62,6 +74,21 @@ public boolean hasTable(String tableName) {
return isTableIncluded(tableName) && isTableNotExcluded(tableName);
}
+ /// Returns whether this principal is scoped to every table, i.e. it carries
neither an allow-list nor an
+ /// exclude-list. Access control implementations use this to decide requests
that name no table, so the return value
+ /// must satisfy: `true` implies [#hasTable(String)] holds for every table
name. A subclass that narrows table scope
+ /// by any other means — in particular by overriding [#hasTable(String)] —
must override this method to match, or it
+ /// will report unrestricted scope while denying individual tables.
+ ///
+ /// Table lists are the signal rather than [ZkBasicAuthPrincipal]'s
`RoleType`. The static factory has no role, both
+ /// controller factories share this check, and an ADMIN that also carries an
allow-list is still scoped. Granting
+ /// cluster access by role would let that principal reach cluster state
outside the list. The bootstrapped admin
+ /// is bootstrapped with a null `UserConfig#getTables()`, which normalizes
to an empty set here, so
+ /// `RoleType.ADMIN` without a table list still passes.
+ public boolean hasUnrestrictedTableAccess() {
Review Comment:
Keeping both methods overridable to preserve compatibility with existing
BasicAuthPrincipal subclasses. Making the previously overridable hasTable
method final would break those subclasses; making only
hasUnrestrictedTableAccess final would not enforce the invariant and would
prevent custom restrictions from being reflected in the cluster check. The
Javadoc already requires subclasses narrowing hasTable to override
hasUnrestrictedTableAccess, and the PR migration notes now make that
requirement explicit. The in-tree ZkBasicAuthPrincipal does not override
hasTable and follows the same table-list policy.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus
affected-module formatting, Checkstyle, and license checks.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -44,12 +47,21 @@ public BasicAuthPrincipal(String name, String token,
Set<String> tables, Set<Str
Set<String> permissions, Map<String, List<String>> rlsFilters) {
_name = name;
_token = token;
- _tables = tables;
- _excludeTables = excludeTables;
+ // Copied because hasUnrestrictedTableAccess() gates cluster-level
authorization off these sets: a caller that
+ // retained and later cleared its own set would silently widen this
principal to unrestricted scope. Copied
+ // permissively rather than with Set.copyOf, which rejects null elements —
a user record stored in ZooKeeper can
+ // carry a null table name, and the broker and server rebuild every
principal on each request, so one malformed
+ // record would otherwise fail authentication for every user.
+ _tables = copyOfNullable(tables);
+ _excludeTables = copyOfNullable(excludeTables);
_permissions = permissions.stream().map(s ->
s.toLowerCase()).collect(Collectors.toSet());
_rlsFilters = rlsFilters;
}
+ private static Set<String> copyOfNullable(@Nullable Set<String> values) {
Review Comment:
Added testBasicAuthPrincipalWithNullTableEntry for allow-lists and
exclude-lists containing both a valid table and null. It verifies construction
succeeds, valid table authorization behaves correctly, and the malformed entry
never makes the principal unrestricted.
Addressed in `6b0ece6f8c`. Validation: 31 focused tests passed, plus
affected-module formatting, Checkstyle, and license checks.
--
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]
