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/25 03:36:56 UTC
Review Request 26011: SENTRY-469: TListSentryPrivilegesByAuthRequest
API should support impersonation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26011/
-----------------------------------------------------------
Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
Bugs: SENTRY-469
https://issues.apache.org/jira/browse/SENTRY-469
Repository: sentry
Description
-------
list_sentry_privileges_by_authorizable API should support impersonation similar to other Sentry service RPCs.
- Add requester username as required argument for the API
- Verify the admin status of the requesting user
- Validate that the requesting user is part of groups and the roles it's trying to access
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 0668912
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift d8357aa
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 38cb39b
Diff: https://reviews.apache.org/r/26011/diff/
Testing
-------
Added test case to validated various options of the list_sentry_privileges_by_authorizable() for non-admin user.
Thanks,
Prasad Mujumdar
Re: Review Request 26011: SENTRY-469:
TListSentryPrivilegesByAuthRequest API should support impersonation
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26011/#review54505
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/26011/#comment94687>
Thanks for catching that. Fixed
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/26011/#comment94688>
The main reason is to handle the error case. If you have required fields in the response, then you need to make sure to set those some dummy/empty values before raising an error. Otherwise the client gets a Thrift exception due to reply missing a required field.
will add a comment.
- Prasad Mujumdar
On Sept. 25, 2014, 1:36 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26011/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 1:36 a.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-469
> https://issues.apache.org/jira/browse/SENTRY-469
>
>
> Repository: sentry
>
>
> Description
> -------
>
> list_sentry_privileges_by_authorizable API should support impersonation similar to other Sentry service RPCs.
> - Add requester username as required argument for the API
> - Verify the admin status of the requesting user
> - Validate that the requesting user is part of groups and the roles it's trying to access
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 0668912
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift d8357aa
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 38cb39b
>
> Diff: https://reviews.apache.org/r/26011/diff/
>
>
> Testing
> -------
>
> Added test case to validated various options of the list_sentry_privileges_by_authorizable() for non-admin user.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 26011: SENTRY-469:
TListSentryPrivilegesByAuthRequest API should support impersonation
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26011/
-----------------------------------------------------------
(Updated Sept. 25, 2014, 3:17 a.m.)
Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
Changes
-------
Addressed review feedback
- Fixed case insensitive role comparison, added testcase
- Added comment in the thrif file
- Fixed typos in variable name
Bugs: SENTRY-469
https://issues.apache.org/jira/browse/SENTRY-469
Repository: sentry
Description
-------
list_sentry_privileges_by_authorizable API should support impersonation similar to other Sentry service RPCs.
- Add requester username as required argument for the API
- Verify the admin status of the requesting user
- Validate that the requesting user is part of groups and the roles it's trying to access
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 0668912
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift d8357aa
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 38cb39b
Diff: https://reviews.apache.org/r/26011/diff/
Testing
-------
Added test case to validated various options of the list_sentry_privileges_by_authorizable() for non-admin user.
Thanks,
Prasad Mujumdar
Re: Review Request 26011: SENTRY-469:
TListSentryPrivilegesByAuthRequest API should support impersonation
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26011/#review54502
-----------------------------------------------------------
Ship it!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/26011/#comment94684>
role is case insensitive, and we persist lower cased roles in db. Hence we need to do toLower here.
- Sravya Tirukkovalur
On Sept. 25, 2014, 1:36 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26011/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 1:36 a.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-469
> https://issues.apache.org/jira/browse/SENTRY-469
>
>
> Repository: sentry
>
>
> Description
> -------
>
> list_sentry_privileges_by_authorizable API should support impersonation similar to other Sentry service RPCs.
> - Add requester username as required argument for the API
> - Verify the admin status of the requesting user
> - Validate that the requesting user is part of groups and the roles it's trying to access
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 0668912
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift d8357aa
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 38cb39b
>
> Diff: https://reviews.apache.org/r/26011/diff/
>
>
> Testing
> -------
>
> Added test case to validated various options of the list_sentry_privileges_by_authorizable() for non-admin user.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 26011: SENTRY-469:
TListSentryPrivilegesByAuthRequest API should support impersonation
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26011/#review54493
-----------------------------------------------------------
Ship it!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/26011/#comment94677>
since the roles are case-sensitive, do you need to call .toLower() on "role"? Might be good to add a test case for this.
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/26011/#comment94678>
comment on when this wouldn't be set... Should we really change it to optional or should it be set to an empty map if nothing matches?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java
<https://reviews.apache.org/r/26011/#comment94679>
Fix spelling of Authrizable
- Lenni Kuff
On Sept. 25, 2014, 1:36 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26011/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 1:36 a.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-469
> https://issues.apache.org/jira/browse/SENTRY-469
>
>
> Repository: sentry
>
>
> Description
> -------
>
> list_sentry_privileges_by_authorizable API should support impersonation similar to other Sentry service RPCs.
> - Add requester username as required argument for the API
> - Verify the admin status of the requesting user
> - Validate that the requesting user is part of groups and the roles it's trying to access
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 0668912
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift d8357aa
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 38cb39b
>
> Diff: https://reviews.apache.org/r/26011/diff/
>
>
> Testing
> -------
>
> Added test case to validated various options of the list_sentry_privileges_by_authorizable() for non-admin user.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>