You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vamsee Yarlagadda <va...@cloudera.com> on 2013/12/30 22:59:17 UTC
Review Request 16522: SENTRY-70: Solr Query authorization tests for Sentry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/
-----------------------------------------------------------
Review request for sentry and Gregory Chanan.
Bugs: SENTRY-70
https://issues.apache.org/jira/browse/SENTRY-70
Repository: sentry
Description
-------
This patch contains tests for validating Query Authorization of Solr/Sentry.
- Includes both +ve and -ve tests.
- Tests will be run for all possible combinations of Query+Update+All privileges.
Diffs
-----
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
Diff: https://reviews.apache.org/r/16522/diff/
Testing
-------
All the tests(Query + Update) passed without any problem.
Thanks,
Vamsee Yarlagadda
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Dec. 31, 2013, 8:34 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 472
> > <https://reviews.apache.org/r/16522/diff/1-3/?file=404614#file404614line472>
> >
> > you don't need the true here.
>
> Vamsee Yarlagadda wrote:
> I didn't get your point.
> Did you mean to use in this way?
> if (field.equals("_version_")) {
> ...
> }
> or something different?
Yup, that's all my meant. Not a big deal at all.
> On Dec. 31, 2013, 8:34 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 467
> > <https://reviews.apache.org/r/16522/diff/1-3/?file=404614#file404614line467>
> >
> > You don't actually check that the solrRespDoc has _version_ as well.
> >
>
> Vamsee Yarlagadda wrote:
> Well, based on the solr schema.xml we are using, the solrRespDoc will always have a "_version_" attached with it. Moreover, it is a reserved field in Solr. So, we need to check only solr input document.
But subclasses can override the schema that is used and might not have a _version_.
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review30999
-----------------------------------------------------------
On Jan. 3, 2014, 10:02 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 10:02 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Dec. 31, 2013, 8:34 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 472
> > <https://reviews.apache.org/r/16522/diff/1-3/?file=404614#file404614line472>
> >
> > you don't need the true here.
I didn't get your point.
Did you mean to use in this way?
if (field.equals("_version_")) {
...
}
or something different?
> On Dec. 31, 2013, 8:34 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 467
> > <https://reviews.apache.org/r/16522/diff/1-3/?file=404614#file404614line467>
> >
> > You don't actually check that the solrRespDoc has _version_ as well.
> >
Well, based on the solr schema.xml we are using, the solrRespDoc will always have a "_version_" attached with it. Moreover, it is a reserved field in Solr. So, we need to check only solr input document.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review30999
-----------------------------------------------------------
On Dec. 31, 2013, 7:45 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 7:45 a.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review30999
-----------------------------------------------------------
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59370>
I don't buy that you need this function...Why can't you get the list of documents, instead of just the size, then get it again at the end and call the SolrDocumentList version of this function?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59371>
same as above.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59361>
I don't think you need the null stuff anymore now that you have ALL_DOCS. Can we just assert that the param is non-null somewhere?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59362>
same here.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59369>
I still don't buy that you need this, see above.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59366>
this makes me pretty nervous...in all the previous calls to getSolrDocs you go back to the user name right away, but not here. It becomes pretty difficult to track who is actually the current user after a function call. I worry we will test something as the admin user by accident.
I think the following would be an improvement:
whenever you call setAuthenticationUser, record the previous user name and restore it at the end of the function. That way, the user is always the same. That only applies for the TestBase, you can do whatever you want in the actual test classes.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59363>
same here
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59364>
it's actually a boolean -- different type
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59368>
You don't actually check that the solrRespDoc has _version_ as well.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59367>
you don't need the true here.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java
<https://reviews.apache.org/r/16522/#comment59358>
That is a strange usage of the word "populate" -- honestly I'd just get rid of the comment.
- Gregory Chanan
On Dec. 31, 2013, 7:45 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 7:45 a.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31182
-----------------------------------------------------------
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59586>
There is no reason for this to be a ternary operator, because one of the possibilities is just to set the value to itself (i.e. this is just an if).
You either want to define this on one line, i.e.:
int expectedRespFieldCount = ()?solrRespDoc.size()-1:solrRespDoc.size()
or just do an if:
if (containsKey) expectedRespFieldCount -= 1;
- Gregory Chanan
On Jan. 3, 2014, 11:01 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 11:01 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31192
-----------------------------------------------------------
Ship it!
Looks good to me. Attach the patch to the JIRA and I'll commit it if it passes for me.
- Gregory Chanan
On Jan. 3, 2014, 11:57 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 11:57 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/
-----------------------------------------------------------
(Updated Jan. 3, 2014, 11:57 p.m.)
Review request for sentry and Gregory Chanan.
Bugs: SENTRY-70
https://issues.apache.org/jira/browse/SENTRY-70
Repository: sentry
Description
-------
This patch contains tests for validating Query Authorization of Solr/Sentry.
- Includes both +ve and -ve tests.
- Tests will be run for all possible combinations of Query+Update+All privileges.
Diffs (updated)
-----
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
Diff: https://reviews.apache.org/r/16522/diff/
Testing
-------
All the tests(Query + Update) passed without any problem.
Thanks,
Vamsee Yarlagadda
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/
-----------------------------------------------------------
(Updated Jan. 3, 2014, 11:01 p.m.)
Review request for sentry and Gregory Chanan.
Bugs: SENTRY-70
https://issues.apache.org/jira/browse/SENTRY-70
Repository: sentry
Description
-------
This patch contains tests for validating Query Authorization of Solr/Sentry.
- Includes both +ve and -ve tests.
- Tests will be run for all possible combinations of Query+Update+All privileges.
Diffs (updated)
-----
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
Diff: https://reviews.apache.org/r/16522/diff/
Testing
-------
All the tests(Query + Update) passed without any problem.
Thanks,
Vamsee Yarlagadda
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Jan. 3, 2014, 11 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 260
> > <https://reviews.apache.org/r/16522/diff/3-4/?file=407128#file407128line260>
> >
> > To do this right you really have to do a nested try i think...just put everything besides the original user in the try and put the finally at the end of the function.
> >
> > That way you don't have to worry about what lines can/can't throw an exception and are guaranteed to set the authentication user correctly at the end.
>
> Vamsee Yarlagadda wrote:
> Let's say i put the validateSolrDocCountAndContent inside the try loop, and if there is any error in data validation; an exception would be raised, which would trigger the "catch" to check for Sentry error. But ideally this shouldn't be checking for Sentry error at all.
>
> AND Moreover, there are only a limited number of functions that talk with Solr. "validateSolrDocCountAndContent" doesn't talk with Solr and so it will not have any impact on authenticated user.
>
> Gregory Chanan wrote:
> I said a *nested* try. Your function would look this like:
>
> String originalUser = getAuthenticatedUser();
> try {
> ...
> try {
> } catch (...)
> // look for sentry error
> }
> } finally {
> // set the original user back
> }
Got it. Thanks for pointing.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31174
-----------------------------------------------------------
On Jan. 3, 2014, 11:57 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 11:57 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Jan. 3, 2014, 11 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 167
> > <https://reviews.apache.org/r/16522/diff/3-4/?file=407128#file407128line167>
> >
> > this should be private, you don't want subclasses to call this. It's an implementation detail of how verifyUpdatePass(username...) and other functions do the implementation. Ideally there wouldn't be a global static user, solrj would just set the user name (somehow) in each request. Since we can't do that (yet?), we have this implementation detail.
Good idea. Will change it to private.
> On Jan. 3, 2014, 11 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 260
> > <https://reviews.apache.org/r/16522/diff/3-4/?file=407128#file407128line260>
> >
> > To do this right you really have to do a nested try i think...just put everything besides the original user in the try and put the finally at the end of the function.
> >
> > That way you don't have to worry about what lines can/can't throw an exception and are guaranteed to set the authentication user correctly at the end.
Let's say i put the validateSolrDocCountAndContent inside the try loop, and if there is any error in data validation; an exception would be raised, which would trigger the "catch" to check for Sentry error. But ideally this shouldn't be checking for Sentry error at all.
AND Moreover, there are only a limited number of functions that talk with Solr. "validateSolrDocCountAndContent" doesn't talk with Solr and so it will not have any impact on authenticated user.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31174
-----------------------------------------------------------
On Jan. 3, 2014, 11:01 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 11:01 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31174
-----------------------------------------------------------
On Jan. 3, 2014, 11:57 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 11:57 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Jan. 3, 2014, 11 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 260
> > <https://reviews.apache.org/r/16522/diff/3-4/?file=407128#file407128line260>
> >
> > To do this right you really have to do a nested try i think...just put everything besides the original user in the try and put the finally at the end of the function.
> >
> > That way you don't have to worry about what lines can/can't throw an exception and are guaranteed to set the authentication user correctly at the end.
>
> Vamsee Yarlagadda wrote:
> Let's say i put the validateSolrDocCountAndContent inside the try loop, and if there is any error in data validation; an exception would be raised, which would trigger the "catch" to check for Sentry error. But ideally this shouldn't be checking for Sentry error at all.
>
> AND Moreover, there are only a limited number of functions that talk with Solr. "validateSolrDocCountAndContent" doesn't talk with Solr and so it will not have any impact on authenticated user.
I said a *nested* try. Your function would look this like:
String originalUser = getAuthenticatedUser();
try {
...
try {
} catch (...)
// look for sentry error
}
} finally {
// set the original user back
}
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31174
-----------------------------------------------------------
On Jan. 3, 2014, 11:01 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 11:01 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review31174
-----------------------------------------------------------
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59575>
this should be private, you don't want subclasses to call this. It's an implementation detail of how verifyUpdatePass(username...) and other functions do the implementation. Ideally there wouldn't be a global static user, solrj would just set the user name (somehow) in each request. Since we can't do that (yet?), we have this implementation detail.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59577>
any reason this can't all go inside the try?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59579>
To do this right you really have to do a nested try i think...just put everything besides the original user in the try and put the finally at the end of the function.
That way you don't have to worry about what lines can/can't throw an exception and are guaranteed to set the authentication user correctly at the end.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59580>
same as above, just throw everything in the try.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59581>
You can't still use validateSolrDocCountAndContent? Record the docs, not the count? That way everything is standard, you don't have to do anything thinking whether you expect 0 or more docs and copy and paste this error message everywhere.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59582>
same as above, you can't use validateSolrDocCountAndContent?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59569>
Put this first and put the rest of the function in the try?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59562>
isn't this in the wrong order?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59566>
just put everything in the try (besides the variable declaration), otherwise what happens if something outside the try throws an exception?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59564>
probably cleaner to put the return in the try and just have the finally at the end of the function.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59561>
this should be in a finally, like above, right?
- Gregory Chanan
On Jan. 3, 2014, 10:02 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 10:02 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/
-----------------------------------------------------------
(Updated Jan. 3, 2014, 10:02 p.m.)
Review request for sentry and Gregory Chanan.
Changes
-------
Fixed issues pointed out by Greg.
Bugs: SENTRY-70
https://issues.apache.org/jira/browse/SENTRY-70
Repository: sentry
Description
-------
This patch contains tests for validating Query Authorization of Solr/Sentry.
- Includes both +ve and -ve tests.
- Tests will be run for all possible combinations of Query+Update+All privileges.
Diffs (updated)
-----
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java 997d420ae9c8ea1f950b8212e06d2c023fce020d
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java 131fa2cc41d558081f80cc5880b2dce75e4ba027
Diff: https://reviews.apache.org/r/16522/diff/
Testing
-------
All the tests(Query + Update) passed without any problem.
Thanks,
Vamsee Yarlagadda
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/
-----------------------------------------------------------
(Updated Dec. 31, 2013, 7:45 a.m.)
Review request for sentry and Gregory Chanan.
Bugs: SENTRY-70
https://issues.apache.org/jira/browse/SENTRY-70
Repository: sentry
Description
-------
This patch contains tests for validating Query Authorization of Solr/Sentry.
- Includes both +ve and -ve tests.
- Tests will be run for all possible combinations of Query+Update+All privileges.
Diffs (updated)
-----
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
Diff: https://reviews.apache.org/r/16522/diff/
Testing
-------
All the tests(Query + Update) passed without any problem.
Thanks,
Vamsee Yarlagadda
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Dec. 30, 2013, 11:36 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 366
> > <https://reviews.apache.org/r/16522/diff/2/?file=404618#file404618line366>
> >
> > can we get rid of this function and just use validateSolrDocListsContent?
>
> Vamsee Yarlagadda wrote:
> Both the functions are quite different in their behavior. This might involve in adding some extra params to the function, which may result in lot of confusion.
It looks like you changed this in the latest version?
> On Dec. 30, 2013, 11:36 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 418
> > <https://reviews.apache.org/r/16522/diff/2/?file=404618#file404618line418>
> >
> > can this be private?
>
> Vamsee Yarlagadda wrote:
> If the inheriting classes want to leverage this function, then it would be better to have this as protected.
Sure, but you could make this argument about every function. In general I think it's better to minimize the number of public/protected functions because it makes it easier to grok for users. If they really need some functionality they can copy the code locally temporarily and request that you make it protected/public.
For example, check out the class that AbstractSolrSentryTestBase depends on: http://lucene.apache.org/solr/4_6_0/solr-test-framework/org/apache/solr/cloud/AbstractFullDistribZkTestBase.html
There's something like 200-300 functions there!
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review30985
-----------------------------------------------------------
On Dec. 31, 2013, 7:45 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 7:45 a.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Dec. 30, 2013, 11:36 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 418
> > <https://reviews.apache.org/r/16522/diff/2/?file=404618#file404618line418>
> >
> > can this be private?
If the inheriting classes want to leverage this function, then it would be better to have this as protected.
> On Dec. 30, 2013, 11:36 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 385
> > <https://reviews.apache.org/r/16522/diff/2/?file=404618#file404618line385>
> >
> > It's pretty confusing that getSolrDocs queries solr as an admin user and querySolr doesn't and that's the only difference.
> >
> > There's a bunch of options you can do: Either call the first getSolrDocsAsAdmin or get rid of that function and have the test cases write out the two lines or just have one function that takes in a user (if null, don't call setAuthenticationUser). I prefer minimizing the number of functions but no big deal either way.
Added extra boolean parameter that specifies whether to run the command as admin or not.
> On Dec. 30, 2013, 11:36 p.m., Gregory Chanan wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java, line 366
> > <https://reviews.apache.org/r/16522/diff/2/?file=404618#file404618line366>
> >
> > can we get rid of this function and just use validateSolrDocListsContent?
Both the functions are quite different in their behavior. This might involve in adding some extra params to the function, which may result in lot of confusion.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review30985
-----------------------------------------------------------
On Dec. 31, 2013, 7:45 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 7:45 a.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 16522: SENTRY-70: Solr Query authorization tests for
Sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16522/#review30985
-----------------------------------------------------------
It might be easier/clearer to define an ALL_DOCS constant for "*:*" rather than relying on null everywhere.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59298>
updated -> queried
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59302>
What if the query string is null?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59300>
This spacing is right? In SENTRY-70 I thought the parameters lined up?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59297>
As I mention below, this should be moved into validateSolrDocListsContent.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59299>
updated -> queried
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59303>
what if the query string is null?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59301>
Spacing.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59294>
can we get rid of this function and just use validateSolrDocListsContent?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59285>
doc what happens if the query string is null
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59288>
It's pretty confusing that getSolrDocs queries solr as an admin user and querySolr doesn't and that's the only difference.
There's a bunch of options you can do: Either call the first getSolrDocsAsAdmin or get rid of that function and have the test cases write out the two lines or just have one function that takes in a user (if null, don't call setAuthenticationUser). I prefer minimizing the number of functions but no big deal either way.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59286>
remove whitespace.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59287>
doc what happens if query string is null
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59289>
can this be private?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59290>
should you check getNumFound is the same for both?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java
<https://reviews.apache.org/r/16522/#comment59292>
In the review of SENTRY-71, I mentioned that we shouldn't be modifying the user's document; in fact the comment here says it "ignores" it but what it actually does is "remove" it. Let's make it ignore it but keep it in.
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java
<https://reviews.apache.org/r/16522/#comment59304>
It's not really "creating" the user name; the user name is already defined in the .ini. It's getting the user name, I guess.
- Gregory Chanan
On Dec. 30, 2013, 9:59 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16522/
> -----------------------------------------------------------
>
> (Updated Dec. 30, 2013, 9:59 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Bugs: SENTRY-70
> https://issues.apache.org/jira/browse/SENTRY-70
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch contains tests for validating Query Authorization of Solr/Sentry.
> - Includes both +ve and -ve tests.
> - Tests will be run for all possible combinations of Query+Update+All privileges.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java c0abd289c67608b400699b376463ad0cb444a526
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16522/diff/
>
>
> Testing
> -------
>
> All the tests(Query + Update) passed without any problem.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>