Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15123 )
Change subject: IMPALA-7002: Throw AuthorizationException when user accessing non-existent table/database in CTE without any privilege. ...................................................................... Patch Set 6: (1 comment) Hi Bikram and Wenzhe, I left some comments for WithClause.java. Let me know if you have any thoughts. Thanks! http://gerrit.cloudera.org:8080/#/c/15123/6/fe/src/main/java/org/apache/impala/analysis/WithClause.java File fe/src/main/java/org/apache/impala/analysis/WithClause.java: http://gerrit.cloudera.org:8080/#/c/15123/6/fe/src/main/java/org/apache/impala/analysis/WithClause.java@96 PS6, Line 96: finally { : // Register all privilege requests made from the root analyzer to the input : // analyzer so that caller could do authorization for all the requests collected : // during analysis and report an authorization error over an analysis error. : // We should not accidentally reveal the non-existence of a database/table if : // the user is not authorized. : for (PrivilegeRequest req : withClauseAnalyzer.getPrivilegeReqs()) { : analyzer.registerPrivReq(req); : } : } After taking a closer look at this patch, I have a question about what we should do in this finally block. Specifically, currently I do not have a concrete answer to whether or not we should register all local views as well as record audit events in that finally block. Recall that after analyze(stmtTableCache) at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java#L415, authzChecker.authorize() will be invoked at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java#L431. After the fix in this patch, when a user is trying to access a non-existing table of which the corresponding privileges were not granted, authzChecker.authorize() would throw an AuthorizationException, which in turn will then be caught at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java#L432. Since AuthorizationExceptions take precedence over AnalysisExceptions according to https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java#L440-L441, it seems okay that we do not register all local views with 'analyzer' and not record audit events, because in the end the user will only see an AuthorizationException. Maybe I am overthinking, but what is not that clear to me now is when there is a policy granting the user the SELECT privilege of that non-exisiting table. (Note that when the authorization provider is Ranger, it is possible to create such a policy via Ranger's web UI instead of executing a GRANT statement.) In this case, authzChecker.authorize() may not throw an AuthorizationException. If so, then I do not know whether or not we should register all local views and record audit events. I guess it depends on how we want Impala to behave in this very special case. Specifically, we may want to determine whether or not we would like Impala to store the information about local views and audit events in this special case. -- To view, visit http://gerrit.cloudera.org:8080/15123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia6b657a7147a136198a9a97a679c9131ee814577 Gerrit-Change-Number: 15123 Gerrit-PatchSet: 6 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Fri, 07 Feb 2020 03:34:08 +0000 Gerrit-HasComments: Yes
