Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 )
Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2654 PS3, Line 2654: /** > Missing java doc for the two new methods Done http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java File fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java@77 PS3, Line 77: ()); > What happens when we register a privilege for a db which doesn't exist? The It is possible that the user does not have the permission to list databases on the server, but has the permission to use a specific database. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java File fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@86 PS3, Line 86: // Start with > Is this registering the priv twice if the function exists and if exists cla The registerFnPriv at line 116 is only called if ifExists_ is true to "raise" the requested privilege from ANY do DROP. Added some comments about this. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@94 PS3, Line 94: throw new AnalysisException(Analyzer.DB_DOES_NOT_EXIST_ERROR_MSG + desc_.dbName()); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@97 PS3, Line 97: (ifExists_) return; : throw new AnalysisException( > Can we make this section cleaner by creating a method which checks for exis I was thinking about something similar, but it seemed tricky to preserve the correct error messages: - If the user has no right to know the functions's existence, then "function does not exist" messages have to be hidden by registering the privilege first (the calling logic will give precedence to authorization errors). - Meanwhile if there is no authorization problem, but the function or db does not exist, then a correct informative error should be returned. I wanted to avoid changing these things too much to avoid breaking even more tests. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@119 PS3, Line 119: > this method can be private. Done http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@116 PS3, Line 116: build()); > What is the behavior when the table doesn't exist and a privilege request i As far as I understand Sentry it simply means that you don't have privileges for the given object (but you can still have privileges for the hierarchies above it like DB and server). Sentry doesn't care about the actual objects, only about privileges. http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py@149 PS3, Line 149: unique_ > is this argument needed? Done http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py@182 PS3, Line 182: _execute_drop_if_exists > Do you think we need a additional test which exercises the drop if exists f I see several holes in the testing of privileges but I would prefer to move this to a different Jira. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 27 Aug 2019 14:26:55 +0000 Gerrit-HasComments: Yes
