You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2014/09/05 23:41:39 UTC
Re: Review Request 25197: SENTRY-331: Add more granular privileges to
the DBModel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25197/#review52502
-----------------------------------------------------------
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.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java
<https://reviews.apache.org/r/25197/#comment91307>
Is this causing a backward incompatibility for SQL client ?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
<https://reviews.apache.org/r/25197/#comment91311>
Can we remove these todos ?
- Prasad Mujumdar
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
>
>
Re: Review Request 25197: SENTRY-331: Add more granular privileges to
the DBModel
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> 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
>
>
Re: Review Request 25197: SENTRY-331: Add more granular privileges to
the DBModel
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> 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.
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? :-)
> On Sept. 5, 2014, 9:41 p.m., Prasad Mujumdar 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/4/?file=673111#file673111line593>
> >
> > Is this causing a backward incompatibility for SQL client ?
Nope, this is just changing a private method to throw Exception, which is already being handled in the calling public function.
> 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 ?
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.
- Sravya
-----------------------------------------------------------
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
>
>