HIVE-28356: HMS’s Authorizer for the CREATE_TABLE event doesn’t handle HivePrivilegeObjectType.STORAGEHANDLER_URI #5343
Conversation
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/CreateTableEvent.java
Outdated
Show resolved
Hide resolved
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/CreateTableEvent.java
Outdated
Show resolved
Hide resolved
...e/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizableEvent.java
Outdated
Show resolved
Hide resolved
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/CreateTableEvent.java
Outdated
Show resolved
Hide resolved
| .setTableName(tblName) | ||
| .addCol("name", ColumnType.STRING_TYPE_NAME) | ||
| .setOwner(authorizedUser) | ||
| .build(conf); |
There was a problem hiding this comment.
This is a normal table, not a storage handler-based table. We need to verify a storage handler-based table.
| @@ -64,6 +61,12 @@ protected HivePrivilegeObject getHivePrivilegeObject(Table table) { | |||
| table.getOwner(), table.getOwnerType()); | |||
| } | |||
|
|
|||
| protected HivePrivilegeObject getHivePrivilegeObjectStorageHandlerUri(String storagehandler_uri, Table table) { | |||
There was a problem hiding this comment.
This method is not used anymore now.
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/CreateTableEvent.java
Show resolved
Hide resolved
|
The only test failure seems to be the newly added test. It looks like you haven't run the test locally, so make sure it runs fine before pushing it here. |
|
@ayushtkn @deniskuzZ Could you please review this PR. |
| @@ -88,7 +110,9 @@ private List<HivePrivilegeObject> getOutputHObjs() { | |||
|
|
|||
| COMMAND_STR = buildCommandString(COMMAND_STR,table); | |||
|
|
|||
| LOG.debug("<== CreateTableEvent.getOutputHObjs(): ret={}", ret); | |||
| if (LOG.isDebugEnabled()) { | |||
There was a problem hiding this comment.
minor, but why do we need this check?
| HiveStorageHandler hiveStorageHandler = (HiveStorageHandler) ReflectionUtils.newInstance( | ||
| conf.getClassByName(table.getParameters().get(hive_metastoreConstants.META_TABLE_STORAGE)), event.getHandler().getConf()); | ||
| String storageUri = hiveStorageHandler.getURIForAuth(table).toString(); | ||
| if (storageUri.contains("iceberg")) { |
There was a problem hiding this comment.
Why do you need to check for iceberg?
This code looks similar to AlterTableEvent#getOutputHObjs
…e HivePrivilegeObjectType.STORAGEHANDLER_URI
DanielZhu58
force-pushed
the
HIVE-28356
branch
from
a04a99e
to
ac6f324
Compare
|
| import org.apache.hadoop.hive.metastore.api.Database; | ||
| import org.apache.hadoop.hive.metastore.api.StorageDescriptor; | ||
| import org.apache.hadoop.hive.metastore.api.Table; | ||
| import org.apache.hadoop.hive.metastore.api.*; |
There was a problem hiding this comment.
nit: incorrect imports
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.apache.hadoop.util.ReflectionUtils; |
There was a problem hiding this comment.
nit: All the newly added imports are in an incorrect order. They should follow alphabetical order.
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
Is the change a dependency upgrade?
How was this patch tested?