Skip to content

Commit

Permalink
RANGER-5115: fix for ConcurrentModificationException during policy ev…
Browse files Browse the repository at this point in the history 
…aluation (#517)
  • Loading branch information
@mneethiraj
mneethiraj authored Jan 26, 2025
1 parent a828d3f commit aab28a5
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 47 deletions.
65 changes: 35 additions & 30 deletions ...main/java/org/apache/ranger/plugin/policyevaluator/RangerAbstractPolicyItemEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,51 +106,56 @@ protected boolean getConditionsDisabledOption() {
}

protected RangerPolicyItem computeWithImpliedGrants() {
final RangerPolicyItem ret;
RangerPolicyItem ret = withImpliedGrants;

if (withImpliedGrants == null) {
if (CollectionUtils.isEmpty(policyItem.getAccesses())) {
ret = policyItem;
} else {
// Compute implied-accesses
Map<String, Collection<String>> impliedAccessGrants = options.getServiceDefHelper().getImpliedAccessGrants();
if (ret == null) {
synchronized (this) {
ret = withImpliedGrants;

if (impliedAccessGrants != null && !impliedAccessGrants.isEmpty()) {
ret = new RangerPolicyItem(policyItem);
if (ret == null) {
ret = policyItem;

// Only one round of 'expansion' is done; multi-level impliedGrants (like shown below) are not handled for now
// multi-level impliedGrants: given admin=>write; write=>read: must imply admin=>read,write
for (Map.Entry<String, Collection<String>> e : impliedAccessGrants.entrySet()) {
String implyingAccessType = e.getKey();
Collection<String> impliedGrants = e.getValue();
if (CollectionUtils.isNotEmpty(policyItem.getAccesses())) {
// Compute implied-accesses
Map<String, Collection<String>> impliedAccessGrants = options.getServiceDefHelper().getImpliedAccessGrants();

RangerPolicy.RangerPolicyItemAccess access = RangerDefaultPolicyEvaluator.getAccess(ret, implyingAccessType);
if (impliedAccessGrants != null && !impliedAccessGrants.isEmpty()) {
ret = new RangerPolicyItem(policyItem);

if (access == null) {
continue;
}
// Only one round of 'expansion' is done; multi-level impliedGrants (like shown below) are not handled for now
// multi-level impliedGrants: given admin=>write; write=>read: must imply admin=>read,write
for (Map.Entry<String, Collection<String>> e : impliedAccessGrants.entrySet()) {
String implyingAccessType = e.getKey();
Collection<String> impliedGrants = e.getValue();

for (String impliedGrant : impliedGrants) {
RangerPolicy.RangerPolicyItemAccess impliedAccess = RangerDefaultPolicyEvaluator.getAccess(ret, impliedGrant);
RangerPolicy.RangerPolicyItemAccess access = RangerDefaultPolicyEvaluator.getAccess(ret, implyingAccessType);

if (access == null) {
continue;
}

if (impliedAccess == null) {
impliedAccess = new RangerPolicy.RangerPolicyItemAccess(impliedGrant, access.getIsAllowed());
for (String impliedGrant : impliedGrants) {
RangerPolicy.RangerPolicyItemAccess impliedAccess = RangerDefaultPolicyEvaluator.getAccess(ret, impliedGrant);

ret.addAccess(impliedAccess);
} else {
if (!impliedAccess.getIsAllowed()) {
impliedAccess.setIsAllowed(access.getIsAllowed());
if (impliedAccess == null) {
impliedAccess = new RangerPolicy.RangerPolicyItemAccess(impliedGrant, access.getIsAllowed());

ret.addAccess(impliedAccess);
} else {
if (!impliedAccess.getIsAllowed()) {
impliedAccess.setIsAllowed(access.getIsAllowed());
}
}
}
}
}
}
} else {
ret = policyItem;

withImpliedGrants = ret;
}
}
} else {
ret = withImpliedGrants;
}

return ret;
}

Expand Down
8 changes: 2 additions & 6 deletions .../main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyItemEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,7 @@ public boolean isMatch(RangerAccessRequest request) {
ret = true;
}
} else {
if (withImpliedGrants == null) {
withImpliedGrants = computeWithImpliedGrants();
}
RangerPolicyItem withImpliedGrants = computeWithImpliedGrants();

if (withImpliedGrants != null && CollectionUtils.isNotEmpty(withImpliedGrants.getAccesses())) {
boolean isAccessTypeMatched = false;
Expand Down Expand Up @@ -155,9 +153,7 @@ public boolean matchAccessType(String accessType) {
if (isAdminAccess) {
ret = policyItem.getDelegateAdmin();
} else {
if (withImpliedGrants == null) {
withImpliedGrants = computeWithImpliedGrants();
}
RangerPolicyItem withImpliedGrants = computeWithImpliedGrants();

if (CollectionUtils.isNotEmpty(withImpliedGrants.getAccesses())) {
boolean isAnyAccess = StringUtils.equals(accessType, RangerPolicyEngine.ANY_ACCESS);
Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/org/apache/ranger/plugin/policyevaluator/RangerOptimizedPolicyEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ private void preprocessPolicyItems(List<? extends RangerPolicy.RangerPolicyItem>

for (RangerPolicy.RangerPolicyItemAccess policyItemAccess : policyItemAccesses) {
if (policyItemAccess.getIsAllowed()) {
add(accessPerms, policyItemAccess.getType());
accessPerms = add(accessPerms, policyItemAccess.getType());
}
}

Expand Down
8 changes: 3 additions & 5 deletions agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyACLs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.lang.reflect.Type;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -119,10 +120,7 @@ private void runTests(InputStreamReader reader, String testName) {
RangerPluginContext pluginContext = new RangerPluginContext(new RangerPluginConfig(serviceType, null, "test-policy-acls", "cl1", "on-prem", policyEngineOptions));
RangerPolicyEngine policyEngine = new RangerPolicyEngineImpl(testCase.servicePolicies, pluginContext, null);

for (PolicyACLsTests.TestCase.OneTest oneTest : testCase.tests) {
if (oneTest == null) {
continue;
}
testCase.tests.parallelStream().filter(Objects::nonNull).forEach(oneTest -> {
RangerAccessRequestImpl request = new RangerAccessRequestImpl(oneTest.resource, RangerPolicyEngine.ANY_ACCESS, null, null, null);

request.setResourceMatchingScope(oneTest.resourceMatchingScope);
Expand Down Expand Up @@ -287,7 +285,7 @@ private void runTests(InputStreamReader reader, String testName) {
assertTrue("getResourceACLs() failed! " + testCase.name + ":" + oneTest.name + " - roleACLsMatched", roleACLsMatched);
assertTrue("getResourceACLs() failed! " + testCase.name + ":" + oneTest.name + " - rowFiltersMatched", rowFiltersMatched);
assertTrue("getResourceACLs() failed! " + testCase.name + ":" + oneTest.name + " - dataMaskingMatched", dataMaskingMatched);
}
});
}
}

Expand Down
8 changes: 3 additions & 5 deletions agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,10 +817,8 @@ private void runTests(InputStreamReader reader, String testName) {
}

private void runTestCaseTests(RangerPolicyEngine policyEngine, RangerServiceDef serviceDef, String testName, List<TestData> tests) {
RangerAccessRequest request = null;

for (TestData test : tests) {
request = test.request;
tests.parallelStream().forEach(test -> {
RangerAccessRequest request = test.request;

if (request.getContext().containsKey(RangerAccessRequestUtil.KEY_CONTEXT_TAGS) ||
request.getContext().containsKey(RangerAccessRequestUtil.KEY_CONTEXT_REQUESTED_RESOURCES)) {
Expand Down Expand Up @@ -949,7 +947,7 @@ private void runTestCaseTests(RangerPolicyEngine policyEngine, RangerServiceDef
assertEquals("deniedUsers mismatched! - " + test.name, expected.getDeniedUsers(), result.getDeniedUsers());
assertEquals("deniedGroups mismatched! - " + test.name, expected.getDeniedGroups(), result.getDeniedGroups());
}
}
});
}

private void setPluginConfig(RangerPluginConfig conf, String suffix, Set<String> value) {
Expand Down

0 comments on commit aab28a5

Please sign in to comment.