diff --git a/fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java b/fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java index 30e1f26fd..be3bf0dde 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java +++ b/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 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_)); - } - } } diff --git a/fe/src/main/java/com/cloudera/impala/analysis/BaseTableRef.java b/fe/src/main/java/com/cloudera/impala/analysis/BaseTableRef.java index 3b5ecaf08..7e9a9217d 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/BaseTableRef.java +++ b/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); diff --git a/fe/src/main/java/com/cloudera/impala/analysis/CollectionTableRef.java b/fe/src/main/java/com/cloudera/impala/analysis/CollectionTableRef.java index deeabb072..840ca9989 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/CollectionTableRef.java +++ b/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)) diff --git a/fe/src/main/java/com/cloudera/impala/analysis/FromClause.java b/fe/src/main/java/com/cloudera/impala/analysis/FromClause.java index 724fa471d..6062bc760 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/FromClause.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/FromClause.java @@ -92,7 +92,7 @@ public class FromClause implements ParseNode, Iterable { // 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(); diff --git a/fe/src/main/java/com/cloudera/impala/analysis/InlineViewRef.java b/fe/src/main/java/com/cloudera/impala/analysis/InlineViewRef.java index e80fe4b9e..50b712be7 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/InlineViewRef.java +++ b/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 diff --git a/fe/src/main/java/com/cloudera/impala/analysis/TableRef.java b/fe/src/main/java/com/cloudera/impala/analysis/TableRef.java index b2ae8a0df..c04c3f1c7 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/TableRef.java +++ b/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. diff --git a/fe/src/test/java/com/cloudera/impala/analysis/AuditingTest.java b/fe/src/test/java/com/cloudera/impala/analysis/AuditingTest.java index 6b3c426e9..3dbe9b9dc 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AuditingTest.java +++ b/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( diff --git a/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java b/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java index 99a8d17c2..9f371c741 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java +++ b/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"); diff --git a/testdata/datasets/functional/functional_schema_template.sql b/testdata/datasets/functional/functional_schema_template.sql index e14492e0f..44604f1d6 100644 --- a/testdata/datasets/functional/functional_schema_template.sql +++ b/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} diff --git a/testdata/datasets/functional/schema_constraints.csv b/testdata/datasets/functional/schema_constraints.csv index a49d445a7..29c70fe6b 100644 --- a/testdata/datasets/functional/schema_constraints.csv +++ b/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