> On Sept. 5, 2014, 9:41 p.m., Prasad Mujumdar wrote: > > Looks fine to me. A couple of minor comments below, mostly to verify things. > > I think we should add some more test cases, especially the negative tests > > to verify that fine grain DDLs don't give you access to data. Eg Create or > > Alter privileges on table shouldn't work for SELECT and INSERT statements. > > Please feel free to log followup tickets for testing. > > Sravya Tirukkovalur wrote: > There are some existing negative tests cases for operations which require > select/insert. But these negative test cases mostly use older privileges (not > create,alter etc). So thats right that it would be good to get some coverage > there. Although, I think as these privileges are siblings to select/insert > (unlike all), one negative test case using one of these siblings should be > good enough, as we have added the heirarchy tests in TestDBWildcardPrivilege? > Just a thought? :-)
That should be fine. > On Sept. 5, 2014, 9:41 p.m., Prasad Mujumdar wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java, > > lines 214-216 > > <https://reviews.apache.org/r/25197/diff/4/?file=673113#file673113line214> > > > > Can we remove these todos ? > > Sravya Tirukkovalur wrote: > Sure, I just left them there to get attention of reviewers to make sure > the mappings are right :-) I will remove them if you think they are fine. that worked for sure :) yes, those look fine to me. - Prasad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25197/#review52502 ----------------------------------------------------------- On Aug. 31, 2014, 7:06 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25197/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2014, 7:06 p.m.) > > > Review request for sentry, Lenni Kuff and Prasad Mujumdar. > > > Bugs: sentry-331 > https://issues.apache.org/jira/browse/sentry-331 > > > Repository: sentry > > > Description > ------- > > Adding more granular privileges to the DBModel: Create, Drop, Alter, Index > and Lock. > Following broad rules apply: > - Creating a data object requires "create" privilege on the parent. That is, > create table requires create on db, create db requires create on server. > - Dropping a data object requires "drop" privilege on that object. > - All alter commands require "alter" privilege on that table with the > following exceptions: > -- Alter table drop also requires "drop" privilege on the table in addition > to "alter". > -- Alter table index rebuild only requires "index" privilege on the table > -- Alter table rename also requires "create" on db. > - Locking table requires "lock" on table. > > This patch also fixes SENTRY-413 and SENTRY-414 > > Note: I put comment "//TODO: Make sure" in the places where a second opinion > will help to make sure we are enforcing the right privileges for those > commands. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java > 49922f9f764579453a23bc4ccea9ce09f5fbcebd > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 0b268068b71b3602b95ddde9c87b21f4bfdc5754 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 2df741cab38aa6e7677d13da9677f8ea398149c6 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > 9498a2806a997c441bf86bcc4b0f229d46cb1777 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > 1c32946980d0b96edfb55a36e1b718d4248437ac > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java > 4e89f68dccbf92ab26d4150e929d363f2f098f27 > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAction.java > a4f3a87862312794fe724f1cffc8926de8b3deba > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestDBWildcardPrivilege.java > f4862e0a072c43b46f5470b988c8bc9419eb1fe1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > d4c58061779a551dda27bcb1dd505f5fb42045ba > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestOperations.java > 30cbb0db13d190e4ec89bcd777021c2eaa756c40 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/SentryPolicyProviderForDb.java > c60d0d54cec4780c8a8c134885c7eef3c388f34f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java > 55ae2f4aab3ea56050363d184eb435b605f3b4c5 > > Diff: https://reviews.apache.org/r/25197/diff/ > > > Testing > ------- > > Added test cases to the new updated privileges. > > > Thanks, > > Sravya Tirukkovalur > >
