You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2017/12/13 18:51:59 UTC
Review Request 64452: SENTRY-2091: User-based Privilege is broken by
SENTRY-769
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
Update the code to make sure user without group but with role has proper access.
test case is added for the scenario that user has no group but with desired role.
test case is added for the scenario that user has no group and no privilege to allow the access
Diffs
-----
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java c730a03
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
Diff: https://reviews.apache.org/r/64452/diff/1/
Testing
-------
unit tests
Thanks,
Na Li
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 15, 2017, 10:48 p.m., Vadim Spector wrote:
> > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
> > Line 372 (original), 372 (patched)
> > <https://reviews.apache.org/r/64452/diff/2/?file=1917438#file1917438line372>
> >
> > Are we sure here that CREATE fails for the right reason, i.e. user1 not having privileges, not because it already exists?
> >
> > If we want to make sure that user1 can neither CREATE nor SELECT, would it make sense to try CREATE on a different table, not the one that already exists, followed by SELECT on already existing table?
change to "CREATE TABLE db1.t_user1", so the table name is different now.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review193992
-----------------------------------------------------------
On Jan. 22, 2018, 9:16 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 9:16 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 447deaf
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java a9b98f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b6
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56
>
>
> Diff: https://reviews.apache.org/r/64452/diff/3/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review193992
-----------------------------------------------------------
sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
Line 372 (original), 372 (patched)
<https://reviews.apache.org/r/64452/#comment272683>
Are we sure here that CREATE fails for the right reason, i.e. user1 not having privileges, not because it already exists?
If we want to make sure that user1 can neither CREATE nor SELECT, would it make sense to try CREATE on a different table, not the one that already exists, followed by SELECT on already existing table?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
Line 370 (original), 368 (patched)
<https://reviews.apache.org/r/64452/#comment272684>
Ditto: do we know CREATE fails for the right reason (user1 lacking privilege), not because the table already exists?
- Vadim Spector
On Dec. 14, 2017, 8:54 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2017, 8:54 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java a41d1bd
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java f060b82
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
>
>
> Diff: https://reviews.apache.org/r/64452/diff/2/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review193979
-----------------------------------------------------------
Ship it!
Ship It!
- Arjun Mishra
On Dec. 14, 2017, 8:54 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2017, 8:54 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java a41d1bd
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java f060b82
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
>
>
> Diff: https://reviews.apache.org/r/64452/diff/2/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 19, 2017, 9:58 p.m., Steve Moist wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
> > Lines 280 (patched)
> > <https://reviews.apache.org/r/64452/diff/2/?file=1917439#file1917439line280>
> >
> > Worth it to add @Test(expected=HiveSQLException) or verify exception message?
I replace the try{}catch{} as context.assertAuthzException.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review194184
-----------------------------------------------------------
On Jan. 22, 2018, 9:16 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 9:16 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 447deaf
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java a9b98f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b6
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56
>
>
> Diff: https://reviews.apache.org/r/64452/diff/3/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Steve Moist via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review194184
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 1896 (original), 1896 (patched)
<https://reviews.apache.org/r/64452/#comment272908>
Can replace with CollectionUtils.isNotEmpty
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 1912 (original), 1912 (patched)
<https://reviews.apache.org/r/64452/#comment272907>
Can replace with CollectionUtils.isNotEmpty(users)
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
Lines 280 (patched)
<https://reviews.apache.org/r/64452/#comment272909>
Worth it to add @Test(expected=HiveSQLException) or verify exception message?
- Steve Moist
On Dec. 14, 2017, 8:54 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2017, 8:54 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java a41d1bd
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java f060b82
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
>
>
> Diff: https://reviews.apache.org/r/64452/diff/2/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 15, 2017, 3:37 p.m., Arjun Mishra wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
> > Lines 282 (patched)
> > <https://reviews.apache.org/r/64452/diff/2/?file=1917439#file1917439line282>
> >
> > Is the fail statement misplacd? Should it not be in the catch block?
No. It is expected to get exception since user does not have permission to "select c1 from t2". That is why the fail() is within the try{}. If no exception is thrown, the test will fail
> On Dec. 15, 2017, 3:37 p.m., Arjun Mishra wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
> > Lines 306 (patched)
> > <https://reviews.apache.org/r/64452/diff/2/?file=1917439#file1917439line306>
> >
> > Is the fail statement misplacd? Should it not be in the catch block?
No. It is expected to get exception since user does not have permission to "select c1 from t2". That is why the fail() is within the try{}. If no exception is thrown, the test will fail
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review193920
-----------------------------------------------------------
On Dec. 14, 2017, 8:54 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2017, 8:54 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java a41d1bd
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java f060b82
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
>
>
> Diff: https://reviews.apache.org/r/64452/diff/2/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review193920
-----------------------------------------------------------
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
Lines 282 (patched)
<https://reviews.apache.org/r/64452/#comment272587>
Is the fail statement misplacd? Should it not be in the catch block?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
Lines 306 (patched)
<https://reviews.apache.org/r/64452/#comment272588>
Is the fail statement misplacd? Should it not be in the catch block?
- Arjun Mishra
On Dec. 14, 2017, 8:54 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2017, 8:54 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java a41d1bd
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java f060b82
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
>
>
> Diff: https://reviews.apache.org/r/64452/diff/2/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review196435
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java
Lines 805-814 (patched)
<https://reviews.apache.org/r/64452/#comment276077>
This class is not used anymore and will be removed in the future, I don't think we should continue maintaining it.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
Line 550 (original)
<https://reviews.apache.org/r/64452/#comment276076>
All the HiveAuthzBindingHookBase changes are not related to the fix of this issue. Can we remove those changes to keep patch smaller and the history clean? I think We should fix those issues on other patches that address syntax issues.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Lines 107-109 (original), 107-109 (patched)
<https://reviews.apache.org/r/64452/#comment276078>
What's the reason of chaning to newHashSet() instead of leaving the emptySet()? Both will have empty sets, don't they?
Also, is it necessary to change the variable name 'e' to 'ex'?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
Lines 188 (patched)
<https://reviews.apache.org/r/64452/#comment276079>
This could have a tabl or empty space, Please remove this change.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
Lines 288-291 (patched)
<https://reviews.apache.org/r/64452/#comment276083>
Can this test be moved to the previous test? I think both tests are similar.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
Line 369 (original), 369 (patched)
<https://reviews.apache.org/r/64452/#comment276081>
Why do we need the table name change? Was the older name causing problems?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java
Lines 371 (patched)
<https://reviews.apache.org/r/64452/#comment276080>
This could have a tabl or empty space, Please remove this change.
- Sergio Pena
On Jan. 22, 2018, 9:16 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 9:16 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 447deaf
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java a9b98f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b6
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56
>
>
> Diff: https://reviews.apache.org/r/64452/diff/3/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review196465
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java
Lines 812 (patched)
<https://reviews.apache.org/r/64452/#comment276125>
Should this be logged as an error? With this feature this is valid scenarion and need logged as an error.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Line 109 (original), 109 (patched)
<https://reviews.apache.org/r/64452/#comment276120>
Should this be logged as an error? With this feature this is valid scenarion and need logged as an error.
- kalyan kumar kalvagadda
On Jan. 22, 2018, 9:16 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 9:16 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 447deaf
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java a9b98f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b6
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56
>
>
> Diff: https://reviews.apache.org/r/64452/diff/3/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Steve Moist via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review196462
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java
Lines 808 (patched)
<https://reviews.apache.org/r/64452/#comment276114>
Just do Set<String> groupNames = Sets.newHashSet(); to bypass the null assignment.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Line 104 (original), 104 (patched)
<https://reviews.apache.org/r/64452/#comment276115>
assign to Sets.newHashSet();
- Steve Moist
On Jan. 22, 2018, 9:16 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 9:16 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 447deaf
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java a9b98f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b6
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56
>
>
> Diff: https://reviews.apache.org/r/64452/diff/3/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/
-----------------------------------------------------------
(Updated Jan. 22, 2018, 9:16 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
Update the code to make sure user without group but with role has proper access.
test case is added for the scenario that user has no group but with desired role.
test case is added for the scenario that user has no group and no privilege to allow the access
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 447deaf
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java a9b98f3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b6
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56
Diff: https://reviews.apache.org/r/64452/diff/3/
Changes: https://reviews.apache.org/r/64452/diff/2-3/
Testing
-------
unit tests
Thanks,
Na Li
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/
-----------------------------------------------------------
(Updated Dec. 14, 2017, 8:54 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
Update the code to make sure user without group but with role has proper access.
test case is added for the scenario that user has no group but with desired role.
test case is added for the scenario that user has no group and no privilege to allow the access
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java a41d1bd
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java f060b82
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java 02ac514
Diff: https://reviews.apache.org/r/64452/diff/2/
Changes: https://reviews.apache.org/r/64452/diff/1-2/
Testing
-------
unit tests
Thanks,
Na Li
Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken
by SENTRY-769
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64452/#review193725
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java
Lines 811 (patched)
<https://reviews.apache.org/r/64452/#comment272343>
Can we add a log statement here?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
Lines 831 (patched)
<https://reviews.apache.org/r/64452/#comment272344>
Can we add a log statement here?
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Lines 107 (patched)
<https://reviews.apache.org/r/64452/#comment272345>
Can we add a log statement here?
- Arjun Mishra
On Dec. 13, 2017, 6:51 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64452/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 6:51 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update the code to make sure user without group but with role has proper access.
> test case is added for the scenario that user has no group but with desired role.
> test case is added for the scenario that user has no group and no privilege to allow the access
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java 5a21dd3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java 9c60c22
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 005724f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java c730a03
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java 5364937
>
>
> Diff: https://reviews.apache.org/r/64452/diff/1/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>