khorgath has commented on the revision "
HIVE-3705 [jira] Adding authorization capability to the metastore".
Replied to a few of the review comments.
conf/hive-default.xml.template:1269 Agreed, changing.
conf/hive-default.xml.template:1284 Agreed, changing
ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 Not true. The handler that is set is used to do things like fetch dbs, fetch privilege bitmaps, etc for the Default authorizer, and in general, as an interface point, it is an important bit of functionality to extend to the authorization managers that run on the metastore-side to check metadata. Without this, a metastore-side authorization provider will wind up making much more costly calls (instantiating a hive object/etc) to do any metadata fetches.
ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java:25 Agreed, done.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:62 It was because we want some HiveConf parameters from a HiveConf object, but the PreEventListener interface is set up to be instantiated by a Configuration object as opposed to a HiveConf object. In implementation, it is instantiated with a HiveConf which is-a Configuration, and thus, we can get away with it, but this is a case of relying on implementation as opposed to interface. That said, this is easily fixed by HiveConf.getVar used by MetastoreUtils, so I was avoiding a nonexistant problem. Fixed.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:64 I did not want to leave metastore-auth to a side-effect of configuring the AuthorizationPreListener, and wanted to have the metastore-side auth config to mimic client-side auth config, but I see the benefit in what you're suggesting. I'll change it.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:112 The listener can only act on the types of events that a listener is triggered for. We'd have to follow it up by changing so that grant/revokes also cause events. But yes, will change javadoc to reflect. And yes, your understanding is correct, a user can still use a DefaultHiveMetastoreAuthProvider and grant privileges to themselves with this patch. We'd have to change that further as we improve it.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:125 Good point, changing.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:232 Agreed, was a result of an eclipse refactor default, changed.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:250 Agreed, was a result of an eclipse refactor default, changed.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 This is needed because leaving it as null(as the case will be with Partition objects yet to be created) will cause a NullPointerException when trying to construct a ql.metadata.Partition, which assumes a .getSd() will not return null. Given that we then need to pick a default, the table's .getSd() is a useful default for when the Partition object has not yet been populated. This way, authorizaton providers can check the table directory for permissions if they need to, rather than being faced with a null, which is not useful. Also, the only place this is used is for Authorization, so it does not affect the rest of the codeflow.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java:30 This would be a bad idea because you're being passed a Configuration object. I understand that in practice you'll likely get a HiveConf object, but this type of runtime casting is asking for trouble later on when someone instantiates it with a Configuration object.
We can do runtime checks to see if we got passed a HiveConf object, in which case we retain it, or if we got a Configuration object, in which case we create a new HiveConf, but that leads to kludgy code.
If we want to prevent instantiating a HiveConf object, then either Hive.get needs to change to accept a Configuration, or HiveAuthorizationProvider.init interface needs to change to require a HiveConf. Also, this is not new code, this is taken verbatim from HiveAuthorizationProviderBase that already did this.
To: JIRA, ashutoshc, khorgath