xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3857774724
##########
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:
**The two misannotation paths now disagree, and I do not think that is
intended.**
Both branches below signal the same class of defect — an `@Authorize(TABLE)`
endpoint whose table parameter cannot be resolved — but they now answer
differently:
- empty `paramName` (line 147) → **500**, with a `// TODO` saying it should
be 403 or startup validation;
- `paramName` declared but unresolved (line 160) → **403**.
`PinotTableRestletResource#copyTable` hits the first, so the one in-tree
misannotation still produces the error-rate page the change was meant to avoid.
Whichever status is right, these two should match; otherwise the next person
debugging a misannotated endpoint gets a different signal depending on which
way it was misannotated.
Separately, on the change itself: my earlier comment was explicit that I
would **not** hold the PR for this and suggested a `// TODO` or a follow-up
issue. Turning it into a behavior change inside a security PR widens the blast
radius for no security benefit.
Worth being precise about what it affects, because it is not only
misannotations. `StringUtils.isEmpty(targetId)` also fires when a caller
**omits a properly declared but optional** `tableName` — `/v2/segments` (both
overloads), `/segments/batchUpload`, `/segments/reingested`. Those requests
already fail on master with 500, so no working client breaks and the risk is
genuinely low; but the observable status for a real client path changes, and
that is not in the **Backward-incompatible** list.
My recommendation is to drop this commit and file it separately, where the
segment-upload status change can be reviewed on its own merits. If you prefer
to keep it, it needs the two branches reconciled and a line in the back-compat
list.
##########
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:
This constant, the two-argument/three-argument `unboundTableParamException`
pair below, and `testUnboundTableParamExceptionRevertsToInternalServerError`
exist only to make a future revert convenient. I would drop all three.
Nothing in production calls the three-argument overload —
`validateFineGrainedAuth` calls the two-argument form, which always resolves to
`FORBIDDEN`. So the `Response.Status` parameter is unreachable outside the test
that exercises it, and the javadoc instruction "Restore `INTERNAL_SERVER_ERROR`
here ... Tests pin both directions" is describing a branch the codebase never
takes.
Reverting a one-line status change is `git revert`; it does not need a seam
carried in the shipped code. Suggest: inline `Response.Status.FORBIDDEN` at the
throw site, delete the constant and both helpers, and delete the revert test.
The behavior is unchanged and roughly 25 lines of main + test code go away.
##########
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:
`AccessControl` is `@InterfaceAudience.Public @InterfaceStability.Stable`,
so this is permanent, published API documentation — and what it now says is a
point-in-time migration note ("audit two resolution changes from
apache/pinot#18975", "changed from 3 arguments ... to 4").
That content is right and worth publishing, but it belongs in the release
notes and the PR description rather than in the type's javadoc, where it will
read as stale by 1.6 and cannot be removed without another API-doc change.
What does belong here is the durable contract, which the per-method javadoc
already states well: a `null` table name means the request named no table and
must be treated as cluster-wide. I would keep that and move the two numbered
migration items out.
The `findRawTargetId` signature note is also on the wrong type — that method
lives on `FineGrainedAuthUtils`, and the class javadoc there (added in the
previous commit) already covers it.
##########
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:
This test asserts that a helper overload with no production caller returns
500 when you hand it 500. It cannot fail for any reason connected to the
endpoint behavior it is named after, and it will keep passing if
`validateFineGrainedAuth` stops calling the helper entirely.
If the revert seam goes away as suggested on the constant above, this test
goes with it. The coverage that matters — an undeclared `?tableName=` is
rejected without reaching `hasAccess` — is already pinned by
`testValidateFineGrainedAuthIgnoresUndeclaredTableQueryParam`, including the
`Mockito.verify(ac, never())`, which is the right assertion.
--
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]
