[impala] 04/05: IMPALA-7002: Throw AuthorizationException when user accesses non-existent table/database in CTE without required privileges.

[jo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 8ddbc1807a641587a05c2f5327c498f92acb935c
Author: wzhou-code <wz...@cloudera.com>
AuthorDate: Mon Jan 27 16:35:54 2020 -0800

    IMPALA-7002: Throw AuthorizationException when user accesses
    non-existent table/database in CTE without required privileges.
    
    Currently if a user without required privileges tries to access a
    non-existent database or table, then impala returns an analysis
    exception instead of authorization exception. This happens because
    during analysis of the with clause, the authorization request does
    not get registered due to analysis exception being thrown before it.
    This patch makes sure that those requests get registered regardless.
    
    Testing:
     - Manual test:
       - ran CTE with non-existent database/table in impala-shell
         without required privilege, verified that it results in
         AuthorizationException.
       - ran CTE with non-existent database/table in impala-shell
         with the required privilege, verified that it results
         in AnalysisException.
     - Added CTE test cases for non-existent database/table/column
       in AuthorizationStmtTest.
     - Passed all FE tests.
     - Passed all core tests.
    
    Change-Id: Ia6b657a7147a136198a9a97a679c9131ee814577
    Reviewed-on: http://gerrit.cloudera.org:8080/15123
    Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../org/apache/impala/analysis/WithClause.java     | 39 +++++++++++++---------
 .../authorization/AuthorizationStmtTest.java       | 18 ++++++++++
 2 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/analysis/WithClause.java b/fe/src/main/java/org/apache/impala/analysis/WithClause.java
index 9459ab3..5721065 100644
--- a/fe/src/main/java/org/apache/impala/analysis/WithClause.java
+++ b/fe/src/main/java/org/apache/impala/analysis/WithClause.java
@@ -77,23 +77,30 @@ public class WithClause extends StmtNode {
     Analyzer withClauseAnalyzer = Analyzer.createWithNewGlobalState(analyzer);
     withClauseAnalyzer.setHasWithClause();
     if (analyzer.isExplain()) withClauseAnalyzer.setIsExplain();
-    for (View view: views_) {
-      Analyzer viewAnalyzer = new Analyzer(withClauseAnalyzer);
-      view.getQueryStmt().analyze(viewAnalyzer);
-      // Register this view so that the next view can reference it.
-      withClauseAnalyzer.registerLocalView(view);
-    }
-    // Register all local views with the analyzer.
-    for (FeView localView: withClauseAnalyzer.getLocalViews().values()) {
-      analyzer.registerLocalView(localView);
+    try {
+      for (View view: views_) {
+        Analyzer viewAnalyzer = new Analyzer(withClauseAnalyzer);
+        view.getQueryStmt().analyze(viewAnalyzer);
+        // Register this view so that the next view can reference it.
+        withClauseAnalyzer.registerLocalView(view);
+      }
+      // Register all local views with the analyzer.
+      for (FeView localView: withClauseAnalyzer.getLocalViews().values()) {
+        analyzer.registerLocalView(localView);
+      }
+      // Record audit events because the resolved table references won't generate any
+      // when a view is referenced.
+      analyzer.getAccessEvents().addAll(withClauseAnalyzer.getAccessEvents());
     }
-    // Record audit events because the resolved table references won't generate any
-    // when a view is referenced.
-    analyzer.getAccessEvents().addAll(withClauseAnalyzer.getAccessEvents());
-
-    // Register all privilege requests made from the root analyzer.
-    for (PrivilegeRequest req: withClauseAnalyzer.getPrivilegeReqs()) {
-      analyzer.registerPrivReq(req);
+    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);
+      }
     }
   }
 
diff --git a/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
index de4510e..c220a89 100644
--- a/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
+++ b/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
@@ -594,6 +594,24 @@ public class AuthorizationStmtTest extends AuthorizationTestBase {
     authorize("select 1 from functional.notbl")
         .error(selectError("functional.notbl"));
 
+    // Select from non-existent database in CTE without required privileges
+    authorize("with t as (select id from nodb.alltypes) select * from t")
+        .error(selectError("nodb.alltypes"));
+
+    // Select from non-existent table in CTE without required privileges
+    authorize("with t as (select id from functional.notbl) select * from t")
+        .error(selectError("functional.notbl"));
+
+    // Select from non-existent column in CTE without required privileges
+    authorize("with t as (select nocol from functional.alltypes) select * from t")
+        .error(selectError("functional.alltypes"))
+        .error(selectError("functional.alltypes"), onColumn("functional", "alltypes",
+            ALLTYPES_COLUMNS, TPrivilegeLevel.SELECT));
+
+    // With clause column labels exceeding the number of columns in the query
+    authorize("with t(c1, c2) as (select id from functional.alltypes) select * from t")
+        .error(selectError("functional.alltypes"));
+
     // Select with inline view.
     authorize("select a.* from (select * from functional.alltypes) a")
         .ok(onServer(TPrivilegeLevel.ALL))