RANGER-4762:Prevent duplicate values for a resource while

validating a policy
apache · Apr 2, 2024 · ffda775 · ffda775
1 parent b209e61
commit ffda775
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 22 deletions.
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java b/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
@@ -107,6 +107,7 @@ public enum ValidationErrorCode {
     POLICY_VALIDATION_ERR_NULL_POLICY_ITEM_USER(3053, "policy items user was null"),
     POLICY_VALIDATION_ERR_NULL_POLICY_ITEM_GROUP(3054, "policy items group was null"),
     POLICY_VALIDATION_ERR_NULL_POLICY_ITEM_ROLE(3055, "policy items role was null"),
+    POLICY_VALIDATION_ERR_DUPLICATE_VALUES_FOR_RESOURCE(3056, "Values for the resource={0} contained a duplicate value={1}. Ensure all values for a resource are unique"),
     POLICY_VALIDATION_ERR_INVALID_SERVICE_TYPE(4009," Invalid service type [{0}] provided for service [{1}]"),
 
     // SECURITY_ZONE Validations

diff --git a/...common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java b/...common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
@@ -883,15 +883,7 @@ boolean isValidResourceValues(Map<String, RangerPolicyResource> resourceMap, Lis
 			String name = entry.getKey();
 			RangerPolicyResource policyResource = entry.getValue();
 			if(policyResource != null) {
-				if(CollectionUtils.isNotEmpty(policyResource.getValues())) {
-					Set<String> resources = new HashSet<>(policyResource.getValues());
-					for (String aValue : resources) {
-						if (StringUtils.isBlank(aValue)) {
-							policyResource.getValues().remove(aValue);
-						}
-					}
-				}
-
+				policyResource.getValues().removeIf(StringUtils::isBlank);
 				if(CollectionUtils.isEmpty(policyResource.getValues())){
 					ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_MISSING_RESOURCE_LIST;
 					if(LOG.isDebugEnabled()) {
@@ -906,23 +898,40 @@ boolean isValidResourceValues(Map<String, RangerPolicyResource> resourceMap, Lis
 						.build());
 					valid=false;
 				}
-
-				if (validationRegExMap.containsKey(name) && CollectionUtils.isNotEmpty(policyResource.getValues())) {
-					String regEx = validationRegExMap.get(name);
-					for (String aValue : policyResource.getValues()) {
-						if (!aValue.matches(regEx)) {
-							if (LOG.isDebugEnabled()) {
-								LOG.debug(String.format("Resource failed regex check: value[%s], resource-name[%s], regEx[%s], service-def-name[%s]", aValue, name, regEx, serviceDef.getName()));
-							}
-							ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_INVALID_RESOURCE_VALUE_REGEX;
-							failures.add(new ValidationFailureDetailsBuilder()
+				else{
+					String duplicateValue = getDuplicate(policyResource.getValues());
+					if (!StringUtils.isBlank(duplicateValue)){
+						ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_DUPLICATE_VALUES_FOR_RESOURCE;
+						if (LOG.isDebugEnabled()){
+							LOG.debug(String.format("Duplicate values found for the resource name[%s] value[%s] service-def-name[%s]",name, duplicateValue,serviceDef.getName()));
+						}
+						failures.add(new ValidationFailureDetailsBuilder()
 								.field("resource-values")
 								.subField(name)
 								.isSemanticallyIncorrect()
-								.becauseOf(error.getMessage(aValue, name))
+								.becauseOf(error.getMessage(name, duplicateValue))
 								.errorCode(error.getErrorCode())
-								.build());
-							valid = false;
+								.build()
+						);
+						valid = false;
+					}
+					if (validationRegExMap.containsKey(name)) {
+						String regEx = validationRegExMap.get(name);
+						for (String aValue : policyResource.getValues()) {
+							if (!aValue.matches(regEx)) {
+								if (LOG.isDebugEnabled()) {
+									LOG.debug(String.format("Resource failed regex check: value[%s], resource-name[%s], regEx[%s], service-def-name[%s]", aValue, name, regEx, serviceDef.getName()));
+								}
+								ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_INVALID_RESOURCE_VALUE_REGEX;
+								failures.add(new ValidationFailureDetailsBuilder()
+									.field("resource-values")
+									.subField(name)
+									.isSemanticallyIncorrect()
+									.becauseOf(error.getMessage(aValue, name))
+									.errorCode(error.getErrorCode())
+									.build());
+								valid = false;
+							}
 						}
 					}
 				}
@@ -935,6 +944,21 @@ boolean isValidResourceValues(Map<String, RangerPolicyResource> resourceMap, Lis
 		return valid;
 	}
 
+	private String getDuplicate(List<String> values) {
+		String duplicate = "";
+		if (values!=null) {
+			HashSet<String> set = new HashSet<>();
+			for (String val:values){
+				if (set.contains(val)){
+					duplicate = val;
+					break;
+				}
+				set.add(val);
+			}
+		}
+		return duplicate;
+	}
+
 	boolean isValidPolicyItems(List<RangerPolicyItem> policyItems, List<ValidationFailureDetails> failures, RangerServiceDef serviceDef) {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug(String.format("==> RangerPolicyValidator.isValid(%s, %s, %s)", policyItems, failures, serviceDef));

diff --git a/...on/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java b/...on/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
@@ -140,6 +140,16 @@ public void setUp() throws Exception {
 			{"extra", new String[] { "extra1", "extra2" }, null, null } // spurious "extra" specified
 	};
 
+	private final Object[][] policyResourceMap_bad_duplicate_values = new Object[][]{
+			// resource-name, values, excludes, recursive
+			{ "db", new String[] {"db1", "db2" }, null, true},
+			{ "tbl", new String[] {"tbl1", "tbl1"}, null, null} // invalid: there should not be any duplicates for any resource value
+	};
+	private final Object[][] policyResourceMap_good_duplicate_values = new Object[][]{
+			// resource-name, values, excludes, recursive
+			{ "db", new String[] {"db1", "db2" }, null, true},
+			{ "tbl", new String[] {"tbl1", "tbl2"}, null, null} // valid: there should not be any duplicates for any resource value
+	};
 	private final Object[][] policyResourceMap_bad_multiple_hierarchies = new Object[][] {
 			// resource-name, values, excludes, recursive
 			{  "db", new String[] { "db1", "db2" }, null, true },
@@ -533,6 +543,14 @@ public void test_isValidResourceValues() {
 
 		policyResources = _utils.createPolicyResourceMap(policyResourceMap_good);
 		Assert.assertTrue(_validator.isValidResourceValues(policyResources, _failures, _serviceDef));
+
+		policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad_duplicate_values);
+		Assert.assertFalse(_validator.isValidResourceValues(policyResources, _failures, _serviceDef));
+		_utils.checkFailureForSemanticError(_failures, "resource-values", "tbl");
+
+		policyResources = _utils.createPolicyResourceMap(policyResourceMap_good_duplicate_values);
+		Assert.assertTrue(_validator.isValidResourceValues(policyResources, _failures, _serviceDef));
+
 	}
 
 	@Test