Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

2019-02-15 Thread Arjun Mishra via Review Board 


> On Feb. 15, 2019, 6:52 p.m., Arjun Mishra wrote:
> > Haley I think we can simplify this a lot without any refactoring by using 
> > PathUtils#parseLocalURI method.
> > 
> > Since the method is isLocalUri => It is looking to check for a local URI. 
> > If uri.getScheme() throws null we can set uri = 
> > PathUtils.parseLocalURI(path). If that is still null we return false. 
> > 
> > Let me know your thoughts
> 
> Haley Reeve wrote:
> That's an alternate way of doing it. In that case the only way we'd get 
> to false is to catch the IllegalArgumentException PathUtils#parseLocalURI 
> throws. I don't think it greatly simplifies anything except that we don't 
> have to pass down the HiveAuthzConf anymore. Is adding that parameter to the 
> function definitions undesirable?

Yeah I think so. It is too much of refactoring for not much gain. We want to 
check if it is a local URI. If You can't parse a local URI then it definitely 
isnt. So that should be sufficient. 
If you want you can directly instantiate: URI uri = new 
URI(PathUtils.parseLocalURI(uriString)); and catch exception.
Will simplify code


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
---


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

2019-02-15 Thread Haley Reeve via Review Board 


> On Feb. 15, 2019, 6:52 p.m., Arjun Mishra wrote:
> > Haley I think we can simplify this a lot without any refactoring by using 
> > PathUtils#parseLocalURI method.
> > 
> > Since the method is isLocalUri => It is looking to check for a local URI. 
> > If uri.getScheme() throws null we can set uri = 
> > PathUtils.parseLocalURI(path). If that is still null we return false. 
> > 
> > Let me know your thoughts

That's an alternate way of doing it. In that case the only way we'd get to 
false is to catch the IllegalArgumentException PathUtils#parseLocalURI throws. 
I don't think it greatly simplifies anything except that we don't have to pass 
down the HiveAuthzConf anymore. Is adding that parameter to the function 
definitions undesirable?


- Haley


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
---


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

2019-02-15 Thread Arjun Mishra via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212874
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Line 283 (original), 285 (patched)


Use PathUtils#parseLocalURI method if uri.getScheme is null to 
re-intantiate the object. That should work


- Arjun Mishra


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

2019-02-15 Thread Arjun Mishra via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69987/#review212873
---



Haley I think we can simplify this a lot without any refactoring by using 
PathUtils#parseLocalURI method.

Since the method is isLocalUri => It is looking to check for a local URI. If 
uri.getScheme() throws null we can set uri = PathUtils.parseLocalURI(path). If 
that is still null we return false. 

Let me know your thoughts

- Arjun Mishra


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69975: SENTRY-2495: Support Conjunctive Matching in Solr QueryDocAuthorizationComponent

2019-02-15 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69975/#review212872
---


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Feb. 15, 2019, 4:54 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69975/
> ---
> 
> (Updated Feb. 15, 2019, 4:54 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adds a conjunctive match capabiltiy to the existing 
> QueryDocAuthorizationComponent (which applies document level security against 
> Sentry roles)
> 
> 
> Diffs
> -
> 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/DocAuthorizationComponent.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java
>  9da3d6e1 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SubsetQueryPlugin.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/SubsetQueryTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/schema-docValuesSubsetMatch.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetmatchcomponent.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetquery.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig.snippet.randomindexconfig.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestCase.java
>  3d4d555f 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/DocLevelGenerator.java
>  40cc153e 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/SolrSentryServiceTestBase.java
>  e1f789cb 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestDocLevelOperations.java
>  7834f339 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSubsetQueryOperations.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/log4j.properties d9418167 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match/conf/schema.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match/conf/solrconfig.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match_missing_false/conf/schema.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match_missing_false/conf/solrconfig.xml
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69975/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added for SubsetQueryPlugin
> End-to-end tests added under sentry-tests/senty-tests-solr for full 
> SolrCloud+Sentry Service using the new QueryDocAuthorizationComponent in 
> conjunctive match mode
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

Re: Review Request 69975: SENTRY-2495: Support Conjunctive Matching in Solr QueryDocAuthorizationComponent

2019-02-15 Thread Tristan Stevens via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69975/
---

(Updated Feb. 15, 2019, 4:54 p.m.)


Review request for sentry.


Changes
---

Updated patch with missing file(s)


Repository: sentry


Description
---

Adds a conjunctive match capabiltiy to the existing 
QueryDocAuthorizationComponent (which applies document level security against 
Sentry roles)


Diffs (updated)
-

  
sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/DocAuthorizationComponent.java
 PRE-CREATION 
  
sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java
 9da3d6e1 
  
sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SubsetQueryPlugin.java
 PRE-CREATION 
  
sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/SubsetQueryTest.java
 PRE-CREATION 
  
sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/schema-docValuesSubsetMatch.xml
 PRE-CREATION 
  
sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetmatchcomponent.xml
 PRE-CREATION 
  
sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetquery.xml
 PRE-CREATION 
  
sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig.snippet.randomindexconfig.xml
 PRE-CREATION 
  
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestCase.java
 3d4d555f 
  
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/DocLevelGenerator.java
 40cc153e 
  
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/SolrSentryServiceTestBase.java
 e1f789cb 
  
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestDocLevelOperations.java
 7834f339 
  
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSubsetQueryOperations.java
 PRE-CREATION 
  sentry-tests/sentry-tests-solr/src/test/resources/log4j.properties d9418167 
  
sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match/conf/schema.xml
 PRE-CREATION 
  
sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match/conf/solrconfig.xml
 PRE-CREATION 
  
sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match_missing_false/conf/schema.xml
 PRE-CREATION 
  
sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match_missing_false/conf/solrconfig.xml
 PRE-CREATION 


Diff: https://reviews.apache.org/r/69975/diff/2/

Changes: https://reviews.apache.org/r/69975/diff/1-2/


Testing
---

Unit tests added for SubsetQueryPlugin
End-to-end tests added under sentry-tests/senty-tests-solr for full 
SolrCloud+Sentry Service using the new QueryDocAuthorizationComponent in 
conjunctive match mode


Thanks,

Tristan Stevens