IMPALA-7502: ALTER TABLE RENAME should require ALL on the old table Prior to this patch, ALTER TABLE/VIEW RENAME required ALTER on the old table. This may pose a potential security risk, such as having ALTER on a table and ALL on a particular database allows a user to move the table to a database with ALL, which will automatically grant that user with ALL privilege on that table due to the privilege inherited from the database. This patch fixes the issue by requring ALL on the old table. What this means is moving a table to a database with ALL privilege will not elevate the privilege since ALL is now required for a table to be renamed.
Testing: - Ran all FE tests Change-Id: I47a417a77df3f3030cf5f54fd2280b5e5e1fb77a Reviewed-on: http://gerrit.cloudera.org:8080/11344 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/b97e5ba3 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/b97e5ba3 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/b97e5ba3 Branch: refs/heads/master Commit: b97e5ba38dcb8128efc18dbc995307386b8d2210 Parents: e8210ab Author: Fredy Wijaya <[email protected]> Authored: Tue Aug 28 14:07:16 2018 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Aug 30 01:11:08 2018 +0000 ---------------------------------------------------------------------- .../analysis/AlterTableOrViewRenameStmt.java | 2 +- .../apache/impala/analysis/AuditingTest.java | 12 ++- .../impala/analysis/AuthorizationStmtTest.java | 86 +++++++++++--------- 3 files changed, 57 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/b97e5ba3/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java index 4cf2ff3..1ba4ae5 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java @@ -70,7 +70,7 @@ public class AlterTableOrViewRenameStmt extends AlterTableStmt { @Override public void analyze(Analyzer analyzer) throws AnalysisException { newTableName_.analyze(); - table_ = analyzer.getTable(tableName_, Privilege.ALTER); + table_ = analyzer.getTable(tableName_, Privilege.ALL); if (table_ instanceof FeView && renameTable_) { throw new AnalysisException(String.format( "ALTER TABLE not allowed on a view: %s", table_.getFullName())); http://git-wip-us.apache.org/repos/asf/impala/blob/b97e5ba3/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java ---------------------------------------------------------------------- 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 7735d3f..944fadb 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java @@ -259,18 +259,26 @@ public class AuditingTest extends FrontendTestBase { "ALTER TABLE functional_seq_snap.alltypes RENAME TO functional_seq_snap.t1"); Assert.assertEquals(accessEvents, Sets.newHashSet( new TAccessEvent( - "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "ALTER"), + "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "ALL"), new TAccessEvent("functional_seq_snap.t1", TCatalogObjectType.TABLE, "CREATE"))); } @Test public void TestAlterView() throws AnalysisException, AuthorizationException { Set<TAccessEvent> accessEvents = AnalyzeAccessEvents( + "ALTER VIEW functional_seq_snap.alltypes_view AS " + + "SELECT * FROM functional.alltypes"); + Assert.assertEquals(accessEvents, Sets.newHashSet( + new TAccessEvent( + "functional_seq_snap.alltypes_view", TCatalogObjectType.VIEW, "ALTER"), + new TAccessEvent("functional.alltypes", TCatalogObjectType.TABLE, "SELECT"))); + + accessEvents = AnalyzeAccessEvents( "ALTER VIEW functional_seq_snap.alltypes_view " + "rename to functional_seq_snap.v1"); Assert.assertEquals(accessEvents, Sets.newHashSet( new TAccessEvent( - "functional_seq_snap.alltypes_view", TCatalogObjectType.VIEW, "ALTER"), + "functional_seq_snap.alltypes_view", TCatalogObjectType.VIEW, "ALL"), new TAccessEvent("functional_seq_snap.v1", TCatalogObjectType.VIEW, "CREATE"))); } http://git-wip-us.apache.org/repos/asf/impala/blob/b97e5ba3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java ---------------------------------------------------------------------- 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 1edb066..6d301d9 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java @@ -2005,29 +2005,32 @@ public class AuthorizationStmtTest extends FrontendTestBase { } // Alter table rename. - authorize("alter table functional.alltypes rename to functional.new_table") + authorize("alter table functional.alltypes rename to functional_parquet.new_table") .ok(onServer(TPrivilegeLevel.ALL)) .ok(onServer(TPrivilegeLevel.OWNER)) - .ok(onServer(TPrivilegeLevel.ALTER, TPrivilegeLevel.CREATE)) - .ok(onDatabase("functional", TPrivilegeLevel.ALL)) - .ok(onDatabase("functional", TPrivilegeLevel.OWNER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALTER, TPrivilegeLevel.CREATE)) - .ok(onDatabase("functional", TPrivilegeLevel.CREATE), onTable("functional", - "alltypes", TPrivilegeLevel.ALTER)) - .error(alterError("functional.alltypes")) - .error(alterError("functional.alltypes"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER, - TPrivilegeLevel.CREATE))) - .error(alterError("functional.alltypes"), onDatabase("functional", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER, - TPrivilegeLevel.CREATE))) - .error(alterError("functional.alltypes"), onDatabase("functional", + .ok(onDatabase("functional", TPrivilegeLevel.ALL), + onDatabase("functional_parquet", TPrivilegeLevel.CREATE)) + .ok(onDatabase("functional_parquet", TPrivilegeLevel.CREATE), + onTable("functional", "alltypes", TPrivilegeLevel.ALL)) + .ok(onDatabase("functional", TPrivilegeLevel.OWNER), + onDatabase("functional_parquet", TPrivilegeLevel.CREATE)) + .ok(onDatabase("functional_parquet", TPrivilegeLevel.CREATE), + onTable("functional", "alltypes", TPrivilegeLevel.OWNER)) + .error(accessError("functional.alltypes")) + .error(accessError("functional.alltypes"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE))) + .error(accessError("functional.alltypes"), onDatabase("functional", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER)), onDatabase("functional_parquet", + TPrivilegeLevel.CREATE)) + .error(createError("functional_parquet"), onDatabase("functional", + TPrivilegeLevel.ALL), onDatabase("functional_parquet", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE))) + .error(accessError("functional.alltypes"), onDatabase("functional", TPrivilegeLevel.CREATE), onTable("functional", "alltypes", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER))) - .error(createError("functional"), onDatabase("functional", - allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, - TPrivilegeLevel.CREATE)), - onTable("functional", "alltypes", TPrivilegeLevel.ALTER)); + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(createError("functional_parquet"), onDatabase("functional", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE)), + onTable("functional", "alltypes", TPrivilegeLevel.ALL)); // Only for Kudu tables. for (AuthzTest test: new AuthzTest[]{ @@ -2156,29 +2159,32 @@ public class AuthorizationStmtTest extends FrontendTestBase { } // Alter view rename. - authorize("alter view functional.alltypes_view rename to functional.new_view") + authorize("alter view functional.alltypes_view rename to functional_parquet.new_view") .ok(onServer(TPrivilegeLevel.ALL)) .ok(onServer(TPrivilegeLevel.OWNER)) - .ok(onServer(TPrivilegeLevel.ALTER, TPrivilegeLevel.CREATE)) - .ok(onDatabase("functional", TPrivilegeLevel.ALL)) - .ok(onDatabase("functional", TPrivilegeLevel.OWNER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALTER, TPrivilegeLevel.CREATE)) - .ok(onDatabase("functional", TPrivilegeLevel.CREATE), onTable("functional", - "alltypes_view", TPrivilegeLevel.ALTER)) - .error(alterError("functional.alltypes_view")) - .error(alterError("functional.alltypes_view"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER, - TPrivilegeLevel.CREATE))) - .error(alterError("functional.alltypes_view"), onDatabase("functional", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER, - TPrivilegeLevel.CREATE))) - .error(alterError("functional.alltypes_view"), onDatabase("functional", + .ok(onDatabase("functional", TPrivilegeLevel.ALL), + onDatabase("functional_parquet", TPrivilegeLevel.CREATE)) + .ok(onDatabase("functional_parquet", TPrivilegeLevel.CREATE), + onTable("functional", "alltypes_view", TPrivilegeLevel.ALL)) + .ok(onDatabase("functional", TPrivilegeLevel.OWNER), + onDatabase("functional_parquet", TPrivilegeLevel.CREATE)) + .ok(onDatabase("functional_parquet", TPrivilegeLevel.CREATE), + onTable("functional", "alltypes_view", TPrivilegeLevel.OWNER)) + .error(accessError("functional.alltypes_view")) + .error(accessError("functional.alltypes_view"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE))) + .error(accessError("functional.alltypes_view"), onDatabase("functional", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER)), + onDatabase("functional_parquet", TPrivilegeLevel.CREATE)) + .error(createError("functional_parquet"), onDatabase("functional", + TPrivilegeLevel.ALL), onDatabase("functional_parquet", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE))) + .error(accessError("functional.alltypes_view"), onDatabase("functional", TPrivilegeLevel.CREATE), onTable("functional", "alltypes_view", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER))) - .error(createError("functional"), onDatabase("functional", - allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, - TPrivilegeLevel.CREATE)), onTable("functional", "alltypes_view", - TPrivilegeLevel.ALTER)); + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(createError("functional_parquet"), onDatabase("functional", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE)), + onTable("functional", "alltypes_view", TPrivilegeLevel.ALL)); // Alter view with constant select. authorize("alter view functional.alltypes_view as select 1")
