Copilot commented on code in PR #4779:
URL: https://github.com/apache/polaris/pull/4779#discussion_r3415298213
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogHandler.java:
##########
@@ -131,7 +131,7 @@ public LoadGenericTableResponse createGenericTable(
public boolean dropGenericTable(TableIdentifier identifier) {
PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.DROP_TABLE_WITHOUT_PURGE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ authorizeBasicTableLikeOperationOrThrow(op,
PolarisEntitySubType.GENERIC_TABLE, identifier);
return this.genericTableCatalog.dropGenericTable(identifier);
}
Review Comment:
This likely changes the behavior for drops of non-existent tables:
authorizing via a table-entity-based path may require resolving the table
entity first, which can force a NOT_FOUND/forbidden-style failure before
`genericTableCatalog.dropGenericTable(identifier)` can return `false` (the
method’s return type suggests idempotent 'not found' handling). If the API is
intended to remain idempotent, consider authorizing against the table entity
when it exists, but falling back to a namespace-based authorization (or an
existence-agnostic check) when the table cannot be resolved, so callers still
get a `false` instead of an exception.
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java:
##########
@@ -126,4 +129,47 @@ Stream<DynamicNode> testDropGenericTablePrivileges() {
.shouldPassWith(PolarisPrivilege.CATALOG_MANAGE_METADATA)
.createTests();
}
+
+ private PolarisAuthzTestsFactory.Builder tableLevelAuthzTestsBuilder(
+ String operationName, TableIdentifier tableId) {
+ return authzTestsBuilder(operationName)
+ .grantAction(
+ priv ->
+ adminService.grantPrivilegeOnTableToRole(
+ CATALOG_NAME, CATALOG_ROLE1, tableId, priv))
+ .revokeAction(
+ priv -> {
+ PrivilegeResult res =
+ adminService.revokePrivilegeOnTableFromRole(
+ CATALOG_NAME, CATALOG_ROLE1, tableId, priv);
+ // After table drop + recreate, grants on the old entity no
longer exist on the
+ // new entity. This is expected and equivalent to a successful
revoke.
+ return res.isSuccess() ? res : new PrivilegeResult(new
PolarisGrantRecord());
+ });
Review Comment:
This converts *any* revoke failure into an apparent success, which can mask
real problems (e.g., revoke failing due to auth, validation, or service errors)
and make the test pass for the wrong reason. Instead, only treat specific
expected failure modes as success (e.g., 'grant not found' / 'entity not found'
after drop+recreate), and propagate all other failures. Concretely: inspect
`PrivilegeResult` for an error/status code and only normalize that specific
case, or add a dedicated helper in the authz test factory/service that performs
'revoke-if-present' semantics.
--
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]
