Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 )
Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege ...................................................................... Patch Set 4: (4 comments) My comments are mainly related to the DELETE t1 FROM t1, t2 WHERE ... case. t1 should need ALL privileges in this case and must be a Kudu table, while t2 should only need SELECT on the referred columns and can be any kind of table. http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java File fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java: http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@128 PS4, Line 128: fromClause_.analyze(analyzer, Privilege.ALL); I think that only the target table needs Privilege.ALL, other tables are ok with Privilege.SELECT (even select privilege for the referred columns should be enough). http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@154 PS4, Line 154: targetTableRef_ The ALL privilege for the target table could be added here. http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java: http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@423 PS4, Line 423: new TAccessEvent("functional_kudu.alltypes", TCatalogObjectType.TABLE, "ALL"), Do we really need ALL privileges for functional_kudu.alltypes? This table is only used in the from + where clause, but it will not be modified the delete. http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1924 PS4, Line 1924: .error(accessError("functional_kudu.alltypes"), onDatabase("functional_kudu", : allExcept(TPrivilegeLevel.ALL))) : .error(accessError("functional_kudu.alltypes"), onTable( : "functional_kudu", "alltypes", allExcept(TPrivilegeLevel.ALL))); nit: I think that similar tests could be a bit more readable by trying to break lines at specific points, for example before a new onDatabase/onTable calls. -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Tue, 31 Jul 2018 12:37:40 +0000 Gerrit-HasComments: Yes
