IMPALA-3915: Register privilege and audit requests when analyzing resolved table refs.

The bug: We used to register privilege requests for table refs in TableRef.analyze()
which only got called for unresolved TableRefs. As a result, a reference to a view that
contains a subquery did not get properly authorized, explained as follows.
1. In the first analysis pass the view is replaced by a an InlineViewRef and we
   correctly register an authorizarion request.
2. We rewrite the subquery via the StmtRewriter and wipe the analysis state, but
   preserve the InlineViewRef that replaces the view reference.
3. The rewritten statement is analyzed again, but since an InlineViewRef is
   considered to be resolved, we never call TableRef.analyze(), and hence
   never register an authorization event for the view.

The fix: We now register authorization and auditing events when calling analyze() on a
resolved TableRef (BaseTableRef, InlineViewRef, CollectionTableRef).

Change-Id: I18fa8af9a94ce190c5a3c29c3221c659a2ace659
Reviewed-on: http://gerrit.cloudera.org:8080/3783
Reviewed-by: Alex Behm <alex.behm@cloudera.com>
Tested-by: Internal Jenkins

fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java

@@ -502,8 +502,11 @@ public class Analyzer {
   /**
    * Resolves the given TableRef into a concrete BaseTableRef, ViewRef or
    * CollectionTableRef. Returns the new resolved table ref or the given table
-   * ref if it is already resolved. Adds audit events and privilege requests for
-   * the database and/or table.
+   * ref if it is already resolved.
+   * Registers privilege requests and throws an AnalysisException if the tableRef's
+   * path could not be resolved. The privilege requests are added to ensure that
+   * an AuthorizationException is preferred over an AnalysisException so as not to
+   * accidentally reveal the non-existence of tables/databases.
    */
   public TableRef resolveTableRef(TableRef tableRef) throws AnalysisException {
     // Return the table if it is already resolved.
@@ -523,8 +526,31 @@ public class Analyzer {
 
     // Resolve the table ref's path and determine what resolved table ref
     // to replace it with.
-    tableRef.analyze(this);
-    Path resolvedPath = tableRef.getResolvedPath();
+    List<String> rawPath = tableRef.getPath();
+    Path resolvedPath = null;
+    try {
+      resolvedPath = resolvePath(tableRef.getPath(), PathType.TABLE_REF);
+    } catch (AnalysisException e) {
+      if (!hasMissingTbls()) {
+        // Register privilege requests to prefer reporting an authorization error over
+        // an analysis error. We should not accidentally reveal the non-existence of a
+        // table/database if the user is not authorized.
+        if (rawPath.size() > 1) {
+          registerPrivReq(new PrivilegeRequestBuilder()
+              .onTable(rawPath.get(0), rawPath.get(1))
+              .allOf(Privilege.SELECT).toRequest());
+        }
+        registerPrivReq(new PrivilegeRequestBuilder()
+            .onTable(getDefaultDb(), rawPath.get(0))
+            .allOf(Privilege.SELECT).toRequest());
+      }
+      throw e;
+    } catch (TableLoadingException e) {
+      throw new AnalysisException(String.format(
+          "Failed to load metadata for table: '%s'", Joiner.on(".").join(rawPath)), e);
+    }
+
+    Preconditions.checkNotNull(resolvedPath);
     if (resolvedPath.destTable() != null) {
       Table table = resolvedPath.destTable();
       Preconditions.checkNotNull(table);
@@ -534,9 +560,9 @@ public class Analyzer {
           table instanceof KuduTable ||
           table instanceof HBaseTable ||
           table instanceof DataSourceTable);
-      return new BaseTableRef(tableRef);
+      return new BaseTableRef(tableRef, resolvedPath);
     } else {
-      return new CollectionTableRef(tableRef);
+      return new CollectionTableRef(tableRef, resolvedPath);
     }
   }
 
@@ -2333,10 +2359,9 @@ public class Analyzer {
    * Privilege level. The privilege request is tracked in the analyzer
    * and authorized post-analysis.
    *
-   * If the database does not exist in the catalog an AnalysisError is thrown.
+   * Registers a new access event if the catalog lookup was successful.
    *
-   * If addAccessEvent is true, this call will add a new entry to accessEvents if the
-   * catalog access was successful. If false, no accessEvent will be added.
+   * If the database does not exist in the catalog an AnalysisError is thrown.
    */
   public Db getDb(String dbName, Privilege privilege) throws AnalysisException {
     return getDb(dbName, privilege, true);
@@ -2486,6 +2511,51 @@ public class Analyzer {
     globalState_.warnings.put(msg, count + 1);
   }
 
+  /**
+   * Registers a new PrivilegeRequest in the analyzer. If authErrorMsg_ is set,
+   * the privilege request will be added to the list of "masked" privilege requests,
+   * using authErrorMsg_ as the auth failure error message. Otherwise it will get
+   * added as a normal privilege request that will use the standard error message
+   * on authorization failure.
+   * If enablePrivChecks_ is false, the registration request will be ignored. This
+   * is used when analyzing catalog views since users should be able to query a view
+   * even if they do not have privileges on the underlying tables.
+   */
+  public void registerPrivReq(PrivilegeRequest privReq) {
+    if (!enablePrivChecks_) return;
+
+    if (Strings.isNullOrEmpty(authErrorMsg_)) {
+      globalState_.privilegeReqs.add(privReq);
+    } else {
+      globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, authErrorMsg_));
+    }
+  }
+
+  /**
+   * Registers a table-level SELECT privilege request and an access event for auditing
+   * for the given table. The given table must be a base table or a catalog view (not
+   * a local view).
+   */
+  public void registerAuthAndAuditEvent(Table table, Analyzer analyzer) {
+    // Add access event for auditing.
+    if (table instanceof View) {
+      View view = (View) table;
+      Preconditions.checkState(!view.isLocalView());
+      analyzer.addAccessEvent(new TAccessEvent(
+          table.getFullName(), TCatalogObjectType.VIEW,
+          Privilege.SELECT.toString()));
+    } else {
+      analyzer.addAccessEvent(new TAccessEvent(
+          table.getFullName(), TCatalogObjectType.TABLE,
+          Privilege.SELECT.toString()));
+    }
+    // Add privilege request.
+    TableName tableName = table.getTableName();
+    analyzer.registerPrivReq(new PrivilegeRequestBuilder()
+        .onTable(tableName.getDb(), tableName.getTbl())
+        .allOf(Privilege.SELECT).toRequest());
+  }
+
   /**
    * Efficiently computes and stores the transitive closure of the value transfer graph
    * of slots. After calling computeValueTransfers(), value transfers between slots can
@@ -2851,24 +2921,4 @@ public class Analyzer {
       return expectedStr.equals(actualStr);
     }
   }
-
-  /**
-   * Registers a new PrivilegeRequest in the analyzer. If authErrorMsg_ is set,
-   * the privilege request will be added to the list of "masked" privilege requests,
-   * using authErrorMsg_ as the auth failure error message. Otherwise it will get
-   * added as a normal privilege request that will use the standard error message
-   * on authorization failure.
-   * If enablePrivChecks_ is false, the registration request will be ignored. This
-   * is used when analyzing catalog views since users should be able to query a view
-   * even if they do not have privileges on the underlying tables.
-   */
-  public void registerPrivReq(PrivilegeRequest privReq) {
-    if (!enablePrivChecks_) return;
-
-    if (Strings.isNullOrEmpty(authErrorMsg_)) {
-      globalState_.privilegeReqs.add(privReq);
-    } else {
-      globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, authErrorMsg_));
-    }
-  }
 }

fe/src/main/java/com/cloudera/impala/analysis/BaseTableRef.java

@@ -14,10 +14,9 @@
 
 package com.cloudera.impala.analysis;
 
-import com.cloudera.impala.common.AnalysisException;
-import com.cloudera.impala.catalog.HdfsFileFormat;
 import com.cloudera.impala.catalog.HdfsTable;
 import com.cloudera.impala.catalog.Table;
+import com.cloudera.impala.common.AnalysisException;
 import com.google.common.base.Preconditions;
 
 /**
@@ -26,10 +25,16 @@ import com.google.common.base.Preconditions;
  * of a SelectStmt.
  */
 public class BaseTableRef extends TableRef {
-  public BaseTableRef(TableRef tableRef) {
+
+  /**
+   * Create a BaseTableRef from the original unresolved table ref as well as
+   * its resolved path. Sets table aliases and join-related attributes.
+   */
+  public BaseTableRef(TableRef tableRef, Path resolvedPath) {
     super(tableRef);
-    Preconditions.checkState(resolvedPath_.isResolved());
-    Preconditions.checkState(resolvedPath_.isRootedAtTable());
+    Preconditions.checkState(resolvedPath.isResolved());
+    Preconditions.checkState(resolvedPath.isRootedAtTable());
+    resolvedPath_ = resolvedPath;
     // Set implicit aliases if no explicit one was given.
     if (hasExplicitAlias()) return;
     aliases_ = new String[] {
@@ -50,7 +55,7 @@ public class BaseTableRef extends TableRef {
   @Override
   public void analyze(Analyzer analyzer) throws AnalysisException {
     if (isAnalyzed_) return;
-    Preconditions.checkNotNull(getPrivilegeRequirement());
+    analyzer.registerAuthAndAuditEvent(resolvedPath_.getRootTable(), analyzer);
     desc_ = analyzer.registerTableRef(this);
     isAnalyzed_ = true;
     analyzeHints(analyzer);

fe/src/main/java/com/cloudera/impala/analysis/CollectionTableRef.java

@@ -42,12 +42,13 @@ public class CollectionTableRef extends TableRef {
   /////////////////////////////////////////
 
   /**
-   * Create a CollectionTableRef for the given collection type from the original
-   * unresolved table ref. Sets the explicit alias and the join-related attributes
-   * of the new collection table ref from the unresolved table ref.
+   * Create a CollectionTableRef from the original unresolved table ref as well as
+   * its resolved path. Sets table aliases and join-related attributes.
    */
-  public CollectionTableRef(TableRef tableRef) {
+  public CollectionTableRef(TableRef tableRef, Path resolvedPath) {
     super(tableRef);
+    Preconditions.checkState(resolvedPath.isResolved());
+    resolvedPath_ = resolvedPath;
     // Use the last path element as an implicit alias if no explicit alias was given.
     if (hasExplicitAlias()) return;
     String implicitAlias = rawPath_.get(rawPath_.size() - 1).toLowerCase();
@@ -76,7 +77,6 @@ public class CollectionTableRef extends TableRef {
   @Override
   public void analyze(Analyzer analyzer) throws AnalysisException {
     if (isAnalyzed_) return;
-    Preconditions.checkNotNull(getPrivilegeRequirement());
     desc_ = analyzer.registerTableRef(this);
     if (isRelative() && !analyzer.isWithClause()) {
       SlotDescriptor parentSlotDesc = analyzer.registerSlotRef(resolvedPath_);
@@ -97,7 +97,10 @@ public class CollectionTableRef extends TableRef {
       }
     }
     if (!isRelative()) {
-      // Register a column-level privilege request for the collection-typed column.
+      // Register a table-level privilege request as well as a column-level privilege request
+      // for the collection-typed column.
+      Preconditions.checkNotNull(resolvedPath_.getRootTable());
+      analyzer.registerAuthAndAuditEvent(resolvedPath_.getRootTable(), analyzer);
       analyzer.registerPrivReq(new PrivilegeRequestBuilder().
           allOf(Privilege.SELECT).onColumn(desc_.getTableName().getDb(),
           desc_.getTableName().getTbl(), desc_.getPath().getRawPath().get(0))

fe/src/main/java/com/cloudera/impala/analysis/FromClause.java

@@ -92,7 +92,7 @@ public class FromClause implements ParseNode, Iterable<TableRef> {
         // Use the fully qualified raw path to preserve the original resolution.
         // Otherwise, non-fully qualified paths might incorrectly match a local view.
         // TODO for 2.3: This full qualification preserves analysis state which is
-        // contraty to the intended semantics of reset(). We could address this issue by
+        // contrary to the intended semantics of reset(). We could address this issue by
         // changing the WITH-clause analysis to register local views that have
         // fully-qualified table refs, and then remove the full qualification here.
         newTblRef.rawPath_ = origTblRef.getResolvedPath().getFullyQualifiedRawPath();

fe/src/main/java/com/cloudera/impala/analysis/InlineViewRef.java

@@ -138,6 +138,7 @@ public class InlineViewRef extends TableRef {
     // Catalog views refs require special analysis settings for authorization.
     boolean isCatalogView = (view_ != null && !view_.isLocalView());
     if (isCatalogView) {
+      analyzer.registerAuthAndAuditEvent(view_, analyzer);
       if (inlineViewAnalyzer_.isExplain()) {
         // If the user does not have privileges on the view's definition
         // then we report a masked authorization error so as not to reveal

fe/src/main/java/com/cloudera/impala/analysis/TableRef.java

@@ -19,19 +19,12 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 
-import com.cloudera.impala.analysis.Path.PathType;
-import com.cloudera.impala.authorization.Privilege;
-import com.cloudera.impala.authorization.PrivilegeRequestBuilder;
 import com.cloudera.impala.catalog.HdfsTable;
 import com.cloudera.impala.catalog.Table;
-import com.cloudera.impala.catalog.TableLoadingException;
-import com.cloudera.impala.catalog.View;
 import com.cloudera.impala.common.AnalysisException;
 import com.cloudera.impala.common.Pair;
 import com.cloudera.impala.planner.JoinNode.DistributionMode;
 import com.cloudera.impala.planner.PlanNode;
-import com.cloudera.impala.thrift.TAccessEvent;
-import com.cloudera.impala.thrift.TCatalogObjectType;
 import com.cloudera.impala.thrift.TReplicaPreference;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
@@ -50,7 +43,7 @@ import com.google.common.collect.Sets;
  * 1. Resolution: A table ref's path is resolved and then the generic TableRef is
  * replaced by a concrete table ref (a BaseTableRef, CollectionTabeRef or ViewRef)
  * in the originating stmt and that is given the resolved path. This step is driven by
- * Analyzer.resolveTableRef() which calls into TableRef.analyze().
+ * Analyzer.resolveTableRef().
  *
  * 2. Analysis/registration: After resolution, the concrete table ref is analyzed
  * to register a tuple descriptor for its resolved path and register other table-ref
@@ -171,56 +164,10 @@ public class TableRef implements ParseNode {
     desc_ = other.desc_;
   }
 
-  /**
-   * Resolves this table ref's raw path and adds privilege requests and audit events
-   * for the referenced catalog entities.
-   */
   @Override
   public void analyze(Analyzer analyzer) throws AnalysisException {
-    try {
-      resolvedPath_ = analyzer.resolvePath(rawPath_, PathType.TABLE_REF);
-    } catch (AnalysisException e) {
-      if (!analyzer.hasMissingTbls()) {
-        // Register privilege requests to prefer reporting an authorization error over
-        // an analysis error. We should not accidentally reveal the non-existence of a
-        // table/database if the user is not authorized.
-        if (rawPath_.size() > 1) {
-          analyzer.registerPrivReq(new PrivilegeRequestBuilder()
-              .onTable(rawPath_.get(0), rawPath_.get(1))
-              .allOf(getPrivilegeRequirement()).toRequest());
-        }
-        analyzer.registerPrivReq(new PrivilegeRequestBuilder()
-            .onTable(analyzer.getDefaultDb(), rawPath_.get(0))
-            .allOf(getPrivilegeRequirement()).toRequest());
-      }
-      throw e;
-    } catch (TableLoadingException e) {
-      throw new AnalysisException(String.format(
-          "Failed to load metadata for table: '%s'", Joiner.on(".").join(rawPath_)), e);
-    }
-    Preconditions.checkState(resolvedPath_ != null);
-    if (resolvedPath_.isRootedAtTable()) {
-      // Add access event for auditing.
-      Table table = resolvedPath_.getRootTable();
-      if (table instanceof View) {
-        View view = (View) table;
-        if (!view.isLocalView()) {
-          analyzer.addAccessEvent(new TAccessEvent(
-              table.getFullName(), TCatalogObjectType.VIEW,
-              getPrivilegeRequirement().toString()));
-        }
-      } else {
-        analyzer.addAccessEvent(new TAccessEvent(
-            table.getFullName(), TCatalogObjectType.TABLE,
-            getPrivilegeRequirement().toString()));
-      }
-
-      // Add privilege requests for authorization.
-      TableName tableName = table.getTableName();
-      analyzer.registerPrivReq(new PrivilegeRequestBuilder()
-          .onTable(tableName.getDb(), tableName.getTbl())
-          .allOf(getPrivilegeRequirement()).toRequest());
-    }
+    throw new IllegalStateException(
+        "Should not call analyze() on an unresolved TableRef.");
   }
 
   /**
@@ -616,11 +563,6 @@ public class TableRef implements ParseNode {
     return output.toString();
   }
 
-  /**
-   * Gets the privilege requirement. This is always SELECT for TableRefs.
-   */
-  public Privilege getPrivilegeRequirement() { return Privilege.SELECT; }
-
   /**
    * Returns a deep clone of this table ref without also cloning the chain of table refs.
    * Sets leftTblRef_ in the returned clone to null.

fe/src/test/java/com/cloudera/impala/analysis/AuditingTest.java

@@ -54,7 +54,17 @@ public class AuditingTest extends AnalyzerTest {
         new TAccessEvent("functional.alltypes", TCatalogObjectType.TABLE, "SELECT")
         ));
 
-    // Select from an inline-view.
+    // Tests audit events after a statement has been rewritten (IMPALA-3915).
+    // Select from a view that contains a subquery.
+    accessEvents = AnalyzeAccessEvents("select * from functional_rc.subquery_view");
+    Assert.assertEquals(accessEvents, Sets.newHashSet(
+        new TAccessEvent("functional_rc.alltypessmall", TCatalogObjectType.TABLE, "SELECT"),
+        new TAccessEvent("functional_rc.alltypes", TCatalogObjectType.TABLE, "SELECT"),
+        new TAccessEvent("functional_rc.subquery_view", TCatalogObjectType.VIEW, "SELECT"),
+        new TAccessEvent("_impala_builtins", TCatalogObjectType.DATABASE, "VIEW_METADATA")
+        ));
+
+    // Select from an inline view.
     accessEvents = AnalyzeAccessEvents(
         "select a.* from (select * from functional.alltypesagg) a");
     Assert.assertEquals(accessEvents, Sets.newHashSet(

fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java

@@ -470,6 +470,15 @@ public class AuthorizationTest {
         "User '%s' does not have privileges to execute 'SELECT' on: " +
         "functional.alltypes");
 
+    // Tests authorization after a statement has been rewritten (IMPALA-3915).
+    // User has SELECT privileges on the view which contains a subquery.
+    AuthzOk("select * from functional_seq_snap.subquery_view");
+
+    // User does not have SELECT privileges on the view which contains a subquery.
+    AuthzError("select * from functional_rc.subquery_view",
+        "User '%s' does not have privileges to execute 'SELECT' on: " +
+        "functional_rc.subquery_view");
+
     // Constant select.
     AuthzOk("select 1");
 

testdata/datasets/functional/functional_schema_template.sql

@@ -859,6 +859,16 @@ AS SELECT * FROM {db_name}{db_suffix}.alltypes_view;
 ---- DATASET
 functional
 ---- BASE_TABLE_NAME
+subquery_view
+---- CREATE
+CREATE VIEW IF NOT EXISTS {db_name}{db_suffix}.{table_name}
+AS SELECT COUNT(*) FROM {db_name}{db_suffix}.alltypes
+WHERE id IN (SELECT id FROM {db_name}{db_suffix}.alltypessmall where int_col < 5);
+---- LOAD
+====
+---- DATASET
+functional
+---- BASE_TABLE_NAME
 alltypes_parens
 ---- CREATE
 CREATE VIEW IF NOT EXISTS {db_name}{db_suffix}.{table_name}

testdata/datasets/functional/schema_constraints.csv

@@ -95,6 +95,8 @@ table_name:complex_view, constraint:restrict_to, table_format:text/none/none
 table_name:complex_view, constraint:restrict_to, table_format:seq/snap/block
 table_name:view_view, constraint:restrict_to, table_format:text/none/none
 table_name:view_view, constraint:restrict_to, table_format:seq/snap/block
+table_name:subquery_view, constraint:restrict_to, table_format:seq/snap/block
+table_name:subquery_view, constraint:restrict_to, table_format:rc/none/none
 
 # liketbl and tblwithraggedcolumns all have
 # NULLs in primary key columns. hbase does not support