----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22847/#review46359 -----------------------------------------------------------
Firstly, thanks for taking the initiative to correct the privilege model and for all the work! Overall looks fine. A couple of suggestions high level comments - 1. The metastore plugin needs to be updated for this. Feel free to log a followup ticket and assign to me. I can build that patch on top of this change. 2. The hive binding hook is capturing the partition URI for DB scope operation. See https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java#L386 Now that add partition is table scope, we need to move that to table scope code block. A few more comments/questions on specific changes below. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java <https://reviews.apache.org/r/22847/#comment81694> Should the URI privilege be ALL ? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java <https://reviews.apache.org/r/22847/#comment81696> If this is removed, how are we handling insert ? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java <https://reviews.apache.org/r/22847/#comment81695> Does the URI need to be be removed ? You could still have 'create table .. location ..' which is covered by URI. Will it be able to handle that ? - Prasad Mujumdar On June 21, 2014, 7:17 a.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22847/ > ----------------------------------------------------------- > > (Updated June 21, 2014, 7:17 a.m.) > > > Review request for sentry, Arun Suresh, Jarek Cecho, Lenni Kuff, and Prasad > Mujumdar. > > > Bugs: SENTRY-310 > https://issues.apache.org/jira/browse/SENTRY-310 > > > Repository: sentry > > > Description > ------- > > Required privileges for a given hive operation is too restrictive in some > cases. This patch cleans that up. The new model is documented as a pdf > attached to the ticket. > > In short: > - All DDL statements on an object require ALL on that object, except the > create database/table/view/partition which requires all on the parent, as we > should not allow granting privileges on non existing objects. > - Cleaned up some unwanted uri privileges, now we only support all on URI. > - Fixed some more non intuitive mappings > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > 7d241d0ea7957e6b6c334c78c6bcf0934f1a36ab > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java > e725eb06fc9915b0bcc2609e428a62feea80ec43 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestOperations.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/22847/diff/ > > > Testing > ------- > > Captured most of the Hive operations in TestOperations test class. All of > them pass. > > Added todos for the operations which need test cases. Now running the entire > suite. > > > Thanks, > > Sravya Tirukkovalur > >
