-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Introduce Hive AccessControlMetadata #972
Introduce Hive AccessControlMetadata #972
Conversation
Fixes #971 |
@Override | ||
public void createRole(ConnectorSession session, String role, Optional<HivePrincipal> grantor) | ||
{ | ||
// roles are case insensitive in Hive |
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.
This comment is confusing, because the code here seems to be case sensitive.
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.
Correct. It looks like a bug that existed before. Let me fix that in separate PR.
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.
@Override | ||
public List<GrantInfo> listTablePrivileges(ConnectorSession session, List<SchemaTableName> tableNames) | ||
{ | ||
Set<HivePrincipal> principals = listEnabledPrincipals(metastore, session.getIdentity()) |
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.
Both usages collect to a set. I'd change the method to return a set.
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.
This method listEnabledPrincipals
is also used to verify if user has an access to a table. Populating whole set all the time may affect query semantic analysis time.
|
||
import io.prestosql.plugin.hive.metastore.SemiTransactionalHiveMetastore; | ||
|
||
public class StaticAccessControlMetadataFactory |
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.
Let's put this inside StaticAccessControlMetadataModule
so that it's consistent with SqlStandardAccessControlMetadataFactory
.
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 inlined that.
Thanks to this we can modify and access security access control metadata depending on which security model is used in Hive Connector.
dc5cdd3
to
80368b5
Compare
No description provided.