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 2018/01/22 21:16:20 UTC
Re: 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/
-----------------------------------------------------------
(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 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 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 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
>
>