IMPALA-7344: Restrict ALTER DATABASE/TABLE SET OWNER statements Prior to this patch, any user with ALTER privilege could alter the database/table ownership from one user/role to another user/role. This is undesirable because altering an object ownership means giving a full access to that object. This patch restricts the ALTER DATABASE/TABLE SET OWNER statements to require ALL/OWNER with GRANT OPTION when authorization is enabled.
Testing: - Added FE authorization tests - Ran all FE tests - Ran core tests Change-Id: I2485933c02b5384950b7c882ba1eb0fd703db5a3 Reviewed-on: http://gerrit.cloudera.org:8080/11279 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/649f175d Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/649f175d Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/649f175d Branch: refs/heads/master Commit: 649f175df08cf4708fe48ab5d4364d1634c16d22 Parents: 632e0e2 Author: Fredy Wijaya <[email protected]> Authored: Mon Aug 20 16:02:28 2018 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Wed Aug 22 21:27:01 2018 +0000 ---------------------------------------------------------------------- bin/impala-config.sh | 2 +- .../impala/analysis/AlterDbSetOwnerStmt.java | 5 +- .../analysis/AlterTableOrViewSetOwnerStmt.java | 4 +- .../org/apache/impala/analysis/Analyzer.java | 67 +++++++-- .../apache/impala/analysis/BaseTableRef.java | 3 +- .../impala/analysis/CollectionTableRef.java | 3 +- .../apache/impala/analysis/InlineViewRef.java | 4 +- .../org/apache/impala/analysis/TableRef.java | 15 +- .../authorization/AuthorizationChecker.java | 31 +++-- .../impala/authorization/PrivilegeRequest.java | 31 +++-- .../authorization/PrivilegeRequestBuilder.java | 11 +- .../impala/catalog/PrincipalPrivilege.java | 1 + .../apache/impala/util/SentryPolicyService.java | 2 +- .../impala/analysis/AnalyzeStmtsTest.java | 2 +- .../impala/analysis/AuthorizationStmtTest.java | 139 ++++++++++++++----- 15 files changed, 236 insertions(+), 84 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/bin/impala-config.sh ---------------------------------------------------------------------- diff --git a/bin/impala-config.sh b/bin/impala-config.sh index 656861d..facb7f4 100755 --- a/bin/impala-config.sh +++ b/bin/impala-config.sh @@ -162,7 +162,7 @@ unset IMPALA_KUDU_URL : ${CDH_DOWNLOAD_HOST:=native-toolchain.s3.amazonaws.com} export CDH_DOWNLOAD_HOST export CDH_MAJOR_VERSION=6 -export CDH_BUILD_NUMBER=533940 +export CDH_BUILD_NUMBER=537982 export IMPALA_HADOOP_VERSION=3.0.0-cdh6.x-SNAPSHOT export IMPALA_HBASE_VERSION=2.0.0-cdh6.x-SNAPSHOT export IMPALA_HIVE_VERSION=2.1.1-cdh6.x-SNAPSHOT http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java index 379a806..87c0edd 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java @@ -18,6 +18,7 @@ package org.apache.impala.analysis; import com.google.common.base.Preconditions; +import org.apache.impala.authorization.Privilege; import org.apache.impala.common.AnalysisException; import org.apache.impala.thrift.TAlterDbParams; import org.apache.impala.thrift.TAlterDbSetOwnerParams; @@ -38,7 +39,9 @@ public class AlterDbSetOwnerStmt extends AlterDbStmt { @Override public void analyze(Analyzer analyzer) throws AnalysisException { - super.analyze(analyzer); + // Require ALL with GRANT OPTION privilege. + analyzer.getDb(dbName_, Privilege.ALL, /* throw if does not exist */ true, + /* grant option */ true); String ownerName = owner_.getOwnerName(); if (ownerName.length() > MetaStoreUtil.MAX_OWNER_LENGTH) { throw new AnalysisException(String.format("Owner name exceeds maximum length of " + http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java index c508413..25470de 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java @@ -46,7 +46,9 @@ public abstract class AlterTableOrViewSetOwnerStmt extends AlterTableStmt { MetaStoreUtil.MAX_OWNER_LENGTH, ownerName.length())); } tableName_ = analyzer.getFqTableName(tableName_); - TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALTER); + // Require ALL with GRANT OPTION privilege. + TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALL, + /* grant option */ true); tableRef = analyzer.resolveTableRef(tableRef); Preconditions.checkNotNull(tableRef); tableRef.analyze(analyzer); http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/Analyzer.java ---------------------------------------------------------------------- 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 6468513..94375a4 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java +++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java @@ -569,13 +569,21 @@ public class Analyzer { // 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() + PrivilegeRequestBuilder builder = new PrivilegeRequestBuilder() .onTable(rawPath.get(0), rawPath.get(1)) - .allOf(tableRef.getPrivilege()).toRequest()); + .allOf(tableRef.getPrivilege()); + if (tableRef.requireGrantOption()) { + builder.grantOption(); + } + registerPrivReq(builder.toRequest()); } - registerPrivReq(new PrivilegeRequestBuilder() + PrivilegeRequestBuilder builder = new PrivilegeRequestBuilder() .onTable(getDefaultDb(), rawPath.get(0)) - .allOf(tableRef.getPrivilege()).toRequest()); + .allOf(tableRef.getPrivilege()); + if (tableRef.requireGrantOption()) { + builder.grantOption(); + } + registerPrivReq(builder.toRequest()); throw e; } catch (TableLoadingException e) { throw new AnalysisException(String.format( @@ -2448,21 +2456,39 @@ public class Analyzer { } /** - * Returns the Catalog Db object for the given database at the given - * Privilege level. The privilege request is tracked in the analyzer - * and authorized post-analysis. - * - * Registers a new access event if the catalog lookup was successful. - * * If the database does not exist in the catalog an AnalysisError is thrown. + * This method does not require the grant option permission. */ public FeDb getDb(String dbName, Privilege privilege) throws AnalysisException { return getDb(dbName, privilege, true); } + /** + * This method does not require the grant option permission. + */ public FeDb getDb(String dbName, Privilege privilege, boolean throwIfDoesNotExist) throws AnalysisException { + return getDb(dbName, privilege, throwIfDoesNotExist, false); + } + + /** + * Returns the Catalog Db object for the given database at the given + * Privilege level. The privilege request is tracked in the analyzer + * and authorized post-analysis. + * + * Registers a new access event if the catalog lookup was successful. + * + * If throwIfDoesNotExist is set to true and the database does not exist in the catalog + * an AnalysisError is thrown. + * If requireGrantOption is set to true, the grant option permission is required for + * the specified privilege. + */ + public FeDb getDb(String dbName, Privilege privilege, boolean throwIfDoesNotExist, + boolean requireGrantOption) throws AnalysisException { PrivilegeRequestBuilder pb = new PrivilegeRequestBuilder(); + if (requireGrantOption) { + pb.grantOption(); + } if (privilege == Privilege.ANY) { registerPrivReq( pb.any().onAnyColumn(dbName, AuthorizeableTable.ANY_TABLE_NAME).toRequest()); @@ -2593,11 +2619,20 @@ public class Analyzer { } /** + * This method does not require the grant option permission. + */ + public void registerAuthAndAuditEvent(FeTable table, Privilege priv) { + registerAuthAndAuditEvent(table, priv, false); + } + + /** * Registers a table-level privilege request and an access event for auditing * for the given table and privilege. The table must be a base table or a - * catalog view (not a local view). + * catalog view (not a local view). If requireGrantOption is set to true, the + * the grant option permission is required for the specified privilege. */ - public void registerAuthAndAuditEvent(FeTable table, Privilege priv) { + public void registerAuthAndAuditEvent(FeTable table, Privilege priv, + boolean requireGrantOption) { // Add access event for auditing. if (table instanceof FeView) { FeView view = (FeView) table; @@ -2612,8 +2647,12 @@ public class Analyzer { } // Add privilege request. TableName tableName = table.getTableName(); - registerPrivReq(new PrivilegeRequestBuilder() + PrivilegeRequestBuilder builder = new PrivilegeRequestBuilder() .onTable(tableName.getDb(), tableName.getTbl()) - .allOf(priv).toRequest()); + .allOf(priv); + if (requireGrantOption) { + builder.grantOption(); + } + registerPrivReq(builder.toRequest()); } } http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java b/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java index 5b80712..f6ecdb8 100644 --- a/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java +++ b/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java @@ -60,7 +60,8 @@ public class BaseTableRef extends TableRef { @Override public void analyze(Analyzer analyzer) throws AnalysisException { if (isAnalyzed_) return; - analyzer.registerAuthAndAuditEvent(resolvedPath_.getRootTable(), priv_); + analyzer.registerAuthAndAuditEvent(resolvedPath_.getRootTable(), priv_, + requireGrantOption_); desc_ = analyzer.registerTableRef(this); isAnalyzed_ = true; analyzeTableSample(analyzer); http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java b/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java index 92015f5..a6f66ab 100644 --- a/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java +++ b/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java @@ -103,7 +103,8 @@ public class CollectionTableRef extends TableRef { // 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(), priv_); + analyzer.registerAuthAndAuditEvent(resolvedPath_.getRootTable(), priv_, + requireGrantOption_); analyzer.registerPrivReq(new PrivilegeRequestBuilder(). allOf(Privilege.SELECT).onColumn(desc_.getTableName().getDb(), desc_.getTableName().getTbl(), desc_.getPath().getRawPath().get(0)) http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java b/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java index dbcb59a..494b4af 100644 --- a/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java +++ b/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java @@ -95,7 +95,7 @@ public class InlineViewRef extends TableRef { */ public InlineViewRef(FeView view, TableRef origTblRef) { super(view.getTableName().toPath(), origTblRef.getExplicitAlias(), - origTblRef.getPrivilege()); + origTblRef.getPrivilege(), origTblRef.requireGrantOption()); queryStmt_ = view.getQueryStmt().clone(); queryStmt_.reset(); if (view.isLocalView()) queryStmt_.reset(); @@ -145,7 +145,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_, priv_); + analyzer.registerAuthAndAuditEvent(view_, priv_, requireGrantOption_); 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 http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/analysis/TableRef.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/TableRef.java b/fe/src/main/java/org/apache/impala/analysis/TableRef.java index 8ccc9ab..0cd65ae 100644 --- a/fe/src/main/java/org/apache/impala/analysis/TableRef.java +++ b/fe/src/main/java/org/apache/impala/analysis/TableRef.java @@ -79,6 +79,7 @@ public class TableRef implements ParseNode { // Analysis registers privilege and/or audit requests based on this privilege. protected final Privilege priv_; + protected final boolean requireGrantOption_; // Optional TABLESAMPLE clause. Null if not specified. protected TableSampleClause sampleParams_; @@ -135,15 +136,20 @@ public class TableRef implements ParseNode { } public TableRef(List<String> path, String alias, TableSampleClause tableSample) { - this(path, alias, tableSample, Privilege.SELECT); + this(path, alias, tableSample, Privilege.SELECT, false); } public TableRef(List<String> path, String alias, Privilege priv) { - this(path, alias, null, priv); + this(path, alias, null, priv, false); + } + + public TableRef(List<String> path, String alias, Privilege priv, + boolean requireGrantOption) { + this(path, alias, null, priv, requireGrantOption); } public TableRef(List<String> path, String alias, TableSampleClause sampleParams, - Privilege priv) { + Privilege priv, boolean requireGrantOption) { rawPath_ = path; if (alias != null) { aliases_ = new String[] { alias.toLowerCase() }; @@ -153,6 +159,7 @@ public class TableRef implements ParseNode { } sampleParams_ = sampleParams; priv_ = priv; + requireGrantOption_ = requireGrantOption; isAnalyzed_ = false; replicaPreference_ = null; randomReplica_ = false; @@ -168,6 +175,7 @@ public class TableRef implements ParseNode { hasExplicitAlias_ = other.hasExplicitAlias_; sampleParams_ = other.sampleParams_; priv_ = other.priv_; + requireGrantOption_ = other.requireGrantOption_; joinOp_ = other.joinOp_; joinHints_ = Lists.newArrayList(other.joinHints_); onClause_ = (other.onClause_ != null) ? other.onClause_.clone() : null; @@ -274,6 +282,7 @@ public class TableRef implements ParseNode { } public TableSampleClause getSampleParams() { return sampleParams_; } public Privilege getPrivilege() { return priv_; } + public boolean requireGrantOption() { return requireGrantOption_; } public List<PlanHint> getJoinHints() { return joinHints_; } public List<PlanHint> getTableHints() { return tableHints_; } public Expr getOnClause() { return onClause_; } http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java b/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java index 82dcbc8..326ffa4 100644 --- a/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java +++ b/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java @@ -19,7 +19,6 @@ package org.apache.impala.authorization; import java.util.Collections; import java.util.EnumSet; -import java.util.Iterator; import java.util.List; import java.util.Set; @@ -104,20 +103,22 @@ public class AuthorizationChecker { Privilege privilege = privilegeRequest.getPrivilege(); if (privilegeRequest.getAuthorizeable() instanceof AuthorizeableFn) { throw new AuthorizationException(String.format( - "User '%s' does not have privileges to %s functions in: %s", - user.getName(), privilege, privilegeRequest.getName())); + "User '%s' does not have privileges%s to %s functions in: %s", + user.getName(), grantOption(privilegeRequest.hasGrantOption()), privilege, + privilegeRequest.getName())); } if (EnumSet.of(Privilege.ANY, Privilege.ALL, Privilege.VIEW_METADATA) .contains(privilege)) { throw new AuthorizationException(String.format( - "User '%s' does not have privileges to access: %s", - user.getName(), privilegeRequest.getName())); + "User '%s' does not have privileges%s to access: %s", + user.getName(), grantOption(privilegeRequest.hasGrantOption()), + privilegeRequest.getName())); } else if (privilege == Privilege.REFRESH) { throw new AuthorizationException(String.format( - "User '%s' does not have privileges to execute " + + "User '%s' does not have privileges%s to execute " + "'INVALIDATE METADATA/REFRESH' on: %s", user.getName(), - privilegeRequest.getName())); + grantOption(privilegeRequest.hasGrantOption()), privilegeRequest.getName())); } else if (privilege == Privilege.CREATE && privilegeRequest.getAuthorizeable() instanceof AuthorizeableTable) { // Creating a table requires CREATE on a database and we shouldn't @@ -125,16 +126,22 @@ public class AuthorizationChecker { AuthorizeableTable authorizeableTable = (AuthorizeableTable) privilegeRequest.getAuthorizeable(); throw new AuthorizationException(String.format( - "User '%s' does not have privileges to execute '%s' on: %s", - user.getName(), privilege, authorizeableTable.getDbName())); + "User '%s' does not have privileges%s to execute '%s' on: %s", + user.getName(), grantOption(privilegeRequest.hasGrantOption()), privilege, + authorizeableTable.getDbName())); } else { throw new AuthorizationException(String.format( - "User '%s' does not have privileges to execute '%s' on: %s", - user.getName(), privilege, privilegeRequest.getName())); + "User '%s' does not have privileges%s to execute '%s' on: %s", + user.getName(), grantOption(privilegeRequest.hasGrantOption()), privilege, + privilegeRequest.getName())); } } } + private static String grantOption(boolean hasGrantOption) { + return hasGrantOption ? " with 'GRANT OPTION'" : ""; + } + /* * Returns true if the given user has permission to execute the given * request, false otherwise. Always returns true if authorization is disabled. @@ -180,6 +187,6 @@ public class AuthorizationChecker { authorizeables.remove(authorizeables.size() - 1); } return provider_.hasAccess(new Subject(user.getShortName()), authorizeables, actions, - ActiveRoleSet.ALL); + request.hasGrantOption(), ActiveRoleSet.ALL); } } http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java b/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java index d206908..e6335d3 100644 --- a/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java +++ b/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java @@ -19,6 +19,8 @@ package org.apache.impala.authorization; import com.google.common.base.Preconditions; +import java.util.Objects; + /** * Represents a privilege request in the context of an Authorizeable object. If no * Authorizeable object is provided, it represents a privilege request on the server. @@ -27,18 +29,26 @@ import com.google.common.base.Preconditions; public class PrivilegeRequest { private final Authorizeable authorizeable_; private final Privilege privilege_; + private final boolean grantOption_; public PrivilegeRequest(Authorizeable authorizeable, Privilege privilege) { + this(authorizeable, privilege, false); + } + + public PrivilegeRequest(Authorizeable authorizeable, Privilege privilege, + boolean grantOption) { Preconditions.checkNotNull(authorizeable); Preconditions.checkNotNull(privilege); authorizeable_ = authorizeable; privilege_ = privilege; + grantOption_ = grantOption; } public PrivilegeRequest(Privilege privilege) { Preconditions.checkNotNull(privilege); authorizeable_ = null; privilege_ = privilege; + grantOption_ = false; } /* @@ -53,25 +63,28 @@ public class PrivilegeRequest { */ public Privilege getPrivilege() { return privilege_; } - /* * Returns Authorizeable object. Null if the request is for server-level permission. */ public Authorizeable getAuthorizeable() { return authorizeable_; } + /** + * Returns whether the grant option is required or not. + */ + public boolean hasGrantOption() { return grantOption_; } + @Override public int hashCode() { - return (authorizeable_ == null ? 0 : authorizeable_.hashCode()) * 37 + - privilege_.hashCode(); + return Objects.hash(authorizeable_, privilege_, grantOption_); } @Override public boolean equals(Object o) { - if (!(o instanceof PrivilegeRequest)) return false; - if (authorizeable_ == null) { - return ((PrivilegeRequest) o).getPrivilege().equals(privilege_); - } - return ((PrivilegeRequest) o).getAuthorizeable().equals(authorizeable_) && - ((PrivilegeRequest) o).getPrivilege().equals(privilege_); + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + PrivilegeRequest that = (PrivilegeRequest) o; + return grantOption_ == that.grantOption_ && + Objects.equals(authorizeable_, that.authorizeable_) && + privilege_ == that.privilege_; } } http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java b/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java index efe043d..6aa71e7 100644 --- a/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java +++ b/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java @@ -35,6 +35,7 @@ import com.google.common.base.Preconditions; public class PrivilegeRequestBuilder { Authorizeable authorizeable_; Privilege privilege_; + boolean grantOption_ = false; /** * Sets the authorizeable object to be a column. @@ -108,12 +109,20 @@ public class PrivilegeRequestBuilder { } /** + * Specifies that grant option is required. + */ + public PrivilegeRequestBuilder grantOption() { + grantOption_ = true; + return this; + } + + /** * Builds a PrivilegeRequest object based on the current Authorizeable object * and privilege settings. */ public PrivilegeRequest toRequest() { Preconditions.checkNotNull(authorizeable_); Preconditions.checkNotNull(privilege_); - return new PrivilegeRequest(authorizeable_, privilege_); + return new PrivilegeRequest(authorizeable_, privilege_, grantOption_); } } http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java index 033b9c9..bb7500b 100644 --- a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java +++ b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java @@ -116,6 +116,7 @@ public class PrincipalPrivilege extends CatalogObjectImpl { authorizable.add(KV_JOINER.join("action", privilege.getPrivilege_level().toString())); } + authorizable.add(KV_JOINER.join("grantoption", privilege.isHas_grant_opt())); return AUTHORIZABLE_JOINER.join(authorizable); } catch (Exception e) { // Should never make it here unless the privilege is malformed. http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java b/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java index 5d3ee33..3a5c11c 100644 --- a/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java +++ b/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java @@ -493,7 +493,6 @@ public class SentryPolicyService { privilege.setPrivilege_level(Enum.valueOf(TPrivilegeLevel.class, sentryPriv.getAction().toUpperCase())); } - privilege.setPrivilege_name(PrincipalPrivilege.buildPrivilegeName(privilege)); privilege.setCreate_time_ms(sentryPriv.getCreateTime()); if (sentryPriv.isSetGrantOption() && sentryPriv.getGrantOption() == TSentryGrantOption.TRUE) { @@ -501,6 +500,7 @@ public class SentryPolicyService { } else { privilege.setHas_grant_opt(false); } + privilege.setPrivilege_name(PrincipalPrivilege.buildPrivilegeName(privilege)); privilege.setPrincipal_id(principal.getId()); privilege.setPrincipal_type(principal.getPrincipalType()); return privilege; http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java index 5249826..520b88f 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java @@ -3782,7 +3782,7 @@ public class AnalyzeStmtsTest extends AnalyzerTest { testNumberOfMembers(ValuesStmt.class, 0); // Also check TableRefs. - testNumberOfMembers(TableRef.class, 20); + testNumberOfMembers(TableRef.class, 21); testNumberOfMembers(BaseTableRef.class, 0); testNumberOfMembers(InlineViewRef.class, 8); } http://git-wip-us.apache.org/repos/asf/impala/blob/649f175d/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 8da44c1..1edb066 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java @@ -1958,15 +1958,15 @@ public class AuthorizationStmtTest extends FrontendTestBase { "delimited fields terminated by ' '"), authorize("alter table functional.alltypes add partition(year=1, month=1)"), authorize("alter table functional.alltypes drop partition(" + - "year=2009, month=1)"), - authorize("alter table functional.alltypes set owner user foo_owner"), - authorize("alter table functional.alltypes set owner role foo_owner")}) { + "year=2009, month=1)")}) { test.ok(onServer(TPrivilegeLevel.ALL)) .ok(onServer(TPrivilegeLevel.OWNER)) .ok(onServer(TPrivilegeLevel.ALTER)) .ok(onDatabase("functional", TPrivilegeLevel.ALL)) .ok(onDatabase("functional", TPrivilegeLevel.OWNER)) .ok(onDatabase("functional", TPrivilegeLevel.ALTER)) + .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL)) + .ok(onTable("functional", "alltypes", TPrivilegeLevel.OWNER)) .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER)) .error(alterError("functional.alltypes")) .error(alterError("functional.alltypes"), onServer(allExcept( @@ -1978,6 +1978,32 @@ public class AuthorizationStmtTest extends FrontendTestBase { TPrivilegeLevel.ALTER))); } + // Alter table set owner. + for (AuthzTest test: new AuthzTest[]{ + authorize("alter table functional.alltypes set owner user foo_owner"), + authorize("alter table functional.alltypes set owner role foo_owner")}) { + test.ok(onServer(true, TPrivilegeLevel.ALL)) + .ok(onServer(true, TPrivilegeLevel.OWNER)) + .ok(onDatabase(true, "functional", TPrivilegeLevel.ALL)) + .ok(onDatabase(true, "functional", TPrivilegeLevel.OWNER)) + .ok(onTable(true, "functional", "alltypes", TPrivilegeLevel.ALL)) + .ok(onTable(true, "functional", "alltypes", TPrivilegeLevel.OWNER)) + .error(accessError(true, "functional.alltypes"), onServer( + TPrivilegeLevel.values())) + .error(accessError(true, "functional.alltypes"), onDatabase("functional", + TPrivilegeLevel.values())) + .error(accessError(true, "functional.alltypes"), onTable("functional", + "alltypes", TPrivilegeLevel.values())) + .error(accessError(true, "functional.alltypes")) + .error(accessError(true, "functional.alltypes"), onServer(true, allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(accessError(true, "functional.alltypes"), onDatabase(true, "functional", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(accessError(true, "functional.alltypes"), + onTable(true, "functional", "alltypes", allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))); + } + // Alter table rename. authorize("alter table functional.alltypes rename to functional.new_table") .ok(onServer(TPrivilegeLevel.ALL)) @@ -2178,24 +2204,25 @@ public class AuthorizationStmtTest extends FrontendTestBase { for (AuthzTest test: new AuthzTest[]{ authorize("alter view functional.alltypes_view set owner user foo_owner"), authorize("alter view functional.alltypes_view set owner role foo_owner")}) { - test.ok(onServer(TPrivilegeLevel.ALL)) - .ok(onServer(TPrivilegeLevel.OWNER)) - .ok(onServer(TPrivilegeLevel.ALTER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALL)) - .ok(onDatabase("functional", TPrivilegeLevel.OWNER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALTER)) - .ok(onTable("functional", "alltypes_view", TPrivilegeLevel.ALL)) - .ok(onTable("functional", "alltypes_view", TPrivilegeLevel.OWNER)) - .ok(onTable("functional", "alltypes_view", TPrivilegeLevel.ALTER)) - .error(alterError("functional.alltypes_view")) - .error(alterError("functional.alltypes_view"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER))) - .error(alterError("functional.alltypes_view"), onDatabase("functional", - allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, - TPrivilegeLevel.ALTER))) - .error(alterError("functional.alltypes_view"), onTable("functional", - "alltypes_view", allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, - TPrivilegeLevel.ALTER))); + test.ok(onServer(true, TPrivilegeLevel.ALL)) + .ok(onServer(true, TPrivilegeLevel.OWNER)) + .ok(onDatabase(true, "functional", TPrivilegeLevel.ALL)) + .ok(onDatabase(true, "functional", TPrivilegeLevel.OWNER)) + .ok(onTable(true, "functional", "alltypes_view", TPrivilegeLevel.ALL)) + .ok(onTable(true, "functional", "alltypes_view", TPrivilegeLevel.OWNER)) + .error(accessError(true, "functional.alltypes_view"), onServer( + TPrivilegeLevel.values())) + .error(accessError(true, "functional.alltypes_view"), onDatabase("functional", + TPrivilegeLevel.values())) + .error(accessError(true, "functional.alltypes_view"), onTable("functional", + "alltypes_view", TPrivilegeLevel.values())) + .error(accessError(true, "functional.alltypes_view")) + .error(accessError(true, "functional.alltypes_view"), onServer(allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(accessError(true, "functional.alltypes_view"), onDatabase("functional", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(accessError(true, "functional.alltypes_view"), onTable("functional", + "alltypes_view", allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))); } // Database does not exist. @@ -2215,25 +2242,27 @@ public class AuthorizationStmtTest extends FrontendTestBase { @Test public void testAlterDatabase() throws ImpalaException { + // Alter database set owner. for (String ownerType: new String[]{"user", "role"}) { authorize(String.format("alter database functional set owner %s foo", ownerType)) - .ok(onServer(TPrivilegeLevel.ALL)) - .ok(onServer(TPrivilegeLevel.OWNER)) - .ok(onServer(TPrivilegeLevel.ALTER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALL)) - .ok(onDatabase("functional", TPrivilegeLevel.OWNER)) - .ok(onDatabase("functional", TPrivilegeLevel.ALTER)) - .error(alterError("functional")) - .error(alterError("functional"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER))) - .error(alterError("functional"), onDatabase("functional", allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER))); + .ok(onServer(true, TPrivilegeLevel.ALL)) + .ok(onServer(true, TPrivilegeLevel.OWNER)) + .ok(onDatabase(true, "functional", TPrivilegeLevel.ALL)) + .ok(onDatabase(true, "functional", TPrivilegeLevel.OWNER)) + .error(accessError(true, "functional"), onServer(TPrivilegeLevel.values())) + .error(accessError(true, "functional"), onDatabase("functional", + TPrivilegeLevel.values())) + .error(accessError(true, "functional")) + .error(accessError(true, "functional"), onServer(true, allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) + .error(accessError(true, "functional"), onDatabase(true, "functional", + allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))); // Database does not exist. authorize(String.format("alter database nodb set owner %s foo", ownerType)) - .error(alterError("nodb")) - .error(alterError("nodb"), onServer(allExcept( - TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, TPrivilegeLevel.ALTER))); + .error(accessError(true, "nodb")) + .error(accessError(true, "nodb"), onServer(true, allExcept( + TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))); } } @@ -2580,7 +2609,12 @@ public class AuthorizationStmtTest extends FrontendTestBase { } private static String accessError(String object) { - return "User '%s' does not have privileges to access: " + object; + return accessError(false, object); + } + + private static String accessError(boolean grantOption, String object) { + return "User '%s' does not have privileges" + + (grantOption ? " with 'GRANT OPTION'" : "") + " to access: " + object; } private static String refreshError(String object) { @@ -2880,10 +2914,15 @@ public class AuthorizationStmtTest extends FrontendTestBase { } private TPrivilege[] onServer(TPrivilegeLevel... levels) { + return onServer(false, levels); + } + + private TPrivilege[] onServer(boolean grantOption, TPrivilegeLevel... levels) { TPrivilege[] privileges = new TPrivilege[levels.length]; for (int i = 0; i < levels.length; i++) { privileges[i] = new TPrivilege("", levels[i], TPrivilegeScope.SERVER, false); privileges[i].setServer_name(SENTRY_SERVER); + privileges[i].setHas_grant_opt(grantOption); privileges[i].setPrivilege_name(PrincipalPrivilege.buildPrivilegeName( privileges[i])); } @@ -2891,11 +2930,17 @@ public class AuthorizationStmtTest extends FrontendTestBase { } private TPrivilege[] onDatabase(String db, TPrivilegeLevel... levels) { + return onDatabase(false, db, levels); + } + + private TPrivilege[] onDatabase(boolean grantOption, String db, + TPrivilegeLevel... levels) { TPrivilege[] privileges = new TPrivilege[levels.length]; for (int i = 0; i < levels.length; i++) { privileges[i] = new TPrivilege("", levels[i], TPrivilegeScope.DATABASE, false); privileges[i].setServer_name(SENTRY_SERVER); privileges[i].setDb_name(db); + privileges[i].setHas_grant_opt(grantOption); privileges[i].setPrivilege_name(PrincipalPrivilege.buildPrivilegeName( privileges[i])); } @@ -2903,12 +2948,18 @@ public class AuthorizationStmtTest extends FrontendTestBase { } private TPrivilege[] onTable(String db, String table, TPrivilegeLevel... levels) { + return onTable(false, db, table, levels); + } + + private TPrivilege[] onTable(boolean grantOption, String db, String table, + TPrivilegeLevel... levels) { TPrivilege[] privileges = new TPrivilege[levels.length]; for (int i = 0; i < levels.length; i++) { privileges[i] = new TPrivilege("", levels[i], TPrivilegeScope.TABLE, false); privileges[i].setServer_name(SENTRY_SERVER); privileges[i].setDb_name(db); privileges[i].setTable_name(table); + privileges[i].setHas_grant_opt(grantOption); privileges[i].setPrivilege_name(PrincipalPrivilege.buildPrivilegeName( privileges[i])); } @@ -2920,8 +2971,18 @@ public class AuthorizationStmtTest extends FrontendTestBase { return onColumn(db, table, new String[]{column}, levels); } + private TPrivilege[] onColumn(boolean grantOption, String db, String table, + String column, TPrivilegeLevel... levels) { + return onColumn(grantOption, db, table, new String[]{column}, levels); + } + private TPrivilege[] onColumn(String db, String table, String[] columns, TPrivilegeLevel... levels) { + return onColumn(false, db, table, columns, levels); + } + + private TPrivilege[] onColumn(boolean grantOption, String db, String table, + String[] columns, TPrivilegeLevel... levels) { int size = columns.length * levels.length; TPrivilege[] privileges = new TPrivilege[size]; int idx = 0; @@ -2932,6 +2993,7 @@ public class AuthorizationStmtTest extends FrontendTestBase { privileges[idx].setDb_name(db); privileges[idx].setTable_name(table); privileges[idx].setColumn_name(column); + privileges[idx].setHas_grant_opt(grantOption); privileges[idx].setPrivilege_name(PrincipalPrivilege.buildPrivilegeName( privileges[idx])); idx++; @@ -2941,11 +3003,16 @@ public class AuthorizationStmtTest extends FrontendTestBase { } private TPrivilege[] onUri(String uri, TPrivilegeLevel... levels) { + return onUri(false, uri, levels); + } + + private TPrivilege[] onUri(boolean grantOption, String uri, TPrivilegeLevel... levels) { TPrivilege[] privileges = new TPrivilege[levels.length]; for (int i = 0; i < levels.length; i++) { privileges[i] = new TPrivilege("", levels[i], TPrivilegeScope.URI, false); privileges[i].setServer_name(SENTRY_SERVER); privileges[i].setUri(uri); + privileges[i].setHas_grant_opt(grantOption); privileges[i].setPrivilege_name(PrincipalPrivilege.buildPrivilegeName( privileges[i])); }
