Copilot commented on code in PR #13806:
URL: https://github.com/apache/gravitino/pull/13806#discussion_r4254447205
##########
server/src/main/java/org/apache/gravitino/server/web/filter/authorization/LoadTableAuthorizationExecutor.java:
##########
@@ -65,6 +74,23 @@ public LoadTableAuthorizationExecutor(
}
}
+ @Override
+ public boolean execute(AuthorizationRequestContext
authorizationRequestContext) throws Exception {
+ if (super.execute(authorizationRequestContext)) {
+ return true;
+ }
+
+ if (allowCheckExistenceEvaluator == null
+ || !allowCheckExistenceEvaluator.evaluate(
+ metadataContext, pathParams, authorizationRequestContext,
entityType)) {
+ return false;
+ }
+
+ NameIdentifier tableIdentifier =
metadataContext.get(Entity.EntityType.TABLE);
+ return tableIdentifier != null
+ &&
!GravitinoEnv.getInstance().tableDispatcher().tableExists(tableIdentifier);
Review Comment:
This authorizes the request from a negative `tableExists` result, but the
interceptor then calls `methodInvocation.proceed()` and loads the table again.
If another request creates the table between those operations, a caller with
only create/probe privileges can receive the newly created table metadata
without satisfying the load expression. Return a 404 directly from the
existence result (for example via a response-capable executor result or a
dedicated exception handled by the interceptor) rather than treating absence as
authorization success.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java:
##########
@@ -49,6 +49,12 @@ public class AuthorizationExpressionConstants {
ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_SELECT_TABLE || ANY_MODIFY_TABLE)
""";
+ public static final String PROBE_TABLE_LIKE_AUTHORIZATION_EXPRESSION =
Review Comment:
The new public authorization-expression constant lacks JavaDoc, which is
required for new public members and can fail documentation checks. Document
that this expression permits only an existence probe after normal table-load
authorization is denied.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestTableAuthorizationExpression.java:
##########
@@ -170,6 +170,30 @@ public void testLoadTable() throws NoSuchMethodException,
OgnlException {
"METALAKE::USE_CATALOG")));
}
+ @Test
+ public void testLoadTableExistenceProbe() throws NoSuchMethodException,
OgnlException {
+ Method method =
+ TableOperations.class.getMethod(
+ "loadTable", String.class, String.class, String.class,
String.class, String.class);
+ AuthorizationExpression authorizationExpressionAnnotation =
+ method.getAnnotation(AuthorizationExpression.class);
+ String expression =
authorizationExpressionAnnotation.allowCheckExistence();
+ MockAuthorizationExpressionEvaluator mockEvaluator =
+ new MockAuthorizationExpressionEvaluator(expression);
+ assertFalse(mockEvaluator.getResult(ImmutableSet.of()));
+ assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::OWNER")));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("SCHEMA::CREATE_TABLE")));
+ assertTrue(
+ mockEvaluator.getResult(
+ ImmutableSet.of("SCHEMA::CREATE_TABLE", "CATALOG::USE_CATALOG",
"SCHEMA::USE_SCHEMA")));
+ assertTrue(
+ mockEvaluator.getResult(
+ ImmutableSet.of("SCHEMA::SELECT_TABLE", "CATALOG::USE_CATALOG",
"SCHEMA::USE_SCHEMA")));
+ assertTrue(
+ mockEvaluator.getResult(
+ ImmutableSet.of("SCHEMA::MODIFY_TABLE", "CATALOG::USE_CATALOG",
"SCHEMA::USE_SCHEMA")));
Review Comment:
The expression also grants the probe path for `ANY_CREATE_VIEW`, but this
test covers only create-table, select-table, and modify-table grants. Add the
create-view assertion so a typo or removal of that newly introduced
authorization branch is detected.
--
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]
