Re: Review Request 33225: RANGER-400: path-matcher handling of isRecursive updated to fix the issue

[Alok Lal <al...@hortonworks.com>]

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



agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json
<https://reviews.apache.org/r/33225/#comment129983>

    With isRecursive==false shouldn't this be false?



agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json
<https://reviews.apache.org/r/33225/#comment129984>

    I understand why these 2 cases true because that is how eager pattern matcher would work -- but they are counter intutive.
    
    It would good to have a test case added where value is "/public/*test/" to contrast it with this.  Presence of trailing / restricts wildcard only to sub-directory names of /public.



agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json
<https://reviews.apache.org/r/33225/#comment129987>

    It would be good to a input where *test is not a parent of *result, to highlight that * before result can include / too, e.g. "/public/a-test/foo/bar/a-result"



agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json
<https://reviews.apache.org/r/33225/#comment129997>

    Because of the way we treat wildcard there is no way for user to say:
    - allow /public/1test/1result but disallow /public/blah/1test/1result and /public/1test/blah/1result.  Today user can't do that.  I wonder if wildcard * should match anything except the path seperator to make this matching intutive to user.



agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json
<https://reviews.apache.org/r/33225/#comment129992>

    The answer depends on how do the two following patterns differ:
    
    root.default.mycompany*
    root.default.mycompany*.
    
    it should be false if pattern if the second one and it will be, if you were to add a test.  While the behavior is counter intutive, if user is able to control the behavior them we are good.



agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json
<https://reviews.apache.org/r/33225/#comment129993>

    "roots.default" would/should be false


- Alok Lal


On April 15, 2015, 8:43 a.m., Madhan Neethiraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33225/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 8:43 a.m.)
> 
> 
> Review request for ranger, Alok Lal, Don Bosco Durai, Gautam Borad, Abhay Kulkarni, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-400
>     https://issues.apache.org/jira/browse/RANGER-400
> 
> 
> Repository: ranger
> 
> 
> Description
> -------
> 
> Path-matcher handling of isRecursive updated to fix the issue. Added unit tests.
> 
> 
> Diffs
> -----
> 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java b6c98f7 
>   agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerAbstractResourceMatcher.java 646cbc5 
>   agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerDefaultResourceMatcher.java 007fc42 
>   agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerPathResourceMatcher.java fffdbfc 
>   agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerResourceMatcher.java 4a846b5 
>   agents-common/src/test/java/org/apache/ranger/plugin/resourcematcher/TestResourceMatcher.java PRE-CREATION 
>   agents-common/src/test/resources/resourcematcher/test_resourcematcher_default.json PRE-CREATION 
>   agents-common/src/test/resources/resourcematcher/test_resourcematcher_path.json PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/33225/diff/
> 
> 
> Testing
> -------
> 
> All existing and new unit tests pass
> 
> 
> Thanks,
> 
> Madhan Neethiraj
> 
>