This is an automated email from the ASF dual-hosted git repository. tarmstrong pushed a commit to branch 2.x in repository https://gitbox.apache.org/repos/asf/impala.git
commit d1d0e88ea8db3f9e935f95c0b329dee42afb18b5 Author: Fredy Wijaya <[email protected]> AuthorDate: Tue Jul 10 20:09:48 2018 -0700 IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis In order to do COMPUTE STATS, Impala performs several SELECT queries. However, in the COMPUTE STATS analysis phase, we only check for the ALTER privilege. Although the SELECT privilege is eventually checked in the target table by the SELECT child queries, it is better to check the SELECT privilege in the COMPUTE STATS analysis phase and fail early if the privilege requirements are not met instead of failing later in the SELECT child queries due to insufficient privileges. Testing: - Updated the authorization tests - Ran all FE tests Change-Id: Icead43e689404a286859344e309427eb6c68646a Reviewed-on: http://gerrit.cloudera.org:8080/10918 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Reviewed-on: http://gerrit.cloudera.org:8080/13473 Reviewed-by: Fredy Wijaya <[email protected]> --- .../java/org/apache/impala/analysis/Analyzer.java | 34 ++++---- .../apache/impala/analysis/ComputeStatsStmt.java | 2 +- .../impala/analysis/DropTableOrViewStmt.java | 2 +- .../org/apache/impala/analysis/PartitionDef.java | 3 +- .../apache/impala/analysis/PartitionSpecBase.java | 2 +- .../org/apache/impala/analysis/AuditingTest.java | 7 +- .../impala/analysis/AuthorizationStmtTest.java | 94 +++++++++++++++------- 7 files changed, 93 insertions(+), 51 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java index 850005d..1a4ea13 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java +++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java @@ -2397,21 +2397,25 @@ public class Analyzer { * Returns the Table with the given name from the 'loadedTables' map in the global * analysis state. Throws an AnalysisException if the table or the db does not exist. * Throws a TableLoadingException if the registered table failed to load. - * Always registers a privilege request for the table at the given privilege level, + * Always registers privilege request(s) for the table at the given privilege level(s), * regardless of the state of the table (i.e. whether it exists, is loaded, etc.). - * If addAccessEvent is true adds an access event for successfully loaded tables. + * If addAccessEvent is true adds access event(s) for successfully loaded tables. When + * multiple privileges are specified, all those privileges will be required for the + * authorization check. */ - public FeTable getTable(TableName tableName, Privilege privilege, - boolean addAccessEvent) throws AnalysisException, TableLoadingException { + public FeTable getTable(TableName tableName, boolean addAccessEvent, + Privilege... privilege) throws AnalysisException, TableLoadingException { Preconditions.checkNotNull(tableName); Preconditions.checkNotNull(privilege); tableName = getFqTableName(tableName); - if (privilege == Privilege.ANY) { - registerPrivReq(new PrivilegeRequestBuilder() - .any().onAnyColumn(tableName.getDb(), tableName.getTbl()).toRequest()); - } else { - registerPrivReq(new PrivilegeRequestBuilder() - .allOf(privilege).onTable(tableName.getDb(), tableName.getTbl()).toRequest()); + for (Privilege priv : privilege) { + if (priv == Privilege.ANY) { + registerPrivReq(new PrivilegeRequestBuilder() + .any().onAnyColumn(tableName.getDb(), tableName.getTbl()).toRequest()); + } else { + registerPrivReq(new PrivilegeRequestBuilder() + .allOf(priv).onTable(tableName.getDb(), tableName.getTbl()).toRequest()); + } } FeTable table = getTable(tableName.getDb(), tableName.getTbl()); Preconditions.checkNotNull(table); @@ -2419,8 +2423,10 @@ public class Analyzer { // Add an audit event for this access TCatalogObjectType objectType = TCatalogObjectType.TABLE; if (table instanceof FeView) objectType = TCatalogObjectType.VIEW; - globalState_.accessEvents.add(new TAccessEvent( - tableName.toString(), objectType, privilege.toString())); + for (Privilege priv : privilege) { + globalState_.accessEvents.add(new TAccessEvent( + tableName.toString(), objectType, priv.toString())); + } } return table; } @@ -2433,10 +2439,10 @@ public class Analyzer { * AuthorizationException is thrown. * If the table or the db does not exist in the Catalog, an AnalysisError is thrown. */ - public FeTable getTable(TableName tableName, Privilege privilege) + public FeTable getTable(TableName tableName, Privilege... privilege) throws AnalysisException { try { - return getTable(tableName, privilege, true); + return getTable(tableName, true, privilege); } catch (TableLoadingException e) { throw new AnalysisException(e); } diff --git a/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java b/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java index 7743120..4f1173e 100644 --- a/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java @@ -343,7 +343,7 @@ public class ComputeStatsStmt extends StatementBase { throw new AnalysisException(String.format( "COMPUTE STATS not supported for nested collection: %s", tableName_)); } - table_ = analyzer.getTable(tableName_, Privilege.ALTER); + table_ = analyzer.getTable(tableName_, Privilege.ALTER, Privilege.SELECT); if (!(table_ instanceof FeFsTable)) { if (partitionSet_ != null) { diff --git a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java index 3e43a01..e38078a 100644 --- a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java @@ -96,7 +96,7 @@ public class DropTableOrViewStmt extends StatementBase { public void analyze(Analyzer analyzer) throws AnalysisException { dbName_ = analyzer.getTargetDbName(tableName_); try { - FeTable table = analyzer.getTable(tableName_, Privilege.DROP, true); + FeTable table = analyzer.getTable(tableName_, true, Privilege.DROP); Preconditions.checkNotNull(table); if (table instanceof FeView && dropTable_) { throw new AnalysisException(String.format( diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java b/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java index c8e0a4f..7b4f837 100644 --- a/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java +++ b/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java @@ -83,8 +83,7 @@ public class PartitionDef implements ParseNode { FeTable table; try { - table = analyzer.getTable(partitionSpec_.getTableName(), Privilege.ALTER, - false); + table = analyzer.getTable(partitionSpec_.getTableName(), false, Privilege.ALTER); } catch (TableLoadingException e) { throw new AnalysisException(e.getMessage(), e); } diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java index 4d432e2..ede438b 100644 --- a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java +++ b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java @@ -74,7 +74,7 @@ public abstract class PartitionSpecBase implements ParseNode { // be audited outside of the PartitionSpec. FeTable table; try { - table = analyzer.getTable(tableName_, privilegeRequirement_, false); + table = analyzer.getTable(tableName_, false, privilegeRequirement_); } catch (TableLoadingException e) { throw new AnalysisException(e.getMessage(), e); } diff --git a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java index cc91188..7735d3f 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java @@ -280,7 +280,9 @@ public class AuditingTest extends FrontendTestBase { "COMPUTE STATS functional_seq_snap.alltypes"); Assert.assertEquals(accessEvents, Sets.newHashSet( new TAccessEvent( - "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "ALTER"))); + "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "ALTER"), + new TAccessEvent( + "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "SELECT"))); } @Test @@ -443,7 +445,8 @@ public class AuditingTest extends FrontendTestBase { // Compute stats accessEvents = AnalyzeAccessEvents("compute stats functional_kudu.testtbl"); Assert.assertEquals(accessEvents, Sets.newHashSet( - new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALTER"))); + new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALTER"), + new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT"))); // Describe accessEvents = AnalyzeAccessEvents("describe functional_kudu.testtbl"); diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java index df46899..3095ff8 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java @@ -1150,38 +1150,72 @@ public class AuthorizationStmtTest extends FrontendTestBase { @Test public void testStats() throws ImpalaException { - for (String statsType: new String[]{"compute", "drop"}) { - authorize(String.format("%s stats functional.alltypes", statsType)) - .ok(onServer(TPrivilegeLevel.ALL)) - .ok(onServer(TPrivilegeLevel.ALTER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALL)) - .ok(onDatabase("functional", TPrivilegeLevel.ALTER)) - .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL)) - .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER)) - .error(alterError("functional.alltypes")) - .error(alterError("functional.alltypes"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) - .error(alterError("functional.alltypes"), onDatabase("functional", - allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) - .error(alterError("functional.alltypes"), onTable("functional", "alltypes", - allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))); + // Compute stats. + authorize("compute stats functional.alltypes") + .ok(onServer(TPrivilegeLevel.ALL)) + .ok(onServer(TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)) + .ok(onDatabase("functional", TPrivilegeLevel.ALL)) + .ok(onDatabase("functional", TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)) + .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL)) + .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER, + TPrivilegeLevel.SELECT)) + .error(alterError("functional.alltypes")) + .error(alterError("functional.alltypes"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))) + .error(alterError("functional.alltypes"), onDatabase("functional", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, + TPrivilegeLevel.SELECT))) + .error(alterError("functional.alltypes"), onTable("functional", "alltypes", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, + TPrivilegeLevel.SELECT))); + + // Compute stats on database that does not exist. + authorize("compute stats nodb.notbl") + .error(alterError("nodb.notbl")) + .error(alterError("nodb.notbl"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))) + .error(alterError("nodb.notbl"), onDatabase("nodb", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))); + + // Compute stats on table that does not exist. + authorize("compute stats functional.notbl") + .error(alterError("functional.notbl")) + .error(alterError("functional.notbl"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))) + .error(alterError("functional.notbl"), onDatabase("functional", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))); - // Database does not exist. - authorize(String.format("%s stats nodb.notbl", statsType)) - .error(alterError("nodb.notbl")) - .error(alterError("nodb.notbl"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) - .error(alterError("default.nodb"), onDatabase("nodb", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.ALL))); + // Drop stats. + authorize("drop stats functional.alltypes") + .ok(onServer(TPrivilegeLevel.ALL)) + .ok(onServer(TPrivilegeLevel.ALTER)) + .ok(onDatabase("functional", TPrivilegeLevel.ALL)) + .ok(onDatabase("functional", TPrivilegeLevel.ALTER)) + .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL)) + .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER)) + .error(alterError("functional.alltypes")) + .error(alterError("functional.alltypes"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) + .error(alterError("functional.alltypes"), onDatabase("functional", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) + .error(alterError("functional.alltypes"), onTable("functional", "alltypes", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))); - // Table does not exist. - authorize(String.format("%s stats functional.notbl", statsType)) - .error(alterError("functional.notbl")) - .error(alterError("functional.notbl"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) - .error(alterError("default.functional"), onDatabase("functional", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.ALL))); - } + // Drop stats on database that does not exist. + authorize("drop stats nodb.notbl") + .error(alterError("nodb.notbl")) + .error(alterError("nodb.notbl"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) + .error(alterError("nodb.notbl"), onDatabase("nodb", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))); + + // Drop stats on table that does not exist. + authorize("drop stats functional.notbl") + .error(alterError("functional.notbl")) + .error(alterError("functional.notbl"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))) + .error(alterError("functional.notbl"), onDatabase("functional", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER))); } @Test
