You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2016/12/06 22:31:28 UTC
Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Repository: sentry
Description
-------
Both the server_name and action feilds are mandatory in previlage request.
I have made changes in the TSentryPrivilege object validation logic in sentry server which is applied as soon as receiving Thrift message from client.
If the validation is failed exception is raised and appropriate error is message is sent.
Diffs
-----
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
Diff: https://reviews.apache.org/r/54445/diff/
Testing
-------
I have done unit tests using sentry cli thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Dec. 6, 2016, 10:40 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java, line 1031
> > <https://reviews.apache.org/r/54445/diff/2/?file=1578156#file1578156line1031>
> >
> > 1. What if it's an empty string (i.e. combination of any space characterds)? Should probably be treated as missing value?
> >
> > 2. Should we strip leading and trailing spaces if any?
Yeah, that is an invalid input. Should be addressed.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review158238
-----------------------------------------------------------
On Dec. 6, 2016, 10:32 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 10:32 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Both the server_name and action feilds are mandatory in previlage request.
> I have made changes in the TSentryPrivilege object validation logic in sentry server which is applied as soon as receiving Thrift message from client.
> If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
>
> Diff: https://reviews.apache.org/r/54445/diff/
>
>
> Testing
> -------
>
> I have done unit tests using sentry cli thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 6, 2016, 10:40 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java, line 1031
> > <https://reviews.apache.org/r/54445/diff/2/?file=1578156#file1578156line1031>
> >
> > 1. What if it's an empty string (i.e. combination of any space characterds)? Should probably be treated as missing value?
> >
> > 2. Should we strip leading and trailing spaces if any?
>
> kalyan kumar kalvagadda wrote:
> Yeah, that is an invalid input. Should be addressed.
That's a good point - we do this stripping every time in SentryStore code.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review158238
-----------------------------------------------------------
On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:23 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Both the server_name and action feilds are mandatory in previlage request.
>
> I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
>
> If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
>
> Diff: https://reviews.apache.org/r/54445/diff/
>
>
> Testing
> -------
>
> I have done unit tests using sentry cli thrift client.
>
>
> File Attachments
> ----------------
>
> SENTRY-1547.002.patch
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review158238
-----------------------------------------------------------
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java (line 1031)
<https://reviews.apache.org/r/54445/#comment229031>
1. What if it's an empty string (i.e. combination of any space characterds)? Should probably be treated as missing value?
2. Should we strip leading and trailing spaces if any?
- Vadim Spector
On Dec. 6, 2016, 10:32 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 10:32 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Both the server_name and action feilds are mandatory in previlage request.
> I have made changes in the TSentryPrivilege object validation logic in sentry server which is applied as soon as receiving Thrift message from client.
> If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
>
> Diff: https://reviews.apache.org/r/54445/diff/
>
>
> Testing
> -------
>
> I have done unit tests using sentry cli thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review159246
-----------------------------------------------------------
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java (line 1031)
<https://reviews.apache.org/r/54445/#comment230191>
this and other places - is it possible to pass string with leading/trailing white spaces? If yes, need to trim (followed by checking length) or reject (need to decide if it's the right way).
- Vadim Spector
On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:23 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Both the server_name and action feilds are mandatory in previlage request.
>
> I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
>
> If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
>
> Diff: https://reviews.apache.org/r/54445/diff/
>
>
> Testing
> -------
>
> I have done unit tests using sentry cli thrift client.
>
>
> File Attachments
> ----------------
>
> SENTRY-1547.002.patch
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by Mat Crocker <ma...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review160917
-----------------------------------------------------------
minor nit, should be labeled "fields" not "feilds"
Looks like you handled all the right cases though
- Mat Crocker
On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:23 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Both the server_name and action feilds are mandatory in previlage request.
>
> I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
>
> If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
>
> Diff: https://reviews.apache.org/r/54445/diff/
>
>
> Testing
> -------
>
> I have done unit tests using sentry cli thrift client.
>
>
> File Attachments
> ----------------
>
> SENTRY-1547.002.patch
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review159252
-----------------------------------------------------------
It seems like you have the wrong diff here - you should have changes in SentryPolicyStoreProcessor but they are not in this diff.
- Alexander Kolbasov
On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:23 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Both the server_name and action feilds are mandatory in previlage request.
>
> I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
>
> If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
>
> Diff: https://reviews.apache.org/r/54445/diff/
>
>
> Testing
> -------
>
> I have done unit tests using sentry cli thrift client.
>
>
> File Attachments
> ----------------
>
> SENTRY-1547.002.patch
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/
-----------------------------------------------------------
(Updated Dec. 14, 2016, 10:23 p.m.)
Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
Repository: sentry
Description
-------
Both the server_name and action feilds are mandatory in previlage request.
I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
If the validation is failed exception is raised and appropriate error is message is sent.
Diffs
-----
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
Diff: https://reviews.apache.org/r/54445/diff/
Testing
-------
I have done unit tests using sentry cli thrift client.
File Attachments (updated)
----------------
SENTRY-1547.002.patch
https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/
-----------------------------------------------------------
(Updated Dec. 14, 2016, 10:11 p.m.)
Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Repository: sentry
Description (updated)
-------
Both the server_name and action feilds are mandatory in previlage request.
I had to take a different approach in solving the issue based on the review comments. I have added validation logic in SentryPolicyStoreProcessor enforcing the mandatory fields like server_name and action to be present in privilege/privileges added.
If the validation is failed exception is raised and appropriate error is message is sent.
Diffs
-----
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
Diff: https://reviews.apache.org/r/54445/diff/
Testing
-------
I have done unit tests using sentry cli thrift client.
File Attachments (updated)
----------------
SENTRY-1547.002.patch
https://reviews.apache.org/media/uploaded/files/2016/12/14/4c275619-1d37-42a0-a64b-c0e0752a8a63__SENTRY-1547.002.patch
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54445: SENTRY-1547 - It is possible to create a
privilege with all empty fields
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/
-----------------------------------------------------------
(Updated Dec. 6, 2016, 10:32 p.m.)
Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Repository: sentry
Description
-------
Both the server_name and action feilds are mandatory in previlage request.
I have made changes in the TSentryPrivilege object validation logic in sentry server which is applied as soon as receiving Thrift message from client.
If the validation is failed exception is raised and appropriate error is message is sent.
Diffs (updated)
-----
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
Diff: https://reviews.apache.org/r/54445/diff/
Testing
-------
I have done unit tests using sentry cli thrift client.
Thanks,
kalyan kumar kalvagadda