----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25197/#review52562 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java <https://reviews.apache.org/r/25197/#comment91394> Curious what the point of ALLOWED_PRIVS? It seems like it is just a set of all the values in the PrivilegeType enum. sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java <https://reviews.apache.org/r/25197/#comment91392> Might be more naturally expressed with a case statement. sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java <https://reviews.apache.org/r/25197/#comment91393> Maybe: "Unknown privilege type: " + privilegeType". Also, should this be a SentryUserException? It seems more like an UnsupportedOperationException or IllegalStateException sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java <https://reviews.apache.org/r/25197/#comment91395> nit: rename to createTablePrivilege to be consistent with createDb/createServer naming. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java <https://reviews.apache.org/r/25197/#comment91391> 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. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java <https://reviews.apache.org/r/25197/#comment91396> Why does CREATEVIEW have a createViewPrivilege but DROPVIEW has a drop*Table*Privilege? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/SentryPolicyProviderForDb.java <https://reviews.apache.org/r/25197/#comment91397> 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? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/SentryPolicyProviderForDb.java <https://reviews.apache.org/r/25197/#comment91398> should there be an "else" error case here for the case where all the object names are null? - Lenni Kuff 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 > >
