-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-28286: Add filtering support for get_table_metas (Naveen Gangam) #5269
Conversation
@jfsii Could you review this change please? |
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
Show resolved
Hide resolved
return tableMetas; | ||
String catName = null; | ||
String dbName = null; | ||
if (tableMetas != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If "tableMetas" is empty, can it throw ArrayOutOfBound exception?
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
Outdated
Show resolved
Hide resolved
throws MetaException { | ||
return filterTableMetas(tableMetas); | ||
if (LOG.isDebugEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to check LOG.isDebugEnabled(), since LOG.debug will only output, when debug level logging is enabled.
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
Outdated
Show resolved
Hide resolved
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments on structure and code, I didn't see any incorrect functionality - I agree with the other comments around the log lines.
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
Show resolved
Hide resolved
...g/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have addressed the comments, it would be nice if someone can take an extra eye, cc @jfsii @mayankkunwar @zratkai. Thank you in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What changes were proposed in this pull request?
Currently the filterTableMetas() method in the HiveMetastoreAuthorizer is a no-op. It returns the full set of tables that are passed into it. This prevents any filtering that is to be done.
This PR adds logic to be able to filter out the ones that the user does not have access to.
Why are the changes needed?
To enhance security in HMS.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
Tested manually in a Ranger enabled environment.