> On May 10, 2018, 2:17 a.m., Sergio Pena wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > > Lines 123 (patched) > > <https://reviews.apache.org/r/67046/diff/1/?file=2019242#file2019242line123> > > > > Are roles with INSERT privileges allowed to add partitions in a table? > > > > I think that in order to add new partitions on a table, the ALTER > > privilege is required; and to remove partitions the DROP privilege is > > required. Both privileges are supported in Sentry. Can you confirm which > > privilege is required? > > Na Li wrote: > I debug into the code, the following is what's filled by hive > > // input required privilege: > > // 1) select, scope: user, db, table, column 2) delete, scope: > user, db, table > > // output required privilege: > > // 1) insert, scope: user, db, table > > Sergio Pena wrote: > Which code did you debug? > > Sentry has this privilege for adding partitions: > > HiveAuthzPrivileges addPartitionPrivilege = new > HiveAuthzPrivileges.AuthzPrivilegeBuilder(). > addOutputObjectPriviledge(AuthorizableType.Table, > EnumSet.of(DBModelAction.ALTER)). > //TODO: Uncomment this if we want to make it more restrictive > //addInputObjectPriviledge(AuthorizableType.Table, > EnumSet.of(DBModelAction.CREATE)). > addInputObjectPriviledge(AuthorizableType.URI, > EnumSet.of(DBModelAction.SELECT)).//TODO: make it optional > addOutputObjectPriviledge(AuthorizableType.URI, > EnumSet.of(DBModelAction.ALL)). > setOperationScope(HiveOperationScope.TABLE). > setOperationType(HiveOperationType.DDL). > build(); > > And this for dropping partitions: > > HiveAuthzPrivileges dropPartitionPrivilege = new > HiveAuthzPrivileges.AuthzPrivilegeBuilder(). > addInputObjectPriviledge(AuthorizableType.Table, > EnumSet.of(DBModelAction.ALTER)). > addInputObjectPriviledge(AuthorizableType.Table, > EnumSet.of(DBModelAction.DROP)). > setOperationScope(HiveOperationScope.TABLE). > setOperationType(HiveOperationType.DDL). > build(); > > Isn't exchanging partitions the same as adding a partition in the dest > table and dropping a partition in the source table? > > Na Li wrote: > 1) That is why you think, but not what hive does. > > Can you take a look at input required privileges and output required > privileges in stmtOperation for command "ALTER TABLE EXCHANGE" in following > code of HiveAuthzBindingHook class. These are the privileges user must have > in order to execute the command. > > @Override > public void postAnalyze(HiveSemanticAnalyzerHookContext context, > List<Task<? extends Serializable>> rootTasks) throws > SemanticException { > HiveOperation stmtOperation = context.getHiveOperation(); > HiveAuthzPrivileges stmtAuthObject; > > stmtAuthObject = > HiveAuthzPrivilegesMap.getHiveAuthzPrivileges(stmtOperation); <-check > stmtOperation > > > 2) Hive actions > > public enum PrivilegeType { > > ALL(HiveParser.TOK_PRIV_ALL, "All"), > ALTER_DATA(HiveParser.TOK_PRIV_ALTER_DATA, "Update"), > ALTER_METADATA(HiveParser.TOK_PRIV_ALTER_METADATA, "Alter"), > CREATE(HiveParser.TOK_PRIV_CREATE, "Create"), > DROP(HiveParser.TOK_PRIV_DROP, "Drop"), > INDEX(HiveParser.TOK_PRIV_INDEX, "Index"), > LOCK(HiveParser.TOK_PRIV_LOCK, "Lock"), > SELECT(HiveParser.TOK_PRIV_SELECT, "Select"), > SHOW_DATABASE(HiveParser.TOK_PRIV_SHOW_DATABASE, "Show_Database"), > INSERT(HiveParser.TOK_PRIV_INSERT, "Insert"), > DELETE(HiveParser.TOK_PRIV_DELETE, "Delete"), > UNKNOWN(null, null); > > 3) Sentry supported actions > > public enum DBModelAction implements Action { > > // SENTRY-1292 > // Need to ensure the order of enum to have SELECT in front to avoid > performance > // regression. Since most real use case of permissions may be read > only(SELECT). > SELECT(AccessConstants.SELECT), > INSERT(AccessConstants.INSERT), > ALTER(AccessConstants.ALTER), > CREATE(AccessConstants.CREATE), > DROP(AccessConstants.DROP), > INDEX(AccessConstants.INDEX), > LOCK(AccessConstants.LOCK), > ALL(AccessConstants.ALL); > > 4) As you can see Hive has DELECT, but sentry does not have that. DELECT > is different from DROP. IN "ALTER TABLE EXCHANGE", the partition is not > dropped. It is deleted from source table and inserted into destination table. > 5) As shown in the added unit tests, you can see once granting user the > corresponding privileges, user can execute the command. Basically, the > privileges requested by hive have to be granted to user. > > Sergio Pena wrote: > Hive has a DELETE command which requires DELETE privileges. Why do we > need DELETE for an ALTER operation? That's the legacy authorization of Hive > which it does not necessary match to Sentry. Btw, I think both tables (source > and destination) require ALTER privileges instead of CREATE/DROP or > INSERT/DELETE because this exchange happens with an ALTER command, which btw, > I think we would need to fix the ALTER ADDPART and ALTER DROPPART to reflect > the privilege if we do this (in another patch of course). > > Regarding the partition is not dropped comment, I tested it and the > partition is dropped from the source table, and added in the destination > table. But if we do allow the exchange with the ALTER privilege in both > source and destination, then that would fix the privileges, right?
I have changed the code. Now, the privilege required for source table is "ALL", and privileges required for destination table is "INSERT, ALTER". Is that OK for you? - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67046/#review202818 ----------------------------------------------------------- On May 10, 2018, 7:18 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67046/ > ----------------------------------------------------------- > > (Updated May 10, 2018, 7:18 p.m.) > > > Review request for sentry. > > > Bugs: sentry-2226 > https://issues.apache.org/jira/browse/sentry-2226 > > > Repository: sentry > > > Description > ------- > > add support for "ALTER TABLE EXCHANGE" > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > ffa193f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbColumnLevelMetaDataOps.java > 3735179 > > > Diff: https://reviews.apache.org/r/67046/diff/2/ > > > Testing > ------- > > unit test for "ALTER TABLE EXCHANGE" succeeded > > > Thanks, > > Na Li > >
