You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Madhan Neethiraj <ma...@apache.org> on 2015/10/07 23:05:30 UTC
Review Request 39103: RANGER-683: access should not be be allowed if
denied by either a tag-based policy or a resource-based policy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39103/
-----------------------------------------------------------
Review request for ranger, Don Bosco Durai, Gautam Borad, Abhay Kulkarni, Selvamohan Neethiraj, and Velmurugan Periasamy.
Bugs: RANGER-683
https://issues.apache.org/jira/browse/RANGER-683
Repository: ranger
Description
-------
Fixed the earlier implementation that allowed access if the user has permission for the tag even though a resource-based policy denied the access
Diffs
-----
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java 5d1140b
agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java 1764b60
agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerOptimizedPolicyEvaluator.java a118466
agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java d7801b9
agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json ed42d5c
Diff: https://reviews.apache.org/r/39103/diff/
Testing
-------
- added unit tests to cover the following combinations:
-------------------------------------------
| Resource-policy | Tag-policy | Result |
|-----------------|------------|-----------|
| Allowed | Allowed | Allowed |
|-----------------|------------|-----------|
| Allowed | Denied | Denied |
|-----------------|------------|-----------|
| Allowed | No policy | Allowed |
|-----------------|------------|-----------|
| Denied | Allowed | Denied |
|-----------------|------------|-----------|
| Denied | Denied | Denied |
|-----------------|------------|-----------|
| Denied | No policy | Denied |
|-----------------|------------|-----------|
| No policy | Allowed | Allowed |
|-----------------|------------|-----------|
| No policy | Denied | Denied |
|-----------------|------------|-----------|
| No policy | No policy | No result |
|-----------------|------------|-----------|
Thanks,
Madhan Neethiraj
Re: Review Request 39103: RANGER-683: access should not be be allowed
if denied by either a tag-based policy or a resource-based policy
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39103/#review101842
-----------------------------------------------------------
Ship it!
Ship It!
- Abhay Kulkarni
On Oct. 7, 2015, 11:43 p.m., Madhan Neethiraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39103/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 11:43 p.m.)
>
>
> Review request for ranger, Don Bosco Durai, Gautam Borad, Abhay Kulkarni, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-683
> https://issues.apache.org/jira/browse/RANGER-683
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fixed the earlier implementation that allowed access if the user has permission for the tag even though a resource-based policy denied the access
>
>
> Diffs
> -----
>
> agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json ed42d5c
>
> Diff: https://reviews.apache.org/r/39103/diff/
>
>
> Testing
> -------
>
> - added unit tests to cover the following combinations:
>
> -------------------------------------------
> | Resource-policy | Tag-policy | Result |
> |-----------------|------------|-----------|
> | Allowed | Allowed | Allowed |
> |-----------------|------------|-----------|
> | Allowed | Denied | Denied |
> |-----------------|------------|-----------|
> | Allowed | No policy | Allowed |
> |-----------------|------------|-----------|
> | Denied | Allowed | Denied |
> |-----------------|------------|-----------|
> | Denied | Denied | Denied |
> |-----------------|------------|-----------|
> | Denied | No policy | Denied |
> |-----------------|------------|-----------|
> | No policy | Allowed | Allowed |
> |-----------------|------------|-----------|
> | No policy | Denied | Denied |
> |-----------------|------------|-----------|
> | No policy | No policy | No result |
> |-----------------|------------|-----------|
>
>
> Thanks,
>
> Madhan Neethiraj
>
>
Re: Review Request 39103: RANGER-683: access should not be be allowed
if denied by either a tag-based policy or a resource-based policy
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39103/
-----------------------------------------------------------
(Updated Oct. 7, 2015, 11:43 p.m.)
Review request for ranger, Don Bosco Durai, Gautam Borad, Abhay Kulkarni, Selvamohan Neethiraj, and Velmurugan Periasamy.
Changes
-------
Updated unit tests per review comments
Bugs: RANGER-683
https://issues.apache.org/jira/browse/RANGER-683
Repository: ranger
Description
-------
Fixed the earlier implementation that allowed access if the user has permission for the tag even though a resource-based policy denied the access
Diffs (updated)
-----
agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json ed42d5c
Diff: https://reviews.apache.org/r/39103/diff/
Testing
-------
- added unit tests to cover the following combinations:
-------------------------------------------
| Resource-policy | Tag-policy | Result |
|-----------------|------------|-----------|
| Allowed | Allowed | Allowed |
|-----------------|------------|-----------|
| Allowed | Denied | Denied |
|-----------------|------------|-----------|
| Allowed | No policy | Allowed |
|-----------------|------------|-----------|
| Denied | Allowed | Denied |
|-----------------|------------|-----------|
| Denied | Denied | Denied |
|-----------------|------------|-----------|
| Denied | No policy | Denied |
|-----------------|------------|-----------|
| No policy | Allowed | Allowed |
|-----------------|------------|-----------|
| No policy | Denied | Denied |
|-----------------|------------|-----------|
| No policy | No policy | No result |
|-----------------|------------|-----------|
Thanks,
Madhan Neethiraj
Re: Review Request 39103: RANGER-683: access should not be be allowed
if denied by either a tag-based policy or a resource-based policy
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Oct. 7, 2015, 11:24 p.m., Abhay Kulkarni wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java, line 333
> > <https://reviews.apache.org/r/39103/diff/1/?file=1092530#file1092530line333>
> >
> > If isAuditedDetermined is true at this point, then the auditCache is bypassed, i.e. cacheing of the result of audit will not be done. Will we ever need to cache this as an optimization?
I thought about this and decided not to cache the isAudited derived from tag into resource cache. Doing this could cause incorrect auditing if tag was removed from the resource.
> On Oct. 7, 2015, 11:24 p.m., Abhay Kulkarni wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java, line 433
> > <https://reviews.apache.org/r/39103/diff/1/?file=1092530#file1092530line433>
> >
> > getIsAudited() and setIsAudited() is generally not used anywhere in the code. Any specific reason for using those APIs here?
To handle the following cases:
- if audit was not enabled for the current tag, but it was enabled for earlier tag, we should not overwrite with false
- if audit was enabled for the current tag, but not yet for earlier tags, this would set audit to true
Using this condition make it simpler to handle both these cases.
> On Oct. 7, 2015, 11:24 p.m., Abhay Kulkarni wrote:
> > agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java, line 26
> > <https://reviews.apache.org/r/39103/diff/1/?file=1092533#file1092533line26>
> >
> > Can these headers to kept around? I have some commented out code in this file which I sometimes use to set up configuration properties. That code requires these headers.
I was trying to keep the source clean, such that Eclipse wouldn't show yellow icon for the source files. When you uncomment this block, the IDE should automatically add necessary imports (Eclipse does..).
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39103/#review101827
-----------------------------------------------------------
On Oct. 7, 2015, 9:05 p.m., Madhan Neethiraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39103/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 9:05 p.m.)
>
>
> Review request for ranger, Don Bosco Durai, Gautam Borad, Abhay Kulkarni, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-683
> https://issues.apache.org/jira/browse/RANGER-683
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fixed the earlier implementation that allowed access if the user has permission for the tag even though a resource-based policy denied the access
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java 5d1140b
> agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java 1764b60
> agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerOptimizedPolicyEvaluator.java a118466
> agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java d7801b9
> agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json ed42d5c
>
> Diff: https://reviews.apache.org/r/39103/diff/
>
>
> Testing
> -------
>
> - added unit tests to cover the following combinations:
>
> -------------------------------------------
> | Resource-policy | Tag-policy | Result |
> |-----------------|------------|-----------|
> | Allowed | Allowed | Allowed |
> |-----------------|------------|-----------|
> | Allowed | Denied | Denied |
> |-----------------|------------|-----------|
> | Allowed | No policy | Allowed |
> |-----------------|------------|-----------|
> | Denied | Allowed | Denied |
> |-----------------|------------|-----------|
> | Denied | Denied | Denied |
> |-----------------|------------|-----------|
> | Denied | No policy | Denied |
> |-----------------|------------|-----------|
> | No policy | Allowed | Allowed |
> |-----------------|------------|-----------|
> | No policy | Denied | Denied |
> |-----------------|------------|-----------|
> | No policy | No policy | No result |
> |-----------------|------------|-----------|
>
>
> Thanks,
>
> Madhan Neethiraj
>
>
Re: Review Request 39103: RANGER-683: access should not be be allowed
if denied by either a tag-based policy or a resource-based policy
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39103/#review101827
-----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java (line 333)
<https://reviews.apache.org/r/39103/#comment159323>
If isAuditedDetermined is true at this point, then the auditCache is bypassed, i.e. cacheing of the result of audit will not be done. Will we ever need to cache this as an optimization?
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java (line 419)
<https://reviews.apache.org/r/39103/#comment159324>
getIsAudited() and setIsAudited() is generally not used anywhere in the code. Any specific reason for using those APIs here?
agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java
<https://reviews.apache.org/r/39103/#comment159326>
Can these headers to kept around? I have some commented out code in this file which I sometimes use to set up configuration properties. That code requires these headers.
agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json (line 121)
<https://reviews.apache.org/r/39103/#comment159336>
td->ta
agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json (line 138)
<https://reviews.apache.org/r/39103/#comment159337>
remove enforce_expiry
- Abhay Kulkarni
On Oct. 7, 2015, 9:05 p.m., Madhan Neethiraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39103/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 9:05 p.m.)
>
>
> Review request for ranger, Don Bosco Durai, Gautam Borad, Abhay Kulkarni, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-683
> https://issues.apache.org/jira/browse/RANGER-683
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fixed the earlier implementation that allowed access if the user has permission for the tag even though a resource-based policy denied the access
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java 5d1140b
> agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java 1764b60
> agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerOptimizedPolicyEvaluator.java a118466
> agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java d7801b9
> agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json ed42d5c
>
> Diff: https://reviews.apache.org/r/39103/diff/
>
>
> Testing
> -------
>
> - added unit tests to cover the following combinations:
>
> -------------------------------------------
> | Resource-policy | Tag-policy | Result |
> |-----------------|------------|-----------|
> | Allowed | Allowed | Allowed |
> |-----------------|------------|-----------|
> | Allowed | Denied | Denied |
> |-----------------|------------|-----------|
> | Allowed | No policy | Allowed |
> |-----------------|------------|-----------|
> | Denied | Allowed | Denied |
> |-----------------|------------|-----------|
> | Denied | Denied | Denied |
> |-----------------|------------|-----------|
> | Denied | No policy | Denied |
> |-----------------|------------|-----------|
> | No policy | Allowed | Allowed |
> |-----------------|------------|-----------|
> | No policy | Denied | Denied |
> |-----------------|------------|-----------|
> | No policy | No policy | No result |
> |-----------------|------------|-----------|
>
>
> Thanks,
>
> Madhan Neethiraj
>
>