Copilot commented on code in PR #6884:
URL: https://github.com/apache/hive/pull/6884#discussion_r4332432525
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
+ if (rqst.getDbname() == null || rqst.getTablename() == null) {
+ throw new MetaException("The DB and table name cannot be null.");
+ }
+ String[] parsedDbName = parseDbName(rqst.getDbname(), conf);
+ String catName = parsedDbName[CAT_NAME];
+ String dbName = parsedDbName[DB_NAME];
+ String tblName = rqst.getTablename();
+ rqst.setDbname(dbName);
+
+ FilterUtils.checkDbAndTableFilters(isServerFilterEnabled, filterHook,
catName, dbName, tblName);
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName, tblName);
+ getTableRequest.setCatName(catName);
+ Table table;
+ try {
+ table = get_table_core(getTableRequest);
+ } catch (NoSuchObjectException e) {
+ if (!allowMissingTable || rqst.isSetPartitionname()) {
+ throw e;
+ }
+ authorizeCleanupOfMissingTable(rqst, catName, dbName);
+ return;
+ }
+ firePreEvent(new PreAlterTableEvent(table, table, this));
Review Comment:
Using `PreAlterTableEvent` / `PreAlterDatabaseEvent` to authorize
compaction/cleanup is semantically incorrect and can trigger unintended
listener behavior (e.g., audit/replication logic, or privilege checks intended
for ALTER operations). Consider introducing/using a dedicated pre-event type
for compaction/cleanup authorization (or calling the authorization framework
directly) so listeners and authorization policies can distinguish
compaction/cleanup from ALTER_TABLE/ALTER_DATABASE.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
+ if (rqst.getDbname() == null || rqst.getTablename() == null) {
+ throw new MetaException("The DB and table name cannot be null.");
+ }
+ String[] parsedDbName = parseDbName(rqst.getDbname(), conf);
+ String catName = parsedDbName[CAT_NAME];
+ String dbName = parsedDbName[DB_NAME];
+ String tblName = rqst.getTablename();
+ rqst.setDbname(dbName);
+
+ FilterUtils.checkDbAndTableFilters(isServerFilterEnabled, filterHook,
catName, dbName, tblName);
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName, tblName);
+ getTableRequest.setCatName(catName);
+ Table table;
+ try {
+ table = get_table_core(getTableRequest);
+ } catch (NoSuchObjectException e) {
+ if (!allowMissingTable || rqst.isSetPartitionname()) {
+ throw e;
+ }
+ authorizeCleanupOfMissingTable(rqst, catName, dbName);
+ return;
+ }
+ firePreEvent(new PreAlterTableEvent(table, table, this));
+
+ String location = table.getSd() == null ? null :
table.getSd().getLocation();
+ if (rqst.isSetPartitionname() && rqst.getPartitionname() != null) {
+ List<String> partVals = getPartValsFromName(table,
rqst.getPartitionname());
+ Partition partition = getMS().getPartition(catName, dbName, tblName,
partVals);
+ location = partition.getSd() == null ? null :
partition.getSd().getLocation();
+ }
+ if (location == null) {
+ throw new MetaException("Cannot determine storage location of " + dbName
+ "." + tblName
+ + "; compaction is not supported for this table or partition");
+ }
+ rqst.setRunas(TxnUtils.findUserToRunAs(location, table, conf));
+ }
+
+ /**
+ * Cleanup of a directory that has no backing table: authorize against the
database, require that the
+ * requested location is strictly inside the database's locations (so that a
client cannot get an arbitrary
+ * path purged by the Cleaner) and derive the run-as user from the directory
owner.
+ */
+ private void authorizeCleanupOfMissingTable(CompactionRequest rqst, String
catName, String dbName)
+ throws TException, IOException, InterruptedException {
+ Database db = get_database_core(catName, dbName);
+ firePreEvent(new PreAlterDatabaseEvent(db, db, this));
Review Comment:
Using `PreAlterTableEvent` / `PreAlterDatabaseEvent` to authorize
compaction/cleanup is semantically incorrect and can trigger unintended
listener behavior (e.g., audit/replication logic, or privilege checks intended
for ALTER operations). Consider introducing/using a dedicated pre-event type
for compaction/cleanup authorization (or calling the authorization framework
directly) so listeners and authorization policies can distinguish
compaction/cleanup from ALTER_TABLE/ALTER_DATABASE.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
Review Comment:
Catching broad `Exception` here will also catch `InterruptedException`
thrown by `authorizeAndPrepareCompactionRequest(...)`. If `rethrowException(e)`
does not restore the interrupt flag, this can break thread interruption
semantics (important for shutdown/cancellation). Prefer catching
`InterruptedException` explicitly, restoring
`Thread.currentThread().interrupt()`, and then rethrowing (or translating) in a
way consistent with the rest of the handler.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -491,6 +503,11 @@ public void compact(CompactionRequest rqst) throws
TException {
}
@Override
public CompactionResponse compact2(CompactionRequest rqst) throws TException
{
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, false);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().compact(rqst);
}
Review Comment:
Catching broad `Exception` here will also catch `InterruptedException`
thrown by `authorizeAndPrepareCompactionRequest(...)`. If `rethrowException(e)`
does not restore the interrupt flag, this can break thread interruption
semantics (important for shutdown/cancellation). Prefer catching
`InterruptedException` explicitly, restoring
`Thread.currentThread().interrupt()`, and then rethrowing (or translating) in a
way consistent with the rest of the handler.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestFilterHooks.java:
##########
@@ -293,18 +293,26 @@ protected void creatEnv(Configuration conf) throws
Exception {
TestTxnDbUtil.cleanDb(conf);
TestTxnDbUtil.prepDb(conf);
- CompactionRequest tbl1CompactionRequest = new CompactionRequest();
- tbl1CompactionRequest.setDbname(DBNAME1);
- tbl1CompactionRequest.setTablename(TAB1);
- tbl1CompactionRequest.setType(CompactionType.MAJOR);
- client.compact2(tbl1CompactionRequest);
-
- CompactionRequest tbl2CompactionRequest = new CompactionRequest();
- tbl2CompactionRequest.setDbname(DBNAME1);
- tbl2CompactionRequest.setTablename(TAB2);
- tbl2CompactionRequest.setType(CompactionType.MINOR);
- tbl2CompactionRequest.setPartitionname("name=value1");
- client.compact2(tbl2CompactionRequest);
+ // Enqueue compactions while the filter hook is not blocking table names
(server-side
+ // authorization on compact2 rejects blocked tables).
+ boolean origBlockResults = DummyMetaStoreFilterHookImpl.blockResults;
+ DummyMetaStoreFilterHookImpl.blockResults = false;
+ try {
+ CompactionRequest tbl1CompactionRequest = new CompactionRequest();
+ tbl1CompactionRequest.setDbname(DBNAME1);
+ tbl1CompactionRequest.setTablename(TAB1);
+ tbl1CompactionRequest.setType(CompactionType.MAJOR);
+ client.compact2(tbl1CompactionRequest);
+
+ CompactionRequest tbl2CompactionRequest = new CompactionRequest();
+ tbl2CompactionRequest.setDbname(DBNAME1);
+ tbl2CompactionRequest.setTablename(TAB2);
+ tbl2CompactionRequest.setType(CompactionType.MINOR);
+ tbl2CompactionRequest.setPartitionname("name=value1");
+ client.compact2(tbl2CompactionRequest);
+ } finally {
+ DummyMetaStoreFilterHookImpl.blockResults = origBlockResults;
+ }
Review Comment:
This test mutates a static global
(`DummyMetaStoreFilterHookImpl.blockResults`). Even with `finally`, it can
still cause cross-test interference if tests run concurrently (or if other
tests rely on this flag mid-execution). If the test suite can execute in
parallel, consider isolating this via per-test hook instances/configuration
rather than a shared static, or enforce serialization for this test class.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
Review Comment:
The `allowMissingTable` boolean introduces a dual-mode code path that’s easy
to misuse and harder to understand at call sites. Consider splitting into two
explicit methods (e.g., `authorizeAndPrepareCompactionRequest(...)` vs
`authorizeAndPrepareCleanupRequest(...)`), or replace the boolean with a small
enum/strategy that makes the intended behavior self-documenting.
--
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]
