RANGER-5115: fix for ConcurrentModificationException during policy ev…

…aluation (cherry picked from commit aab28a5))
apache · Jan 26, 2025 · ea0f5df · ea0f5df
1 parent 0b5602c
commit ea0f5df
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 48 deletions.
diff --git a/...main/java/org/apache/ranger/plugin/policyevaluator/RangerAbstractPolicyItemEvaluator.java b/...main/java/org/apache/ranger/plugin/policyevaluator/RangerAbstractPolicyItemEvaluator.java
@@ -109,52 +109,56 @@ public RangerPolicyItem getWithImpliedGrants() {
 	}
 
 	protected RangerPolicyItem computeWithImpliedGrants() {
+		RangerPolicyItem ret = withImpliedGrants;
 
-		final RangerPolicyItem ret;
+		if (ret == null) {
+			synchronized (this) {
+				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) {
+					ret = policyItem;
 
-				if (impliedAccessGrants != null && !impliedAccessGrants.isEmpty()) {
-					ret = new RangerPolicyItem(policyItem);
+					if (CollectionUtils.isNotEmpty(policyItem.getAccesses())) {
+						// Compute implied-accesses
+						Map<String, Collection<String>> impliedAccessGrants = options.getServiceDefHelper().getImpliedAccessGrants();
 
-					// 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 (impliedAccessGrants != null && !impliedAccessGrants.isEmpty()) {
+							ret = new RangerPolicyItem(policyItem);
 
-						RangerPolicy.RangerPolicyItemAccess access = RangerDefaultPolicyEvaluator.getAccess(ret, implyingAccessType);
+							// 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 (access == null) {
-							continue;
-						}
+								RangerPolicy.RangerPolicyItemAccess access = RangerDefaultPolicyEvaluator.getAccess(ret, implyingAccessType);
+
+								if (access == null) {
+									continue;
+								}
 
-						for (String impliedGrant : impliedGrants) {
-							RangerPolicy.RangerPolicyItemAccess impliedAccess = RangerDefaultPolicyEvaluator.getAccess(ret, impliedGrant);
+								for (String impliedGrant : impliedGrants) {
+									RangerPolicy.RangerPolicyItemAccess impliedAccess = RangerDefaultPolicyEvaluator.getAccess(ret, impliedGrant);
 
-							if (impliedAccess == null) {
-								impliedAccess = new RangerPolicy.RangerPolicyItemAccess(impliedGrant, access.getIsAllowed());
+									if (impliedAccess == null) {
+										impliedAccess = new RangerPolicy.RangerPolicyItemAccess(impliedGrant, access.getIsAllowed());
 
-								ret.addAccess(impliedAccess);
-							} else {
-								if (!impliedAccess.getIsAllowed()) {
-									impliedAccess.setIsAllowed(access.getIsAllowed());
+										ret.addAccess(impliedAccess);
+									} else {
+										if (!impliedAccess.getIsAllowed()) {
+											impliedAccess.setIsAllowed(access.getIsAllowed());
+										}
+									}
 								}
 							}
 						}
 					}
-				} else {
-					ret = policyItem;
+
+					withImpliedGrants = ret;
 				}
 			}
-		} else {
-			ret = withImpliedGrants;
 		}
+
 		return ret;
 	}
 

diff --git a/.../main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyItemEvaluator.java b/.../main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyItemEvaluator.java
@@ -92,9 +92,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;
@@ -198,9 +196,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);

diff --git a/...rc/main/java/org/apache/ranger/plugin/policyevaluator/RangerOptimizedPolicyEvaluator.java b/...rc/main/java/org/apache/ranger/plugin/policyevaluator/RangerOptimizedPolicyEvaluator.java
@@ -359,7 +359,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());
 	                }
 	            }
 

diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyACLs.java b/agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyACLs.java
@@ -26,6 +26,7 @@
 import java.lang.reflect.Type;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 
 import com.google.gson.JsonDeserializationContext;
@@ -121,10 +122,7 @@ private void runTests(InputStreamReader reader, String testName) throws Exceptio
 			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);
@@ -288,7 +286,7 @@ private void runTests(InputStreamReader reader, String testName) throws Exceptio
 				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);
-			}
+			});
 		}
 	}
 

diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java b/agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java
@@ -645,10 +645,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)) {
@@ -779,8 +777,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) {