Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19194 )
Change subject: IMPALA-10986: Require the SELECT privilege to execute a UDF ...................................................................... Patch Set 15: (6 comments) Just some minor comments. The main concern is whether it is feasible to add a new privilege keyword EXECUTE or USE instead of reusing SELECT. http://gerrit.cloudera.org:8080/#/c/19194/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19194/15//COMMIT_MSG@7 PS15, Line 7: SELECT It seems the proper keyword should be EXECUTE or USE. SELECT may imply the UDF can't insert a row. http://gerrit.cloudera.org:8080/#/c/19194/15/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/19194/15/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@347 PS15, Line 347: dbName_ = path.get(0); : functionName_.setDb(path.get(0)); : functionName_.setFunction(path.get(1)); nit. May just use "*" at RHS or in arguments for readability. http://gerrit.cloudera.org:8080/#/c/19194/15/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@358 PS15, Line 358: // Need to set up 'dbName_', which in turn is used to set up 'db_name' of the : // TPrivilege in createTPrivilege() nit. Suggest to move to line 343 to cover both *.* and non *.* cases. http://gerrit.cloudera.org:8080/#/c/19194/15/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/19194/15/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@579 PS15, Line 579: .error(accessError("functional"), : onUdf("functional", "f", TPrivilegeLevel.SELECT)) should error() be ok() here, since functional.f is automatically SELECTable privilege-wise? http://gerrit.cloudera.org:8080/#/c/19194/15/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/19194/15/tests/authorization/test_ranger.py@482 PS15, Line 482: a wildcard for functional name nit. functional name in wildcard http://gerrit.cloudera.org:8080/#/c/19194/15/tests/authorization/test_ranger.py@521 PS15, Line 521: # Revoke the granted privilege and verify. : admin_client.execute("revoke select on user_defined_fn {0}.{1} from {2} {3}" : .format(unique_database, udf, kw, id)) : result = self.client.execute("show grant {0} {1} on user_defined_fn {2}.{3}" : .format(kw, id, unique_database, udf)) : TestRanger._check_privileges(result, []) Repetition of code from line 483 to 526. May refactor these tests to use the pattern from line 522 to 526. -- To view, visit http://gerrit.cloudera.org:8080/19194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e58ba30545ce169786aac279b00c8f6e09ae740 Gerrit-Change-Number: 19194 Gerrit-PatchSet: 15 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 09 Dec 2022 17:46:19 +0000 Gerrit-HasComments: Yes
