> On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > >
Thanks for the review Lenni! As this patch has already been committed, I am filing a follow on SENTRY-442 to make these minor suggestions. > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java, > > line 25 > > <https://reviews.apache.org/r/25197/diff/5/?file=681363#file681363line25> > > > > Curious what the point of ALLOWED_PRIVS? It seems like it is just a set > > of all the values in the PrivilegeType enum. Yes that is true, we should be able to just do EnumSet.allOf(PrivilegeType.class). > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java, > > line 578 > > <https://reviews.apache.org/r/25197/diff/5/?file=681364#file681364line578> > > > > Might be more naturally expressed with a case statement. Agree. > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java, > > line 593 > > <https://reviews.apache.org/r/25197/diff/5/?file=681364#file681364line593> > > > > Maybe: > > "Unknown privilege type: " + privilegeType". > > > > Also, should this be a SentryUserException? It seems more like an > > UnsupportedOperationException or IllegalStateException Changed the comment as per suggestion. But changing the Exception type requires changes in the calling public function. Also this exception is only for development purposes. It should never be exposed to the user. > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java, > > line 48 > > <https://reviews.apache.org/r/25197/diff/5/?file=681366#file681366line48> > > > > nit: rename to createTablePrivilege to be consistent with > > createDb/createServer naming. This is purposely named differently: All <action><object>Privilege are privileges on that action on that object(dropDb =>db->drop). Where as tableCreate is a "create table" specific privilege, requires "db->create" rather than "table->create" > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java, > > line 103 > > <https://reviews.apache.org/r/25197/diff/5/?file=681366#file681366line103> > > > > Oracle (11g) only requires the ALTER privilege to perform a rename. On > > the other hand mysql requires the "drop + alter" on the old table and > > "create + alter" on the new table (note that neither of these require > > db-level privileges). > > > > As a side note, what happens to the Sentry privileges when a table is > > renamed? I don't think we fixup all the Sentry privileges. Probably > > something to document. Hmm, so do you recommend sticking with either oracle or mysql? And sentry privileges are renamed as well when alter table rename is performed. This is the default behavior, but is configurable. > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java, > > line 205 > > <https://reviews.apache.org/r/25197/diff/5/?file=681366#file681366line205> > > > > Why does CREATEVIEW have a createViewPrivilege but DROPVIEW has a > > drop*Table*Privilege? createview is a command specific privilege(different from createtable) where we need create on db as well as select on table. Where as drop view is same as drop table. > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/SentryPolicyProviderForDb.java, > > line 151 > > <https://reviews.apache.org/r/25197/diff/5/?file=681373#file681373line151> > > > > extra ';' > > Also, do we now allow other privileges on the server? For example, can > > I grant SELECT on server and to a user to provide a read-only view of > > everything? Fixed the extraneous ; Yes, we are now supporting select on server, which would allow read-only of everything. > On Sept. 7, 2014, 10:48 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/SentryPolicyProviderForDb.java, > > line 152 > > <https://reviews.apache.org/r/25197/diff/5/?file=681373#file681373line152> > > > > should there be an "else" error case here for the case where all the > > object names are null? Well does not hurt to have error handling, but this code path is only used in testing. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25197/#review52562 ----------------------------------------------------------- On Sept. 5, 2014, 11:43 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25197/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2014, 11:43 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 > >
