jfsii commented on code in PR #3821:
URL: https://github.com/apache/hive/pull/3821#discussion_r1037478327
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -12550,6 +12550,21 @@ private ParseResult
rewriteASTWithMaskAndFilter(TableMask tableMask, ASTNode ast
}
}
+ void gatherUserSuppliedFunctions(ASTNode ast) {
+ int tokenType = ast.getToken().getType();
+ if (tokenType == HiveParser.TOK_FUNCTION ||
+ tokenType == HiveParser.TOK_FUNCTIONDI ||
+ tokenType == HiveParser.TOK_FUNCTIONSTAR) {
+ if (ast.getChild(0).getType() == HiveParser.Identifier) {
+ // maybe user supplied
+ this.userSuppliedFunctions.add(ast.getChild(0).getText());
Review Comment:
I'm not sure of Hive's stance on this - but I usually only use this when
forced to. Like in a setter in which the in parameter is named the same as the
object variable. Feel free to ignore if you disagree.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -12550,6 +12550,21 @@ private ParseResult
rewriteASTWithMaskAndFilter(TableMask tableMask, ASTNode ast
}
}
+ void gatherUserSuppliedFunctions(ASTNode ast) {
+ int tokenType = ast.getToken().getType();
+ if (tokenType == HiveParser.TOK_FUNCTION ||
+ tokenType == HiveParser.TOK_FUNCTIONDI ||
+ tokenType == HiveParser.TOK_FUNCTIONSTAR) {
+ if (ast.getChild(0).getType() == HiveParser.Identifier) {
+ // maybe user supplied
Review Comment:
Go ahead and remove this comment, I'm not sure why I added it.
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java:
##########
@@ -139,6 +139,13 @@ private static List<HivePrivilegeObject>
getHivePrivObjects(List<? extends Entit
continue;
}
+ if (privObject.getTyp() == Type.FUNCTION &&
!HiveConf.getBoolVar(SessionState.get().getConf(),
+ HiveConf.ConfVars.HIVE_AUTHORIZATION_FUNCTIONS_IN_VIEW) &&
hiveOpType == HiveOperationType.QUERY) {
+ if
(!sem.getUserSuppliedFunctions().contains(privObject.getFunctionName())) {
+ continue;
Review Comment:
Is it possible to add test cases that show the behavior in a couple of cases
(maybe similar to the authorization_view_*.q tests?). Ideally - I would like to
see a query with a user supplied udf with a table, a view, and see how it also
interacts with MVs. Ideally the q.out file would show which objects are being
sent for authorization so it can catch the regression on when it sends an extra
UDFs for auth (or doesn't send the user supplied one for auth).
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1438,6 +1442,10 @@ public void setUpdateColumnAccessInfo(ColumnAccessInfo
updateColumnAccessInfo) {
this.updateColumnAccessInfo = updateColumnAccessInfo;
}
+ public Set<String> getUserSuppliedFunctions() {
Review Comment:
Maybe add java doc with a note that it contains user supplied functions -
and note that it doesn't contain UDFs defined in views, etc.
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java:
##########
@@ -98,7 +98,7 @@ private static void addPermanentFunctionEntities(SessionState
ss, List<ReadEntit
}
private static List<HivePrivilegeObject> getHivePrivObjects(List<? extends
Entity> privObjects,
- Map<String, List<String>> tableName2Cols, HiveOperationType hiveOpType)
throws HiveException {
+ Map<String, List<String>> tableName2Cols, HiveOperationType hiveOpType,
BaseSemanticAnalyzer sem) throws HiveException {
List<HivePrivilegeObject> hivePrivobjs = new
ArrayList<HivePrivilegeObject>();
if (privObjects == null){
Review Comment:
nit: If I was editing this method - I would maybe consider making the format
uniform within in it.
I.E. it seems to have at least 3 different coding styles.
if (conditional){
if(conditional){
if (conditional) {
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -3599,6 +3599,9 @@ public static enum ConfVars {
HIVE_AUTHORIZATION_TABLES_ON_STORAGEHANDLERS("hive.security.authorization.tables.on.storagehandlers",
true,
"Enables authorization on tables with custom storage handlers as
implemented by HIVE-24705. " +
"Default setting is true. Useful for turning the feature off if the
corresponding ranger patch is missing."),
+
HIVE_AUTHORIZATION_FUNCTIONS_IN_VIEW("hive.security.authorization.functions.in.view",
true, "" +
Review Comment:
Is the "" + needed?
--
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]
