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/03/22 06:52:37 UTC
Review Request 19560: SENTRY-137: Validate privilege scope in sentry service
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/
-----------------------------------------------------------
Review request for sentry, Brock Noland and Shreepadma Venugopalan.
Bugs: SENTRY-137
https://issues.apache.org/jira/browse/SENTRY-137
Repository: sentry
Description
-------
Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
Ensure the SentryPolicyStoreClient client is setting this properties.
Validate this property when on the grant/revoke privilege commands in SentryStore
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
Diff: https://reviews.apache.org/r/19560/diff/
Testing
-------
sentry-provider-db tests pass with the patch.
Thanks,
Prasad Mujumdar
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On March 24, 2014, 2:06 p.m., Brock Noland wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, line 159
> > <https://reviews.apache.org/r/19560/diff/1/?file=532871#file532871line159>
> >
> > I see I used "SERVER" on the left, but I think this should be URI?
yes, that sounds correct. We do support DB level URIs as well.
Fixed.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/#review38296
-----------------------------------------------------------
On March 22, 2014, 5:52 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19560/
> -----------------------------------------------------------
>
> (Updated March 22, 2014, 5:52 a.m.)
>
>
> Review request for sentry, Brock Noland and Shreepadma Venugopalan.
>
>
> Bugs: SENTRY-137
> https://issues.apache.org/jira/browse/SENTRY-137
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
> Ensure the SentryPolicyStoreClient client is setting this properties.
> Validate this property when on the grant/revoke privilege commands in SentryStore
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
>
> Diff: https://reviews.apache.org/r/19560/diff/
>
>
> Testing
> -------
>
> sentry-provider-db tests pass with the patch.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/#review38296
-----------------------------------------------------------
Nice work! LGTM, just two comments. I see grant/revoke for URI are using scope=server. I think we should change that to URI?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/19560/#comment70340>
I see I used "SERVER" on the left, but I think this should be URI?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/19560/#comment70339>
tailing ws
- Brock Noland
On March 22, 2014, 5:52 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19560/
> -----------------------------------------------------------
>
> (Updated March 22, 2014, 5:52 a.m.)
>
>
> Review request for sentry, Brock Noland and Shreepadma Venugopalan.
>
>
> Bugs: SENTRY-137
> https://issues.apache.org/jira/browse/SENTRY-137
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
> Ensure the SentryPolicyStoreClient client is setting this properties.
> Validate this property when on the grant/revoke privilege commands in SentryStore
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
>
> Diff: https://reviews.apache.org/r/19560/diff/
>
>
> Testing
> -------
>
> sentry-provider-db tests pass with the patch.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On March 24, 2014, 8:20 p.m., Brock Noland wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, line 214
> > <https://reviews.apache.org/r/19560/diff/2/?file=534082#file534082line214>
> >
> > I think this one needs to be URI as well?
yes, sorry missed that one.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/#review38353
-----------------------------------------------------------
On March 24, 2014, 8:49 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19560/
> -----------------------------------------------------------
>
> (Updated March 24, 2014, 8:49 p.m.)
>
>
> Review request for sentry, Brock Noland and Shreepadma Venugopalan.
>
>
> Bugs: SENTRY-137
> https://issues.apache.org/jira/browse/SENTRY-137
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
> Ensure the SentryPolicyStoreClient client is setting this properties.
> Validate this property when on the grant/revoke privilege commands in SentryStore
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
>
> Diff: https://reviews.apache.org/r/19560/diff/
>
>
> Testing
> -------
>
> sentry-provider-db tests pass with the patch.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/#review38353
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/19560/#comment70438>
I think this one needs to be URI as well?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/19560/#comment70440>
trailing ws
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/19560/#comment70441>
trailing ws
- Brock Noland
On March 24, 2014, 5:24 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19560/
> -----------------------------------------------------------
>
> (Updated March 24, 2014, 5:24 p.m.)
>
>
> Review request for sentry, Brock Noland and Shreepadma Venugopalan.
>
>
> Bugs: SENTRY-137
> https://issues.apache.org/jira/browse/SENTRY-137
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
> Ensure the SentryPolicyStoreClient client is setting this properties.
> Validate this property when on the grant/revoke privilege commands in SentryStore
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
>
> Diff: https://reviews.apache.org/r/19560/diff/
>
>
> Testing
> -------
>
> sentry-provider-db tests pass with the patch.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/#review38574
-----------------------------------------------------------
Ship it!
Ship It!
- Brock Noland
On March 24, 2014, 8:49 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19560/
> -----------------------------------------------------------
>
> (Updated March 24, 2014, 8:49 p.m.)
>
>
> Review request for sentry, Brock Noland and Shreepadma Venugopalan.
>
>
> Bugs: SENTRY-137
> https://issues.apache.org/jira/browse/SENTRY-137
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
> Ensure the SentryPolicyStoreClient client is setting this properties.
> Validate this property when on the grant/revoke privilege commands in SentryStore
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
>
> Diff: https://reviews.apache.org/r/19560/diff/
>
>
> Testing
> -------
>
> sentry-provider-db tests pass with the patch.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/
-----------------------------------------------------------
(Updated March 24, 2014, 8:49 p.m.)
Review request for sentry, Brock Noland and Shreepadma Venugopalan.
Changes
-------
Changer per review feedback
Bugs: SENTRY-137
https://issues.apache.org/jira/browse/SENTRY-137
Repository: sentry
Description
-------
Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
Ensure the SentryPolicyStoreClient client is setting this properties.
Validate this property when on the grant/revoke privilege commands in SentryStore
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
Diff: https://reviews.apache.org/r/19560/diff/
Testing
-------
sentry-provider-db tests pass with the patch.
Thanks,
Prasad Mujumdar
Re: Review Request 19560: SENTRY-137: Validate privilege scope in sentry
service
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19560/
-----------------------------------------------------------
(Updated March 24, 2014, 5:24 p.m.)
Review request for sentry, Brock Noland and Shreepadma Venugopalan.
Changes
-------
Changes per review feedback.
Bugs: SENTRY-137
https://issues.apache.org/jira/browse/SENTRY-137
Repository: sentry
Description
-------
Put privilege scopes in SentryConstants [SERVER, DB, TABLE, URI]
Ensure the SentryPolicyStoreClient client is setting this properties.
Validate this property when on the grant/revoke privilege commands in SentryStore
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 464569c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 9562783
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 1b36690
Diff: https://reviews.apache.org/r/19560/diff/
Testing
-------
sentry-provider-db tests pass with the patch.
Thanks,
Prasad Mujumdar