-
Notifications
You must be signed in to change notification settings - Fork 1k
RANGER-5225: Override policy should take precedence over normal deny policy #588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that override (allow) policies with higher priority correctly take precedence over normal deny policies by expanding test coverage and updating the evaluator logic.
- Added new Hive and HDFS JSON test cases to validate override behavior
- Modified RangerDefaultPolicyEvaluator to apply allow results when override policy priority is higher than a previous deny
- Updated resource test definitions to include override scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
agents-common/src/test/resources/policyengine/test_policyengine_tag_hive.json | Added Hive override test entries under PII tag policy |
agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json | Added HDFS override test case for recursive path /override-resource |
agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java | Changed policy evaluation logic to handle allow overrides based on priority |
agents-common/src/test/resources/policyengine/test_policyengine_tag_hive.json
Show resolved
Hide resolved
agents-common/src/test/resources/policyengine/test_policyengine_tag_hdfs.json
Show resolved
Hide resolved
if (getPolicyPriority() > oldPriority && denyResult != null) { | ||
accessTypeResults.put(accessType, denyResult); | ||
if (getPolicyPriority() > oldPriority) { | ||
if (allowResult != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inner allow check does not include the same context condition (oneRequest.isAccessTypeAny() || RangerAccessRequestUtil.getIsAnyAccessInContext
) used in the initial allow branch; add it to maintain consistent override behavior.
if (allowResult != null) { | |
if (allowResult != null && (oneRequest.isAccessTypeAny() || RangerAccessRequestUtil.getIsAnyAccessInContext(oneRequest.getContext()))) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want this condition check of "is any access" here - condition valid for other access types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The below condition will be false for the scenario being tested because access type any would be false, but we still would want allowResult to override the previous result
&& (oneRequest.isAccessTypeAny() || RangerAccessRequestUtil.getIsAnyAccessInContext(oneRequest.getContext())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mneethiraj
Can you explain why we have different behavior for anyAccess ?
Can we simplify the code to below irrespective of whether access type is any or not ? (I can't understand the case when the policy priority is same as old priority (i.e. why >= is there in the first condition and not >) .
if (getPolicyPriority() > oldPriority) {
if (allowResult != null) {
accessTypeResults.put(accessType, allowResult);
}
else if (denyResult != null) {
accessTypeResults.put(accessType, denyResult);
}
}
What changes were proposed in this pull request?
Steps to reproduce:
Tag an hdfs path /override-path in atlas as PII
Deny PII policy for user userx
Allow override policy for user userx
hdfs dfs -ls /override-path
User gets access denied.
Expected behavior is access allowed
Behavior also reproducible by additional unit tests in policy engine
The PR fixes the bug wherein allow result overrides the previous deny result if priority of allow policy is greater than the priority of the deny policy
How was this patch tested?
Added new unit tests which failed without the proposed changes.
The new unit tests pass after the bug fix.