Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9738 )
Change subject: IMPALA-6674: Add CREATE fine-grained privilege ...................................................................... Patch Set 10: (14 comments) http://gerrit.cloudera.org:8080/#/c/9738/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9738/10//COMMIT_MSG@7 PS10, Line 7: IMPALA-6674: Add CREATE fine-grained privilege Wrong JIRA? http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@422 PS10, Line 422: // authorization is disabled. For example: when a user tries to create a function I prefer the previous behavior because I think it is more accurate. You should only get the "Cannot modify system database" error if you actually try to create a function in the system database "_impala_builtins". If you try to create a function with the same name as a built-in in a regular database, then you should get an AnalysisException. Example: Suppose the user starts by issuing this command: create function _impala_builtins.concat ... If we accept this change, then we'll get the "same built-in name" error message, but even if the user changes the name of the function, the function creation will still not succeed because it's invalid to create a function in the "impala_builtins" database. So indeed, the more severe problem is that the user is trying to modify a system database (even if a non-builtin function name was used). However, I dug into this and found that the following case does not behave as expected by me above: create function sin(double) ... That indeed gives a "Cannot modify system database" error message. To me, that is clearly the bug that needs to be addressed here. It looks like it goes wrong in FunctionName.analyze() where the behavior is not quite right for the CREATE case. If you prefer we can deal with that bug separately in a different patch/JIRA. http://gerrit.cloudera.org:8080/#/c/9738/10/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/9738/10/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@273 PS10, Line 273: * 4. The privilege level is not CREATE. The privilege level is not supported on tables, e.g. CREATE http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@130 PS10, Line 130: "User '%s' does not have privileges to CREATE/DROP functions on: %s", CREATE/DROP functions in http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@187 PS10, Line 187: // AuthorizeableFn is a special case where there is no scope in Sentry for function. Comment can be improved. Some suggestions below. AuthorizeableFn is special due to Sentry not having the concept of a function in DBModelAuthorizable.AuthorizableType. As a result, the list of authorizables for an AuthorizeableFn only contains the server and database, but not the function itself. So there is no need to remove the last authorizable here. http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@188 PS10, Line 188: // AuthorizeableFn's authorization hierarchy only consists of database. Hence we We should not forget to fix this in Sentry. http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizeableFn.java File fe/src/main/java/org/apache/impala/authorization/AuthorizeableFn.java: http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizeableFn.java@37 PS10, Line 37: Preconditions.checkState(fnName != null && !fnName.isEmpty()); use Strings.isNullOrEmpty() here also http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/main/java/org/apache/impala/authorization/AuthorizeableFn.java@44 PS10, Line 44: return Lists.newArrayList((DBModelAuthorizable) database_); No need to cast http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java: http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@88 PS10, Line 88: // SELECT, REFRESH, CREATE permissions on 'functional.alltypesagg' (no INSERT Change looks wrong since there is no CREATE permission at the table level. http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@106 PS10, Line 106: // REFRESH, CREATE permissions on 'functional_text_lzo' database also INSERT? http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@988 PS10, Line 988: // User has CREATE privilege on functional_text_lzo database and SELECT and INSERT Why is INSERT relevant for functional.alltypesagg? http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@995 PS10, Line 995: // User has CREATE privilege on functional_text_lzo database but no SELECT and INSERT Why is INSERT relevant for functional.alltypes? http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1101 PS10, Line 1101: // User has CREATE privilege on functional_text_lzo database and SELECT privilege But the test is creating a view in the tpch database http://gerrit.cloudera.org:8080/#/c/9738/10/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1199 PS10, Line 1199: String roleName = "create_role"; We should also have a test that CREATE on server allows creating tables in any database (right?). Not sure where this test should go, could just be a new test. -- To view, visit http://gerrit.cloudera.org:8080/9738 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id540e78fc9201fc1b4e6cac9b81ea54b8ae9eecd Gerrit-Change-Number: 9738 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Adam Holley <g...@holleyism.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Comment-Date: Thu, 22 Mar 2018 05:57:32 +0000 Gerrit-HasComments: Yes
