RANGER-4472: getResourceACLs() handling of tags associated with resou…

…rce and its desencent - #2

agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagEnricher.java

@@ -718,7 +718,7 @@ private Set<RangerTagForEval> findMatchingTags(final RangerAccessRequest request
 					} else if (request.getResourceMatchingScope() == RangerAccessRequest.ResourceMatchingScope.SELF_OR_DESCENDANTS) {
 						isMatched = matchType != RangerPolicyResourceMatcher.MatchType.NONE;
 					} else {
-						isMatched = matchType == RangerPolicyResourceMatcher.MatchType.SELF || matchType == RangerPolicyResourceMatcher.MatchType.ANCESTOR;
+						isMatched = matchType == RangerPolicyResourceMatcher.MatchType.SELF || matchType == RangerPolicyResourceMatcher.MatchType.SELF_AND_ALL_DESCENDANTS || matchType == RangerPolicyResourceMatcher.MatchType.ANCESTOR;
 					}
 
 					if (isMatched) {

agents-common/src/main/java/org/apache/ranger/plugin/policyengine/PolicyEvaluatorForTag.java

@@ -21,13 +21,16 @@
 
 import org.apache.ranger.plugin.contextenricher.RangerTagForEval;
 import org.apache.ranger.plugin.policyevaluator.RangerPolicyEvaluator;
+import org.apache.ranger.plugin.policyresourcematcher.RangerPolicyResourceMatcher;
 
 import java.io.Serializable;
 import java.util.Comparator;
 
+
 public class PolicyEvaluatorForTag {
 	public static final Comparator<PolicyEvaluatorForTag> EVAL_ORDER_COMPARATOR = new PolicyEvalOrderComparator();
 	public static final Comparator<PolicyEvaluatorForTag> NAME_COMPARATOR       = new PolicyNameComparator();
+	public static final Comparator<PolicyEvaluatorForTag> MATCH_TYPE_COMPARATOR = new MatchTypeComparator();
 
 	private final RangerPolicyEvaluator evaluator;
 	private final RangerTagForEval      tag;
@@ -58,4 +61,17 @@ public int compare(PolicyEvaluatorForTag me, PolicyEvaluatorForTag other) {
 			return RangerPolicyEvaluator.EVAL_ORDER_COMPARATOR.compare(me.getEvaluator(), other.getEvaluator());
 		}
 	}
+
+	static class MatchTypeComparator implements Comparator<PolicyEvaluatorForTag>, Serializable {
+		@Override
+		public int compare(PolicyEvaluatorForTag me, PolicyEvaluatorForTag other) {
+			int ret = RangerPolicyResourceMatcher.MATCH_TYPE_COMPARATOR.compare(me.getTag().getMatchType(), other.getTag().getMatchType());
+
+			if (ret == 0) {
+				ret = RangerPolicyEvaluator.NAME_COMPARATOR.compare(me.getEvaluator(), other.getEvaluator());
+			}
+
+			return ret;
+		}
+	}
 }

agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java

@@ -59,6 +59,7 @@
 import java.util.Map;
 import java.util.Set;
 
+import static org.apache.ranger.plugin.policyengine.PolicyEvaluatorForTag.MATCH_TYPE_COMPARATOR;
 import static org.apache.ranger.plugin.policyevaluator.RangerPolicyEvaluator.ACCESS_CONDITIONAL;
 
 public class RangerPolicyEngineImpl implements RangerPolicyEngine {
@@ -333,7 +334,7 @@ public RangerResourceACLs getResourceACLs(RangerAccessRequest request, Integer r
 							isMatched = isMatch(matchType, request.getResourceMatchingScope());
 
 							if (isMatched) {
-								isConditionalMatch = false;
+								isConditionalMatch = evaluator.getPolicyConditionsCount() > 0;
 
 								break;
 							} else if (matcher.getNeedsDynamicEval() && !isConditionalMatch) {
@@ -990,6 +991,7 @@ private void getResourceACLEvaluatorsForZone(RangerAccessRequest request, String
 			List<PolicyEvaluatorForTag> tagPolicyEvaluators = policyEngine.getTagPolicyRepository() == null ? null : policyEngine.getTagPolicyRepository().getLikelyMatchPolicyEvaluators(request, tags, policyType, null);
 
 			if (CollectionUtils.isNotEmpty(tagPolicyEvaluators)) {
+				tagPolicyEvaluators.sort(MATCH_TYPE_COMPARATOR);
 
 				final boolean useTagPoliciesFromDefaultZone = !policyEngine.isResourceZoneAssociatedWithTagService(zoneName);
 
@@ -1017,9 +1019,11 @@ private void getResourceACLEvaluatorsForZone(RangerAccessRequest request, String
 
 					RangerTagForEval tag = tagEvaluator.getTag();
 
-					allEvaluators.add(evaluator);
-
-					updateMatchTypeForTagEvaluator(tagEvaluator, tagMatchTypeMap);
+					// avoid an evaluator making into the list multiple times when the same tag is associated with the resource multiple times
+					// highest precedence matchType will be recorded in tagMatchTypeMap, since tagPolicyEvaluators is sorted by matchType
+					if (tagMatchTypeMap.putIfAbsent(evaluator.getPolicyId(), tag.getMatchType()) == null) {
+						allEvaluators.add(evaluator);
+					}
 
 					if (CollectionUtils.isNotEmpty(tag.getValidityPeriods())) {
 						policyIdForTemporalTags.add(evaluator.getPolicyId());
@@ -1033,38 +1037,6 @@ private void getResourceACLEvaluatorsForZone(RangerAccessRequest request, String
 		}
 	}
 
-	// Multiple tags can be mapped to a tag-based policy. In such cases, use the match-type of the tag having the highest precedence
-	// Consider following tags:
-	//  table  table1      has tag SENSITIVE(level=normal)
-	//  column table1.col1 has tag SENSITIVE(level=high)
-	//
-	// Following 2 tags will be matched for table1:
-	//  SENSITIVE(level=normal) with MatchType.SELF
-	//  SENSITIVE(level=high)   with MatchType.DESCENDANT
-	//
-	// Following 2 tags will be matched for table1.col1:
-	//  SENSITIVE(level=high)   with MatchType.SELF
-	//  SENSITIVE(level=normal) with MatchType.SELF_AND_ALL_DESCENDANTS
-	//
-	// In these cases, matchType SELF should be used for policy evaluation
-	//
-	private void updateMatchTypeForTagEvaluator(PolicyEvaluatorForTag tag, Map<Long, MatchType> tagMatchTypeMap) {
-		Long      evaluatorId = tag.getEvaluator().getPolicyId();
-		MatchType existing    = tagMatchTypeMap.get(evaluatorId);
-
-		if (existing != MatchType.SELF) {
-			MatchType matchType = tag.getTag().getMatchType();
-
-			if (existing == null || existing == MatchType.NONE || matchType == MatchType.SELF || matchType == MatchType.SELF_AND_ALL_DESCENDANTS) {
-				tagMatchTypeMap.put(evaluatorId, matchType);
-			} else if (matchType == MatchType.ANCESTOR) {
-				if (existing == MatchType.DESCENDANT) {
-					tagMatchTypeMap.put(evaluatorId, MatchType.SELF_AND_ALL_DESCENDANTS);
-				}
-			}
-		}
-	}
-
 	private void getResourceAccessInfoForZone(RangerAccessRequest request, RangerResourceAccessInfo ret, String zoneName) {
 		final RangerPolicyRepository matchedRepository = policyEngine.getRepositoryForZone(zoneName);
 

agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java

@@ -632,7 +632,7 @@ List<PolicyEvaluatorForTag> getLikelyMatchPolicyEvaluators(RangerAccessRequest r
                         Collections.sort(ret, PolicyEvaluatorForTag.EVAL_ORDER_COMPARATOR);
                         break;
                     case RangerPolicy.POLICY_TYPE_DATAMASK:
-                        Collections.sort(ret, PolicyEvaluatorForTag.NAME_COMPARATOR);
+                        Collections.sort(ret, PolicyEvaluatorForTag.MATCH_TYPE_COMPARATOR);
                         break;
                     case RangerPolicy.POLICY_TYPE_ROWFILTER:
                         Collections.sort(ret, PolicyEvaluatorForTag.NAME_COMPARATOR);
@@ -931,14 +931,13 @@ private void init(RangerPolicyEngineOptions options) {
             }
         }
         LOG.info("This policy engine contains " + (policyEvaluators.size()+dataMaskPolicyEvaluators.size()+rowFilterPolicyEvaluators.size()) + " policy evaluators");
-        RangerPolicyEvaluator.PolicyEvalOrderComparator comparator = new RangerPolicyEvaluator.PolicyEvalOrderComparator();
-        Collections.sort(policyEvaluators, comparator);
+        Collections.sort(policyEvaluators, RangerPolicyEvaluator.EVAL_ORDER_COMPARATOR);
         this.policyEvaluators = policyEvaluators;
 
-        Collections.sort(dataMaskPolicyEvaluators, comparator);
+        Collections.sort(dataMaskPolicyEvaluators, RangerPolicyEvaluator.NAME_COMPARATOR);
         this.dataMaskPolicyEvaluators = dataMaskPolicyEvaluators;
 
-        Collections.sort(rowFilterPolicyEvaluators, comparator);
+        Collections.sort(rowFilterPolicyEvaluators, RangerPolicyEvaluator.NAME_COMPARATOR);
         this.rowFilterPolicyEvaluators = rowFilterPolicyEvaluators;
 
         this.policyEvaluatorsMap = createPolicyEvaluatorsMap();
@@ -1175,7 +1174,7 @@ private List<RangerPolicyEvaluator> getReorderedPolicyEvaluators(List<RangerPoli
 
         if (CollectionUtils.isNotEmpty(evaluators)) {
             ret = new ArrayList<>(evaluators);
-            Collections.sort(ret, new RangerPolicyEvaluator.PolicyEvalOrderComparator());
+            Collections.sort(ret, RangerPolicyEvaluator.EVAL_ORDER_COMPARATOR);
         }
 
         return ret;

agents-common/src/main/java/org/apache/ranger/plugin/policyresourcematcher/RangerPolicyResourceMatcher.java

@@ -19,6 +19,7 @@
 
 package org.apache.ranger.plugin.policyresourcematcher;
 
+import java.util.Comparator;
 import java.util.Map;
 
 import org.apache.ranger.plugin.model.RangerPolicy;
@@ -27,12 +28,15 @@
 import org.apache.ranger.plugin.model.validation.RangerServiceDefHelper;
 import org.apache.ranger.plugin.policyengine.RangerAccessRequest.ResourceElementMatchingScope;
 import org.apache.ranger.plugin.policyengine.RangerAccessResource;
+import org.apache.ranger.plugin.policyevaluator.RangerPolicyEvaluator;
 import org.apache.ranger.plugin.resourcematcher.RangerResourceMatcher;
 
 public interface RangerPolicyResourceMatcher {
 	enum MatchScope { SELF, SELF_OR_DESCENDANT, SELF_OR_ANCESTOR, DESCENDANT, ANCESTOR, ANY, SELF_AND_ALL_DESCENDANTS}
 	enum MatchType { NONE, SELF, DESCENDANT, ANCESTOR, SELF_AND_ALL_DESCENDANTS}
 
+	Comparator<MatchType> MATCH_TYPE_COMPARATOR = new MatchTypeComparator();
+
 	void init();
 
 	void setServiceDef(RangerServiceDef serviceDef);
@@ -76,4 +80,45 @@ enum MatchType { NONE, SELF, DESCENDANT, ANCESTOR, SELF_AND_ALL_DESCENDANTS}
 	boolean getNeedsDynamicEval();
 
 	StringBuilder toString(StringBuilder sb);
+
+	// order: SELF, SELF_AND_ALL_DESCENDANTS, ANCESTOR, DESCENDANT, NONE
+	class MatchTypeComparator implements Comparator<MatchType> {
+		@Override
+		public int compare(MatchType o1, MatchType o2) {
+			final int ret;
+
+			if (o1 == o2) {
+				ret = 0;
+			} else if (o1 == null) {
+				return 1;
+			} else if (o2 == null) {
+				return -1;
+			} else {
+				switch (o1) {
+					case SELF:
+						ret = -1;
+					break;
+
+					case SELF_AND_ALL_DESCENDANTS:
+						ret = o2 == MatchType.SELF ? 1 : -1;
+					break;
+
+					case ANCESTOR:
+						ret = (o2 == MatchType.SELF || o2 == MatchType.SELF_AND_ALL_DESCENDANTS) ? 1 : -1;
+					break;
+
+					case DESCENDANT:
+						ret = o2 == MatchType.NONE ? -1 : 1;
+					break;
+
+					case NONE:
+					default:
+						ret = 1;
+					break;
+				}
+			}
+
+			return ret;
+		}
+	}
 }

agents-common/src/test/java/org/apache/ranger/plugin/contextenricher/TestTagEnricher.java

@@ -31,6 +31,8 @@
 import org.apache.ranger.plugin.model.RangerTag;
 import org.apache.ranger.plugin.model.RangerTagDef;
 import org.apache.ranger.plugin.policyengine.*;
+import org.apache.ranger.plugin.policyresourcematcher.RangerPolicyResourceMatcher;
+import org.apache.ranger.plugin.policyresourcematcher.RangerPolicyResourceMatcher.MatchType;
 import org.apache.ranger.plugin.util.RangerAccessRequestUtil;
 import org.apache.ranger.plugin.util.ServiceTags;
 import org.junit.AfterClass;
@@ -64,6 +66,27 @@ public static void setUpBeforeClass() throws Exception {
     public static void tearDownAfterClass() throws Exception {
     }
 
+    @Test
+    public void testRangerTagsForEvalSort() {
+        List<MatchType> matchTypes = new ArrayList<>();
+
+        matchTypes.add(null);
+        matchTypes.add(MatchType.NONE);
+        matchTypes.add(MatchType.DESCENDANT);
+        matchTypes.add(MatchType.ANCESTOR);
+        matchTypes.add(MatchType.SELF_AND_ALL_DESCENDANTS);
+        matchTypes.add(MatchType.SELF);
+
+        matchTypes.sort(RangerPolicyResourceMatcher.MATCH_TYPE_COMPARATOR);
+
+        assertEquals(matchTypes.get(0), MatchType.SELF);
+        assertEquals(matchTypes.get(1), MatchType.SELF_AND_ALL_DESCENDANTS);
+        assertEquals(matchTypes.get(2), MatchType.ANCESTOR);
+        assertEquals(matchTypes.get(3), MatchType.DESCENDANT);
+        assertEquals(matchTypes.get(4), MatchType.NONE);
+        assertEquals(matchTypes.get(5), null);
+    }
+
     @Test
     public void testTagEnricher_hive() {
         String[] hiveTestResourceFiles = { "/contextenricher/test_tagenricher_hive.json" };

agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java

@@ -496,6 +496,13 @@ public void testAnyResourceAccess_S3() throws Exception {
 		runTestsFromResourceFiles(resourceFiles);
 	}
 
+	@Test
+	public void testResourceHierarchyTags() throws Exception {
+		String[] resourceFiles = {"/policyengine/test_policyengine_resource_hierarchy_tags.json"};
+
+		runTestsFromResourceFiles(resourceFiles);
+	}
+
 	private void runTestsFromResourceFiles(String[] resourceNames) {
 		for(String resourceName : resourceNames) {
 			InputStream inStream = this.getClass().getResourceAsStream(resourceName);

agents-common/src/test/resources/policyengine/resource_hierarchy_tags.json

@@ -3,83 +3,40 @@
     "tagModel":    "resource_private",
     "serviceName": "cl1_hive",
     "tagDefinitions": {
-      "1": {
-        "id":            1,
-        "guid":          "tag-def-1",
-        "name":          "SENSITIVE",
-        "attributeDefs": [ { "name": "level", "type": "string" } ]
-      }
+      "1": { "id": 1, "guid": "tag-def-1", "name": "SENSITIVE", "attributeDefs": [ { "name": "level", "type": "string" } ] },
+      "2": { "id": 2, "guid": "tag-def-2", "name": "ORDER" },
+      "3": { "id": 3, "guid": "tag-def-3", "name": "CUSTOMER" },
+      "4": { "id": 4, "guid": "tag-def-4", "name": "ADDRESS" }
     },
     "tags": {
-      "1": {
-        "id":         1,
-        "guid":       "tag-1",
-        "type":       "SENSITIVE",
-        "attributes": { "level": "normal" }
-      },
-      "2": {
-        "id":         2,
-        "guid":       "tag-2",
-        "type":       "SENSITIVE",
-        "attributes": { "level": "high" }
-      },
-      "3": {
-        "id":         3,
-        "guid":       "tag-3",
-        "type":       "SENSITIVE",
-        "attributes": { "level": "top" }
-      },
-      "4": {
-        "id":         4,
-        "guid":       "tag-4",
-        "type":       "SENSITIVE",
-        "attributes": { "level": "top" }
-      },
-      "5": {
-        "id":         5,
-        "guid":       "tag-5",
-        "type":       "SENSITIVE",
-        "attributes": { "level": "top" }
-      }
+      "1": { "id": 1, "guid": "tag-1", "type": "SENSITIVE", "attributes": { "level": "normal" } },
+      "2": { "id": 2, "guid": "tag-2", "type": "SENSITIVE", "attributes": { "level": "high" } },
+      "3": { "id": 3, "guid": "tag-3", "type": "SENSITIVE", "attributes": { "level": "top" } },
+      "4": { "id": 4, "guid": "tag-4", "type": "SENSITIVE", "attributes": { "level": "top" } },
+      "5": { "id": 5, "guid": "tag-5", "type": "SENSITIVE", "attributes": { "level": "top" } },
+      "6": { "id": 6, "guid": "tag-6", "type": "ORDER" },
+      "7": { "id": 7, "guid": "tag-7", "type": "CUSTOMER" },
+      "8": { "id": 8, "guid": "tag-8", "type": "ADDRESS" }
     },
     "serviceResources": [
-      {
-        "id":               1,
-        "guid":             "resource-1",
-        "serviceName":      "cl1_hive",
-        "resourceElements": { "database": { "values": [ "db1" ] },  "table": { "values": [ "tbl1" ] } }
-      },
-      {
-        "id":               2,
-        "guid":             "resource-2",
-        "serviceName":      "cl1_hive",
-        "resourceElements": { "database": { "values": [ "db1" ] },  "table": { "values": [ "tbl1" ] },  "column": { "values": [ "SSN" ] } }
-      },
-      {
-        "id":               3,
-        "guid":             "resource-3",
-        "serviceName":      "cl1_hive",
-        "resourceElements": { "database": { "values": [ "db1" ] },  "table": { "values": [ "tbl1" ] },  "column": { "values": [ "Age" ] } }
-      },
-      {
-        "id":               4,
-        "guid":             "resource-4",
-        "serviceName":      "cl1_hive",
-        "resourceElements": { "database": { "values": [ "db1" ] },  "table": { "values": [ "tbl1" ] },  "column": { "values": [ "Name" ] } }
-      },
-      {
-        "id":               5,
-        "guid":             "resource-5",
-        "serviceName":      "cl1_hive",
-        "resourceElements": { "database": { "values": [ "db2" ] }, "table": { "values": [ "*" ] }, "column": { "values": [ "*" ] } }
-      }
+      { "id": 1, "guid": "resource-1", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "db1" ] }, "table": { "values": [ "tbl1" ] } } },
+      { "id": 2, "guid": "resource-2", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "db1" ] }, "table": { "values": [ "tbl1" ] }, "column": { "values": [ "SSN" ] } } },
+      { "id": 3, "guid": "resource-3", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "db1" ] }, "table": { "values": [ "tbl1" ] }, "column": { "values": [ "Age" ] } } },
+      { "id": 4, "guid": "resource-4", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "db1" ] }, "table": { "values": [ "tbl1" ] }, "column": { "values": [ "Name" ] } } },
+      { "id": 5, "guid": "resource-5", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "db2" ] }, "table": { "values": [ "*" ] },    "column": { "values": [ "*" ] } } },
+      { "id": 6, "guid": "resource-6", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "order" ] }, "table": { "values":  [ "*" ] }, "column": { "values":  [ "*" ] } } },
+      { "id": 7, "guid": "resource-7", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "order" ] }, "table": { "values": [ "customer" ] } } },
+      { "id": 8, "guid": "resource-8", "serviceName": "cl1_hive", "resourceElements": { "database": { "values": [ "order" ] }, "table": { "values": [ "customer" ] }, "column": { "values": [ "address" ] } } }
     ],
     "resourceToTagIds": {
       "1": [ 1 ],
       "2": [ 2 ],
       "3": [ 3 ],
       "4": [ 4 ],
-      "5": [ 5 ]
+      "5": [ 5 ],
+      "6": [ 6 ],
+      "7": [ 7 ],
+      "8": [ 8 ]
     }
 }