You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ranger.apache.org by ma...@apache.org on 2015/04/02 04:31:34 UTC

incubator-ranger git commit: RANGER-354 Policy validation during create/update: prevent creation of policies with duplicate resources

Repository: incubator-ranger
Updated Branches:
  refs/heads/master 77f8ad98d -> 32f3262bc


RANGER-354 Policy validation during create/update: prevent creation of policies with duplicate resources

Signed-off-by: Madhan Neethiraj <ma...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/incubator-ranger/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-ranger/commit/32f3262b
Tree: http://git-wip-us.apache.org/repos/asf/incubator-ranger/tree/32f3262b
Diff: http://git-wip-us.apache.org/repos/asf/incubator-ranger/diff/32f3262b

Branch: refs/heads/master
Commit: 32f3262bcf44622b3f0af3d5ac323ed761ea337a
Parents: 77f8ad9
Author: Alok Lal <al...@hortonworks.com>
Authored: Wed Apr 1 01:21:38 2015 -0700
Committer: Madhan Neethiraj <ma...@apache.org>
Committed: Wed Apr 1 19:26:43 2015 -0700

----------------------------------------------------------------------
 .../RangerPolicyResourceSignature.java          | 142 +++++++++++++
 .../model/validation/RangerPolicyValidator.java | 130 ++++++++---
 .../model/validation/RangerValidator.java       |  13 +-
 .../ranger/plugin/util/RangerObjectFactory.java |  10 +
 .../TestRangerPolicyResourceSignature.java      | 213 +++++++++++++++++++
 .../validation/TestRangerPolicyValidator.java   | 125 +++++++++--
 .../model/validation/TestRangerValidator.java   |  18 +-
 .../model/validation/ValidationTestUtils.java   |  17 --
 8 files changed, 598 insertions(+), 70 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyResourceSignature.java
----------------------------------------------------------------------
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyResourceSignature.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyResourceSignature.java
new file mode 100644
index 0000000..0952ae8
--- /dev/null
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyResourceSignature.java
@@ -0,0 +1,142 @@
+package org.apache.ranger.plugin.model.validation;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.TreeMap;
+
+import org.apache.commons.codec.digest.DigestUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.ranger.plugin.model.RangerPolicy;
+import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyResource;
+
+public class RangerPolicyResourceSignature {
+
+	private static final Log LOG = LogFactory.getLog(RangerPolicyResourceSignature.class);
+	static final RangerPolicyResourceSignature _EmptyResourceSignature = new RangerPolicyResourceSignature((RangerPolicy)null);
+	
+	private final String _string;
+	private final String _hash;
+	private final RangerPolicy _policy;
+
+	public RangerPolicyResourceSignature(RangerPolicy policy) {
+		_policy = policy;
+		String asString = getResourceString(_policy);
+		if (asString == null) {
+			_string = "";
+		} else {
+			_string = asString;
+		}
+		_hash = DigestUtils.md5Hex(_string);
+	}
+
+	/**
+	 * Only added for testability.  Do not make public
+	 * @param string
+	 */
+	RangerPolicyResourceSignature(String string) {
+		_policy = null;
+		if (string == null) {
+			_string = "";
+		} else {
+			_string = string;
+		}
+		_hash = DigestUtils.md5Hex(_string);
+	}
+	
+	public String asString() {
+		return _string;
+	}
+
+	public String asHashHex() {
+		return _hash;
+	}
+	
+	@Override
+	public int hashCode() {
+		// we assume no collision
+		return Objects.hashCode(_hash);
+	}
+	
+	@Override
+	public boolean equals(Object object) {
+		if (object == null || !(object instanceof RangerPolicyResourceSignature)) {
+			return false;
+		}
+		RangerPolicyResourceSignature that = (RangerPolicyResourceSignature)object;
+		return Objects.equals(this._hash, that._hash);
+	}
+	
+	@Override
+	public String toString() {
+		return String.format("%s: %s", _hash, _string);
+	}
+
+	String getResourceString(RangerPolicy policy) {
+		// invalid/empty policy gets a deterministic signature as if it had an
+		// empty resource string
+		if (!isPolicyValidForResourceSignatureComputation(policy)) {
+			return null;
+		}
+		Map<String, RangerPolicyResourceView> resources = new TreeMap<String, RangerPolicyResourceView>();
+		for (Map.Entry<String, RangerPolicyResource> entry : policy.getResources().entrySet()) {
+			String resourceName = entry.getKey();
+			RangerPolicyResourceView resourceView = new RangerPolicyResourceView(entry.getValue());
+			resources.put(resourceName, resourceView);
+		}
+		String result = resources.toString();
+		return result;
+	}
+
+	boolean isPolicyValidForResourceSignatureComputation(RangerPolicy policy) {
+		boolean valid = false;
+		if (policy == null) {
+			LOG.debug("isPolicyValidForResourceSignatureComputation: policy was null!");
+		} else if (policy.getResources() == null) {
+			LOG.debug("isPolicyValidForResourceSignatureComputation: resources collection on policy was null!");
+		} else if (policy.getResources().containsKey(null)) {
+			LOG.debug("isPolicyValidForResourceSignatureComputation: resources collection has resource with null name!");
+		} else {
+			valid = true;
+		}
+		return valid;
+	}
+
+	static class RangerPolicyResourceView {
+		final RangerPolicyResource _policyResource;
+
+		RangerPolicyResourceView(RangerPolicyResource policyResource) {
+			_policyResource = policyResource;
+		}
+
+		@Override
+		public String toString() {
+			StringBuilder builder = new StringBuilder();
+			builder.append("{");
+			if (_policyResource != null) {
+				builder.append("values=");
+				if (_policyResource.getValues() != null) {
+					List<String> values = _policyResource.getValues();
+					Collections.sort(values);
+					builder.append(values);
+				}
+				builder.append(",excludes=");
+				if (_policyResource.getIsExcludes() == null) { // null is same as false
+					builder.append(Boolean.FALSE);
+				} else {
+					builder.append(_policyResource.getIsExcludes());
+				}
+				builder.append(",recursive=");
+				if (_policyResource.getIsRecursive() == null) { // null is the same as false
+					builder.append(Boolean.FALSE);
+				} else {
+					builder.append(_policyResource.getIsRecursive());
+				}
+			}
+			builder.append("}");
+			return builder.toString();
+		}
+	}
+}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
----------------------------------------------------------------------
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
index f5d6bff..b7500bd 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
@@ -1,6 +1,7 @@
 package org.apache.ranger.plugin.model.validation;
 
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -133,7 +134,7 @@ public class RangerPolicyValidator extends RangerValidator {
 					.build());
 				valid = false;
 			} else {
-				List<RangerPolicy> policies = getPolicies(policyName, serviceName);
+				List<RangerPolicy> policies = getPolicies(serviceName, policyName);
 				if (CollectionUtils.isNotEmpty(policies)) {
 					if (policies.size() > 1) {
 						failures.add(new ValidationFailureDetailsBuilder()
@@ -171,8 +172,8 @@ public class RangerPolicyValidator extends RangerValidator {
 				if (service == null) {
 					failures.add(new ValidationFailureDetailsBuilder()
 						.field("service")
-						.isMissing()
-						.becauseOf("service name was null/empty/blank")
+						.isSemanticallyIncorrect()
+						.becauseOf("service does not exist")
 						.build());
 					valid = false;
 				}
@@ -202,38 +203,107 @@ public class RangerPolicyValidator extends RangerValidator {
 					valid = isValidPolicyItems(policyItems, failures, serviceDef) && valid;
 				}
 			}
-			if (serviceDef != null) {
-				Set<String> mandatoryResources = getMandatoryResourceNames(serviceDef);
-				Set<String> policyResources = getPolicyResources(policy);
-				Set<String> missingResources = Sets.difference(mandatoryResources, policyResources);
-				if (!missingResources.isEmpty()) {
-					failures.add(new ValidationFailureDetailsBuilder()
-						.field("resources")
-						.subField(missingResources.iterator().next()) // we return any one parameter!
-						.isMissing()
-						.becauseOf("required resources[" + missingResources + "] are missing")
-						.build());
-					valid = false;
-				}
-				Set<String> allResource = getAllResourceNames(serviceDef);
-				Set<String> unknownResources = Sets.difference(policyResources, allResource);
-				if (!unknownResources.isEmpty()) {
-					failures.add(new ValidationFailureDetailsBuilder()
-						.field("resources")
-						.subField(unknownResources.iterator().next()) // we return any one parameter!
-						.isSemanticallyIncorrect()
-						.becauseOf("resource[" + unknownResources + "] is not valid for service-def[" + serviceDefName + "]")
-						.build());
-					valid = false;
+			valid = isValidResources(policy, failures, action, serviceDef, serviceName) && valid;
+		}
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s, %s): %s", policy, action, failures, valid));
+		}
+		return valid;
+	}
+	
+	boolean isValidResources(RangerPolicy policy, final List<ValidationFailureDetails> failures, Action action, final RangerServiceDef serviceDef, final String serviceName) {
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValidResources(%s, %s, %s, %s, %s)", policy, failures, action, serviceDef, serviceName));
+		}
+		
+		boolean valid = true;
+		if (serviceDef != null) { // following checks can't be done meaningfully otherwise
+			valid = isValidResourceNames(policy, failures, serviceDef);
+			Map<String, RangerPolicyResource> resourceMap = policy.getResources();
+			valid = isValidResourceValues(resourceMap, failures, serviceDef) && valid;
+			valid = isValidResourceFlags(resourceMap, failures, serviceDef.getResources(), serviceDef.getName(), policy.getName()) && valid;
+		}
+		if (StringUtils.isNotBlank(serviceName)) { // resource uniqueness check cannot be done meaningfully otherwise
+			valid = isPolicyResourceUnique(policy, failures, action, serviceName) && valid;
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValidResources(%s, %s, %s, %s, %s): %s", policy, failures, action, serviceDef, serviceName, valid));
+		}
+		return valid;
+	}
+	
+	boolean isPolicyResourceUnique(RangerPolicy policy, final List<ValidationFailureDetails> failures, Action action, final String serviceName) {
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isPolicyResourceUnique(%s, %s, %s, %s)", policy, failures, action, serviceName));
+		}
+
+		boolean foundDuplicate = false;
+		RangerPolicyResourceSignature signature = _factory.createPolicyResourceSignature(policy);
+		List<RangerPolicy> policies = getPolicies(serviceName, null);
+		if (CollectionUtils.isNotEmpty(policies)) {
+			Iterator<RangerPolicy> iterator = policies.iterator();
+			while (iterator.hasNext() && !foundDuplicate) {
+				RangerPolicy otherPolicy = iterator.next();
+				if (otherPolicy.getId().equals(policy.getId()) && action == Action.UPDATE) {
+					LOG.debug("isPolicyResourceUnique: Skipping self during update!");
+				} else {
+					RangerPolicyResourceSignature otherSignature = _factory.createPolicyResourceSignature(otherPolicy);
+					if (signature.equals(otherSignature)) {
+						foundDuplicate = true;
+						failures.add(new ValidationFailureDetailsBuilder()
+							.field("resources")
+							.isSemanticallyIncorrect()
+							.becauseOf("found another policy[" + policy.getName() + "] with matching resources[" + policy.getResources() + "]!")
+							.build());
+					}
 				}
-				Map<String, RangerPolicyResource> resourceMap = policy.getResources();
-				valid = isValidResourceValues(resourceMap, failures, serviceDef) && valid;
-				valid = isValidResourceFlags(resourceMap, failures, serviceDef.getResources(), serviceDefName, policyName) && valid;
 			}
 		}
+
+		boolean valid = !foundDuplicate;
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isPolicyResourceUnique(%s, %s, %s, %s): %s", policy, failures, action, serviceName, valid));
+		}
+		return valid;
+	}
+
+	boolean isValidResourceNames(final RangerPolicy policy, final List<ValidationFailureDetails> failures, final RangerServiceDef serviceDef) {
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s, %s): %s", policy, action, failures, valid));
+			LOG.debug(String.format("==> RangerPolicyValidator.isValidResourceNames(%s, %s, %s)", policy, failures, serviceDef));
+		}
+
+		boolean valid = true;
+		Set<String> mandatoryResources = getMandatoryResourceNames(serviceDef);
+		Set<String> policyResources = getPolicyResources(policy);
+		Set<String> missingResources = Sets.difference(mandatoryResources, policyResources);
+		if (!missingResources.isEmpty()) {
+			failures.add(new ValidationFailureDetailsBuilder()
+				.field("resources")
+				.subField(missingResources.iterator().next()) // we return any one parameter!
+				.isMissing()
+				.becauseOf("required resources[" + missingResources + "] are missing")
+				.build());
+			valid = false;
+		}
+		Set<String> allResource = getAllResourceNames(serviceDef);
+		Set<String> unknownResources = Sets.difference(policyResources, allResource);
+		if (!unknownResources.isEmpty()) {
+			failures.add(new ValidationFailureDetailsBuilder()
+				.field("resources")
+				.subField(unknownResources.iterator().next()) // we return any one parameter!
+				.isSemanticallyIncorrect()
+				.becauseOf("resource[" + unknownResources + "] is not valid for service-def[" + serviceDef.getName() + "]")
+				.build());
+			valid = false;
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValidResourceNames(%s, %s, %s): %s", policy, failures, serviceDef, valid));
 		}
 		return valid;
 	}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
----------------------------------------------------------------------
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
index 7bf744e..492949b 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
@@ -42,6 +42,7 @@ import org.apache.ranger.plugin.model.RangerServiceDef.RangerEnumDef;
 import org.apache.ranger.plugin.model.RangerServiceDef.RangerResourceDef;
 import org.apache.ranger.plugin.model.RangerServiceDef.RangerServiceConfigDef;
 import org.apache.ranger.plugin.store.ServiceStore;
+import org.apache.ranger.plugin.util.RangerObjectFactory;
 import org.apache.ranger.plugin.util.SearchFilter;
 
 public abstract class RangerValidator {
@@ -49,6 +50,7 @@ public abstract class RangerValidator {
 	private static final Log LOG = LogFactory.getLog(RangerValidator.class);
 
 	ServiceStore _store;
+	RangerObjectFactory _factory = new RangerObjectFactory();
 
 	public enum Action {
 		CREATE, UPDATE, DELETE;
@@ -242,15 +244,17 @@ public abstract class RangerValidator {
 		return result;
 	}
 
-	List<RangerPolicy> getPolicies(final String policyName, final String serviceName) {
+	List<RangerPolicy> getPolicies(final String serviceName, final String policyName) {
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("==> RangerValidator.getPolicies(" + policyName + ", " + serviceName + ")");
+			LOG.debug("==> RangerValidator.getPolicies(" + serviceName + ", " + policyName + ")");
 		}
 
 		List<RangerPolicy> policies = null;
 		try {
 			SearchFilter filter = new SearchFilter();
-			filter.setParam(SearchFilter.POLICY_NAME, policyName);
+			if (StringUtils.isNotBlank(policyName)) {
+				filter.setParam(SearchFilter.POLICY_NAME, policyName);
+			}
 			filter.setParam(SearchFilter.SERVICE_NAME, serviceName);
 			
 			policies = _store.getPolicies(filter);
@@ -259,7 +263,8 @@ public abstract class RangerValidator {
 		}
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("<== RangerValidator.getPolicies(" + policyName + ", " + serviceName + "): " + policies);
+			int count = policies == null ? 0 : policies.size();
+			LOG.debug("<== RangerValidator.getPolicies(" + serviceName + ", " + policyName + "): count[" + count + "], " + policies);
 		}
 		return policies;
 	}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/main/java/org/apache/ranger/plugin/util/RangerObjectFactory.java
----------------------------------------------------------------------
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/util/RangerObjectFactory.java b/agents-common/src/main/java/org/apache/ranger/plugin/util/RangerObjectFactory.java
new file mode 100644
index 0000000..e02c968
--- /dev/null
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/util/RangerObjectFactory.java
@@ -0,0 +1,10 @@
+package org.apache.ranger.plugin.util;
+
+import org.apache.ranger.plugin.model.RangerPolicy;
+import org.apache.ranger.plugin.model.validation.RangerPolicyResourceSignature;
+
+public class RangerObjectFactory {
+	public RangerPolicyResourceSignature createPolicyResourceSignature(RangerPolicy policy) {
+		return new RangerPolicyResourceSignature(policy);
+	}
+}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyResourceSignature.java
----------------------------------------------------------------------
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyResourceSignature.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyResourceSignature.java
new file mode 100644
index 0000000..7d34d96
--- /dev/null
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyResourceSignature.java
@@ -0,0 +1,213 @@
+package org.apache.ranger.plugin.model.validation;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.commons.codec.digest.DigestUtils;
+import org.apache.ranger.plugin.model.RangerPolicy;
+import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyResource;
+import org.apache.ranger.plugin.model.validation.RangerPolicyResourceSignature.RangerPolicyResourceView;
+import org.junit.Test;
+
+public class TestRangerPolicyResourceSignature {
+
+	@Test
+	public void test_RangerPolicyResourceView_toString() {
+		// null resource
+		RangerPolicyResource policyResource = null;
+		RangerPolicyResourceView policyResourceView = new RangerPolicyResourceView(policyResource);
+		assertEquals("{}", policyResourceView.toString());
+		
+		// non-null policy resource with null values/recursive flag
+		policyResource = createPolicyResource(null, null, null);
+		policyResourceView = new RangerPolicyResourceView(policyResource);
+		assertEquals("{values=,excludes=false,recursive=false}", policyResourceView.toString());
+		
+		// valid values in non-asending order
+		policyResource = createPolicyResource(new String[]{"b", "a", "d", "c"}, true, false);
+		policyResourceView = new RangerPolicyResourceView(policyResource);
+		assertEquals("{values=[a, b, c, d],excludes=false,recursive=true}", policyResourceView.toString());
+		
+		// recursive flag is false and different variation of values to show lexicographic ordering
+		policyResource = createPolicyResource(new String[]{"9", "A", "e", "_"}, false, true);
+		policyResourceView = new RangerPolicyResourceView(policyResource);
+		assertEquals("{values=[9, A, _, e],excludes=true,recursive=false}", policyResourceView.toString());
+	}
+	
+	RangerPolicyResource createPolicyResource(String[] values, Boolean recursive, Boolean excludes) {
+		
+		RangerPolicyResource resource = mock(RangerPolicyResource.class);
+		if (values == null) {
+			when(resource.getValues()).thenReturn(null);
+		} else {
+			when(resource.getValues()).thenReturn(Arrays.asList(values));
+		}
+		when(resource.getIsRecursive()).thenReturn(recursive);
+		when(resource.getIsExcludes()).thenReturn(excludes);
+		
+		return resource;
+	}
+
+	@Test
+	public void test_isPolicyValidForResourceSignatureComputation() {
+		// null policy is invalid
+		RangerPolicyResourceSignature utils = new RangerPolicyResourceSignature((String)null);
+		RangerPolicy rangerPolicy = null;
+		assertFalse("policy==null", utils.isPolicyValidForResourceSignatureComputation(rangerPolicy));
+
+		// null resource map is invalid
+		rangerPolicy = mock(RangerPolicy.class);
+		when(rangerPolicy.getResources()).thenReturn(null);
+		assertFalse("policy.getResources()==null", utils.isPolicyValidForResourceSignatureComputation(rangerPolicy));
+		
+		// empty resources map is ok!
+		Map<String, RangerPolicyResource> policyResources = new HashMap<String, RangerPolicyResource>();
+		when(rangerPolicy.getResources()).thenReturn(policyResources);
+		assertTrue("policy.getResources().isEmpty()", utils.isPolicyValidForResourceSignatureComputation(rangerPolicy));
+		
+		// but having a resource map with null key is not ok!
+		RangerPolicyResource aPolicyResource = mock(RangerPolicyResource.class);
+		policyResources.put(null, aPolicyResource);
+		assertFalse("policy.getResources().contains(null)", utils.isPolicyValidForResourceSignatureComputation(rangerPolicy));
+	}
+	
+	@Test
+	public void test_RangerPolicyResourceSignature() {
+		// String rep of a null policy is an empty string! and its hash is sha of empty string!
+		RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature((String)null);
+		assertEquals("", signature.asString());
+		assertEquals(DigestUtils.md5Hex(""), signature.asHashHex());
+	}
+
+	/*
+	 * Format of data expected by the utility function which uses this is:
+	 * { "resource-name", "values" "isExcludes", "isRecursive" }
+	 */
+	Object[][] first = new Object[][] {
+			{ "table", new String[] { "tbl3", "tbl1", "tbl2"}, true,  false},
+			{ "db",    new String[] { "db1", "db2"},           false, null},
+			{ "col",   new String[] { "col2", "col1", "col3"}, null,  true},
+	};
+
+	Object[][] first_recursive_null_or_false = new Object[][] {
+			{ "table", new String[] { "tbl3", "tbl1", "tbl2"}, true,  null}, // recursive flag is false in first
+			{ "db",    new String[] { "db1", "db2"},           false, null},
+			{ "col",   new String[] { "col2", "col1", "col3"}, null,  true},
+	};
+
+	Object[][] first_recursive_flag_different = new Object[][] {
+			{ "table", new String[] { "tbl3", "tbl1", "tbl2"}, true,  false},
+			{ "db",    new String[] { "db1", "db2"},           false, null},
+			{ "col",   new String[] { "col2", "col1", "col3"}, null,  false}, // recursive flag is true in first
+	};
+
+	Object[][] first_excludes_null_or_false = new Object[][] {
+			{ "table", new String[] { "tbl3", "tbl1", "tbl2"}, true,  false},
+			{ "db",    new String[] { "db1", "db2"},           false, null}, // excludes flag is null in first
+			{ "col",   new String[] { "col2", "col1", "col3"}, false, true},
+	};
+
+	Object[][] first_excludes_flag_different = new Object[][] {
+			{ "table", new String[] { "tbl3", "tbl1", "tbl2"}, true,  false},
+			{ "db",    new String[] { "db1", "db2"},           false, null},
+			{ "col",   new String[] { "col2", "col1", "col3"}, true,  true}, // excludes flag is false in first
+	};
+
+	Object[][] data_second = new Object[][] {
+			{ "db",    new String[] { "db2", "db1"},           false, null},
+			{ "table", new String[] { "tbl2", "tbl3", "tbl1"}, true, false},
+			{ "col",   new String[] { "col1", "col3", "col2"}, null, true},
+	};
+
+	@Test
+	public void test_getResourceSignature_happyPath() {
+		// null policy returns signature of empty resource
+		RangerPolicy policy = null;
+		RangerPolicyResourceSignature sig = new RangerPolicyResourceSignature(policy);
+		assertEquals(null, sig.getResourceString(policy));
+		
+		policy = mock(RangerPolicy.class);
+		Map<String, RangerPolicyResource> policyResources = _utils.createPolicyResourceMap(first);
+		when(policy.getResources()).thenReturn(policyResources);
+		String expected = "{" +
+			"col={values=[col1, col2, col3],excludes=false,recursive=true}, " + 
+			"db={values=[db1, db2],excludes=false,recursive=false}, " +
+			"table={values=[tbl1, tbl2, tbl3],excludes=true,recursive=false}" +
+		"}"; 
+		assertEquals(expected, sig.getResourceString(policy));
+
+		// order of values should not matter
+		policyResources = _utils.createPolicyResourceMap(data_second);
+		when(policy.getResources()).thenReturn(policyResources);
+		assertEquals(expected, sig.getResourceString(policy));
+	}
+	
+	
+	@Test
+	public void test_nullRecursiveFlagIsSameAsFlase() {
+		// create two policies with resources that differ only in the recursive flag such that flags are null in one and false in another
+		RangerPolicy policy1 = createPolicy(first);
+		RangerPolicy policy2 = createPolicy(first_recursive_null_or_false);
+		RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature((String)null);
+		assertEquals("null is same as false", signature.getResourceString(policy1), signature.getResourceString(policy2));
+	}
+	
+	@Test
+	public void test_onlyDifferByRecursiveFlag() {
+		// create two policies with resources that differ only in the recursive flag, i.e. null/false in one and true in another
+		RangerPolicy policy1 = createPolicy(first);
+		RangerPolicy policy2 = createPolicy(first_recursive_flag_different);
+		RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature((String)null);
+		assertFalse("Resources differ only by recursive flag true vs false/null", signature.getResourceString(policy1).equals(signature.getResourceString(policy2)));
+	}
+	
+	@Test
+	public void test_nullExcludesFlagIsSameAsFlase() {
+		// create two policies with resources that differ only in the excludes flag such that flags are null in one and false in another
+		RangerPolicy policy1 = createPolicy(first);
+		RangerPolicy policy2 = createPolicy(first_excludes_null_or_false);
+		RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature((String)null);
+		assertEquals("null is same as false", signature.getResourceString(policy1), signature.getResourceString(policy2));
+	}
+	
+	@Test
+	public void test_onlyDifferByExcludesFlag() {
+		// create two policies with resources that differ only in the excludes flag, i.e. null/false in one and true in another
+		RangerPolicy policy1 = createPolicy(first);
+		RangerPolicy policy2 = createPolicy(first_excludes_flag_different);
+		RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature((String)null);
+		assertFalse("Resources differ only by recursive flag true vs false/null", signature.getResourceString(policy1).equals(signature.getResourceString(policy2)));
+	}
+	
+	RangerPolicy createPolicy(Object[][] data) {
+		RangerPolicy policy = mock(RangerPolicy.class);
+		Map<String, RangerPolicyResource> resources = _utils.createPolicyResourceMap(data);
+		when(policy.getResources()).thenReturn(resources);
+		return policy;
+	}
+
+	@Test
+	public void test_integration() {
+		// setup two policies with resources that are structurally different but semantically the same.
+		RangerPolicy aPolicy = mock(RangerPolicy.class);
+		Map<String, RangerPolicyResource> resources = _utils.createPolicyResourceMap(first);
+		when(aPolicy.getResources()).thenReturn(resources);
+		RangerPolicyResourceSignature signature = new RangerPolicyResourceSignature(aPolicy);
+		
+		RangerPolicy anotherPolicy = mock(RangerPolicy.class);
+		resources = _utils.createPolicyResourceMap(data_second);
+		when(anotherPolicy.getResources()).thenReturn(resources);
+		RangerPolicyResourceSignature anotherSignature = new RangerPolicyResourceSignature(anotherPolicy);
+		assertTrue(signature.equals(anotherSignature));
+		assertTrue(anotherSignature.equals(signature));
+	}
+	
+	ValidationTestUtils _utils = new ValidationTestUtils();
+}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
----------------------------------------------------------------------
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
index e0f68ad..edf19d5 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
@@ -3,10 +3,12 @@ package org.apache.ranger.plugin.model.validation;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Matchers.argThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -19,13 +21,13 @@ import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyResource;
 import org.apache.ranger.plugin.model.RangerService;
 import org.apache.ranger.plugin.model.RangerServiceDef;
 import org.apache.ranger.plugin.model.RangerServiceDef.RangerResourceDef;
-import org.apache.ranger.plugin.model.validation.RangerPolicyValidator;
-import org.apache.ranger.plugin.model.validation.ValidationFailureDetails;
 import org.apache.ranger.plugin.model.validation.RangerValidator.Action;
 import org.apache.ranger.plugin.store.ServiceStore;
+import org.apache.ranger.plugin.util.RangerObjectFactory;
 import org.apache.ranger.plugin.util.SearchFilter;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.ArgumentMatcher;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
@@ -38,6 +40,8 @@ public class TestRangerPolicyValidator {
 		_policy = mock(RangerPolicy.class);
 		_validator = new RangerPolicyValidator(_store);
 		_serviceDef = mock(RangerServiceDef.class);
+		_factory = mock(RangerObjectFactory.class);
+		_validator._factory = _factory;
 	}
 	
 	final Action[] cu = new Action[] { Action.CREATE, Action.UPDATE };
@@ -179,7 +183,13 @@ public class TestRangerPolicyValidator {
 		when(_serviceDef.getResources()).thenReturn(resourceDefs);
 		Map<String, RangerPolicyResource> resourceMap = _utils.createPolicyResourceMap(policyResourceMap_good);
 		when(_policy.getResources()).thenReturn(resourceMap);
-
+		// let's add some other policies in the store for this service that have a different signature
+		SearchFilter resourceDuplicationFilter = new SearchFilter();
+		resourceDuplicationFilter.setParam(SearchFilter.SERVICE_NAME, "service-name");
+		when(_factory.createPolicyResourceSignature(_policy)).thenReturn(new RangerPolicyResourceSignature("policy"));
+		when(_factory.createPolicyResourceSignature(existingPolicy)).thenReturn(new RangerPolicyResourceSignature("policy-name-2"));
+		// we are reusing the same policies collection here -- which is fine
+		when(_store.getPolicies(resourceDuplicationFilter)).thenReturn(existingPolicies);
 		for (Action action : cu) {
 			if (action == Action.CREATE) {
 				when(_policy.getId()).thenReturn(7L);
@@ -277,17 +287,35 @@ public class TestRangerPolicyValidator {
 		
 		// policy must have service name on it and it should be valid
 		when(_policy.getName()).thenReturn("policy-name");
+		for (Action action : cu) {
+			when(_policy.getService()).thenReturn(null);
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "service");
+
+			when(_policy.getService()).thenReturn("");
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "service");
+		}
+		
+		// service name should be valid
 		when(_store.getServiceByName("service-name")).thenReturn(null);
 		when(_store.getServiceByName("another-service-name")).thenThrow(new Exception());
-
 		for (Action action : cu) {
-			when(_policy.getService()).thenReturn("service-name");
+			when(_policy.getService()).thenReturn(null);
 			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
 			_utils.checkFailureForMissingValue(_failures, "service");
 
-			when(_policy.getService()).thenReturn("another-service-name");
+			when(_policy.getService()).thenReturn(null);
 			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
 			_utils.checkFailureForMissingValue(_failures, "service");
+
+			when(_policy.getService()).thenReturn("service-name");
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForSemanticError(_failures, "service");
+
+			when(_policy.getService()).thenReturn("another-service-name");
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForSemanticError(_failures, "service");
 		}
 		
 		// policy must contain at least one policy item
@@ -331,7 +359,8 @@ public class TestRangerPolicyValidator {
 		List<RangerResourceDef> resourceDefs = _utils.createResourceDefs(resourceDefData);
 		when(_serviceDef.getResources()).thenReturn(resourceDefs);
 		when(_store.getServiceDefByName("service-type")).thenReturn(_serviceDef);
-		// one mandtory is missing (tbl) and one unknown resource is specified (extra), and values of option resource don't conform to validation pattern (col)
+
+		// one mandatory is missing (tbl) and one unknown resource is specified (extra), and values of option resource don't conform to validation pattern (col)
 		Map<String, RangerPolicyResource> policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad);
 		when(_policy.getResources()).thenReturn(policyResources);
 		for (Action action : cu) {
@@ -342,6 +371,28 @@ public class TestRangerPolicyValidator {
 			_utils.checkFailureForSemanticError(_failures, "isRecursive", "db"); // for specifying it as true when def did not allow it
 			_utils.checkFailureForSemanticError(_failures, "isExcludes", "col"); // for specifying it as true when def did not allow it
 		}
+		
+		// create the right resource def but let it clash with another policy with matching resource-def
+		policyResources = _utils.createPolicyResourceMap(policyResourceMap_good);
+		when(_policy.getResources()).thenReturn(policyResources);
+		filter = new SearchFilter(); filter.setParam(SearchFilter.SERVICE_NAME, "service-name");
+		when(_store.getPolicies(filter)).thenReturn(existingPolicies);
+		// we are doctoring the factory to always return the same signature
+		when(_factory.createPolicyResourceSignature(anyPolicy())).thenReturn(new RangerPolicyResourceSignature("blah"));
+		for (Action action : cu) {
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForSemanticError(_failures, "resources");
+		}
+	}
+	
+	RangerPolicy anyPolicy() {
+		return argThat(new ArgumentMatcher<RangerPolicy>() {
+
+			@Override
+			public boolean matches(Object argument) {
+				return true;
+			}
+		});
 	}
 	
 	@Test
@@ -480,10 +531,11 @@ public class TestRangerPolicyValidator {
 	};
 	
 	private Object[][] policyResourceMap_happyPath = new Object[][] {
-			// { "resource-name", "isExcludes", "isRecursive" }
-			{ "db", null, true },    // null should be treated as false
-			{ "tbl", false, false }, // set to false where def is null and def is true  
-			{ "col", true, null}     // set to null where def is false
+			// { "resource-name", "values" "isExcludes", "isRecursive" }
+			// values collection is null as it isn't relevant to the part being tested with this data
+			{ "db", null, null, true },    // null should be treated as false
+			{ "tbl", null, false, false }, // set to false where def is null and def is true  
+			{ "col", null, true, null}     // set to null where def is false
 	};
 	
 	@Test
@@ -491,34 +543,73 @@ public class TestRangerPolicyValidator {
 		// passing null values effectively bypasses the filter
 		assertTrue(_validator.isValidResourceFlags(null, _failures, null, "a-service-def", "a-policy"));
 		// so does passing in empty collections
-		Map<String, RangerPolicyResource> resourceMap = _utils.createPolicyResourceMap2(policyResourceMap_happyPath);
+		Map<String, RangerPolicyResource> resourceMap = _utils.createPolicyResourceMap(policyResourceMap_happyPath);
 		List<RangerResourceDef> resourceDefs = _utils.createResourceDefs2(resourceDef_happyPath);
 		when(_serviceDef.getResources()).thenReturn(resourceDefs);
 		assertTrue(_validator.isValidResourceFlags(resourceMap, _failures, resourceDefs, "a-service-def", "a-policy"));
 	}
 
 	private Object[][] policyResourceMap_failures = new Object[][] {
-			// { "resource-name", "isExcludes", "isRecursive" }
-			{ "db", true, true },    // ok: def has true for both  
-			{ "tbl", true, null },   // excludes: def==false, policy==true  
-			{ "col", false, true }    // recursive: def==null (i.e. false), policy==true
+			// { "resource-name", "values" "isExcludes", "isRecursive" }
+			// values collection is null as it isn't relevant to the part being tested with this data
+			{ "db", null, true, true },    // ok: def has true for both  
+			{ "tbl", null, true, null },   // excludes: def==false, policy==true  
+			{ "col", null, false, true }    // recursive: def==null (i.e. false), policy==true
 	};
 	
 	@Test
 	public final void test_isValidResourceFlags_failures() {
 		// passing true when def says false/null
 		List<RangerResourceDef> resourceDefs = _utils.createResourceDefs2(resourceDef_happyPath);
-		Map<String, RangerPolicyResource> resourceMap = _utils.createPolicyResourceMap2(policyResourceMap_failures);
+		Map<String, RangerPolicyResource> resourceMap = _utils.createPolicyResourceMap(policyResourceMap_failures);
 		when(_serviceDef.getResources()).thenReturn(resourceDefs);
 		assertFalse(_validator.isValidResourceFlags(resourceMap, _failures, resourceDefs, "a-service-def", "a-policy"));
 		_utils.checkFailureForSemanticError(_failures, "isExcludes", "tbl");
 		_utils.checkFailureForSemanticError(_failures, "isRecursive", "col");
 	}
 
+	@Test
+	public final void test_isPolicyResourceUnique() throws Exception {
+
+		RangerPolicy[] policies = new RangerPolicy[3];
+		RangerPolicyResourceSignature[] signatures = new RangerPolicyResourceSignature[3];
+		for (int i = 0; i < 3; i++) {
+			RangerPolicy policy = mock(RangerPolicy.class);
+			when(policy.getId()).thenReturn((long)i);
+			policies[i] = policy;
+			signatures[i] = new RangerPolicyResourceSignature("policy" + i);
+			when(_factory.createPolicyResourceSignature(policies[i])).thenReturn(signatures[i]);
+		}
+		
+		SearchFilter searchFilter = new SearchFilter();
+		String serviceName = "aService";
+		searchFilter.setParam(SearchFilter.SERVICE_NAME, serviceName);
+		
+		List<RangerPolicy> existingPolicies = Arrays.asList(new RangerPolicy[] { policies[1], policies[2]} );
+		// all existing policies have distinct signatures
+		for (Action action : cu) {
+			when(_store.getPolicies(searchFilter)).thenReturn(existingPolicies);
+			assertTrue("No duplication: " + action, _validator.isPolicyResourceUnique(policies[0], _failures, action, serviceName));
+		}
+	
+		// Failure if signature matches an existing policy
+		// We change the signature of 3rd policy to be same as that of 1st so duplication check will fail
+		for (Action action : cu) {
+			when(_factory.createPolicyResourceSignature(policies[2])).thenReturn(new RangerPolicyResourceSignature("policy0"));
+			when(_store.getPolicies(searchFilter)).thenReturn(existingPolicies);
+			assertFalse("Duplication:" + action, _validator.isPolicyResourceUnique(policies[0], _failures, action, serviceName));
+		}
+
+		// update should exclude itself! - let's change id of 3rd policy to be the same as the 1st one.
+		when(policies[2].getId()).thenReturn((long)0);
+		assertTrue("No duplication if updating policy", _validator.isPolicyResourceUnique(policies[0], _failures, Action.UPDATE, serviceName));
+	}
+	
 	private ValidationTestUtils _utils = new ValidationTestUtils();
 	private List<ValidationFailureDetails> _failures = new ArrayList<ValidationFailureDetails>();
 	private ServiceStore _store;
 	private RangerPolicy _policy;
 	private RangerPolicyValidator _validator;
 	private RangerServiceDef _serviceDef;
+	private RangerObjectFactory _factory;
 }

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
----------------------------------------------------------------------
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
index f17b2c2..f014552 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
@@ -401,15 +401,29 @@ public class TestRangerValidator {
 		filter.setParam(SearchFilter.SERVICE_NAME, serviceName);
 		
 		when(_store.getPolicies(filter)).thenReturn(null);
-		List<RangerPolicy> result = _validator.getPolicies(policyName, serviceName);
+		List<RangerPolicy> result = _validator.getPolicies(serviceName, policyName);
 		// validate store is queried with both parameters
 		verify(_store).getPolicies(filter);
 		assertNull(result);
 
 		// returns null if store throws an exception
 		when(_store.getPolicies(filter)).thenThrow(new Exception());
-		result = _validator.getPolicies(policyName, serviceName);
+		result = _validator.getPolicies(serviceName, policyName);
 		assertNull(result);
+		
+		// does not shove policy into search filter if policy name passed in is "blank"
+		filter = new SearchFilter();
+		filter.setParam(SearchFilter.SERVICE_NAME, serviceName);
+
+		List<RangerPolicy> policies = new ArrayList<RangerPolicy>();
+		RangerPolicy policy = mock(RangerPolicy.class);
+		policies.add(policy);
+		
+		when(_store.getPolicies(filter)).thenReturn(policies);
+		for (String aName : new String[]{ null, "", "  "}) {
+			result = _validator.getPolicies(serviceName, aName);
+			assertTrue(result.iterator().next() == policy);
+		}
 	}
 	
 	@Test

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/32f3262b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/ValidationTestUtils.java
----------------------------------------------------------------------
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/ValidationTestUtils.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/ValidationTestUtils.java
index 5ed2691..1e81ec3 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/ValidationTestUtils.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/ValidationTestUtils.java
@@ -295,23 +295,6 @@ public class ValidationTestUtils {
 		return defs;
 	}
 
-	Map<String, RangerPolicyResource> createPolicyResourceMap2(Object[][] input) {
-		if (input == null) {
-			return null;
-		}
-		Map<String, RangerPolicyResource> result = new HashMap<String, RangerPolicyResource>(input.length);
-		for (Object[] row : input) {
-			String resourceName = (String)row[0];
-			Boolean isExcludes = (Boolean)row[1];
-			Boolean isRecursive = (Boolean)row[2];
-			RangerPolicyResource aResource = mock(RangerPolicyResource.class);
-			when(aResource.getIsExcludes()).thenReturn(isExcludes);
-			when(aResource.getIsRecursive()).thenReturn(isRecursive);
-			result.put(resourceName, aResource);
-		}
-		return result;
-	}
-
 	List<RangerEnumElementDef> createEnumElementDefs(String[] input) {
 		if (input == null) {
 			return null;