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/03/10 23:13:49 UTC

[2/2] incubator-ranger git commit: RANGER-278 Add validation for policy create/update/delete operations. Moved action out of ctor to validate call.

RANGER-278 Add validation for policy create/update/delete operations. Moved action out of ctor to validate call.

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/f721abec
Tree: http://git-wip-us.apache.org/repos/asf/incubator-ranger/tree/f721abec
Diff: http://git-wip-us.apache.org/repos/asf/incubator-ranger/diff/f721abec

Branch: refs/heads/master
Commit: f721abec9cd95ac4c6b70fb964884346f2814ed1
Parents: 96fd15e
Author: Alok Lal <al...@hortonworks.com>
Authored: Wed Mar 4 23:32:28 2015 -0800
Committer: Madhan Neethiraj <ma...@apache.org>
Committed: Tue Mar 10 15:13:32 2015 -0700

----------------------------------------------------------------------
 .../apache/ranger/plugin/util/SearchFilter.java |  15 +
 pom.xml                                         |   2 +-
 .../ranger/rest/RangerPolicyValidator.java      | 430 +++++++++++++++++
 .../ranger/rest/RangerServiceValidator.java     | 135 +++---
 .../org/apache/ranger/rest/RangerValidator.java | 310 ++++++++++--
 .../ranger/rest/RangerValidatorFactory.java     |  10 +-
 .../org/apache/ranger/rest/ServiceREST.java     |  34 +-
 .../ranger/rest/TestRangerPolicyValidator.java  | 471 +++++++++++++++++++
 .../ranger/rest/TestRangerServiceValidator.java | 246 +++++-----
 .../apache/ranger/rest/TestRangerValidator.java | 333 +++++++++++++
 .../rest/TestServiceRESTForValidation.java      | 225 +++++++--
 .../ranger/rest/TestServiceValidator.java       | 202 --------
 .../apache/ranger/rest/ValidationTestUtils.java | 195 ++++++++
 13 files changed, 2126 insertions(+), 482 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java
----------------------------------------------------------------------
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java b/agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java
index ab8384c..d67df8d 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java
@@ -21,6 +21,7 @@ package org.apache.ranger.plugin.util;
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 
 import org.apache.commons.collections.MapUtils;
 import org.apache.commons.lang.StringUtils;
@@ -113,4 +114,18 @@ public class SearchFilter {
 	public boolean isEmpty() {
 		return MapUtils.isEmpty(params);
 	}
+	
+	@Override
+	public boolean equals(Object object) {
+		if (object == null || !(object instanceof SearchFilter)) {
+			return false;
+		}
+		SearchFilter that = (SearchFilter)object;
+		return Objects.equals(params, that.params);
+	}
+	
+	@Override
+	public int hashCode() {
+		return Objects.hash(params);
+	}
 }

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/pom.xml
----------------------------------------------------------------------
diff --git a/pom.xml b/pom.xml
index 79a80b9..7df033d 100644
--- a/pom.xml
+++ b/pom.xml
@@ -309,7 +309,7 @@
         <artifactId>maven-surefire-plugin</artifactId>
         <version>2.17</version>
         <configuration>
-           <argLine>-Djava.library.path=${hadoop.library.path}${path.separator}${java.library.path}"</argLine>
+           <argLine>-Djava.library.path=${hadoop.library.path}${path.separator}${java.library.path}</argLine>
            <skipTests>${skipTests}</skipTests>
            <encoding>UTF-8</encoding>
            <systemProperties>

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/main/java/org/apache/ranger/rest/RangerPolicyValidator.java
----------------------------------------------------------------------
diff --git a/security-admin/src/main/java/org/apache/ranger/rest/RangerPolicyValidator.java b/security-admin/src/main/java/org/apache/ranger/rest/RangerPolicyValidator.java
new file mode 100644
index 0000000..941bb21
--- /dev/null
+++ b/security-admin/src/main/java/org/apache/ranger/rest/RangerPolicyValidator.java
@@ -0,0 +1,430 @@
+package org.apache.ranger.rest;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang3.StringUtils;
+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.RangerPolicyItem;
+import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyItemAccess;
+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.store.ServiceStore;
+
+import com.google.common.collect.Sets;
+
+public class RangerPolicyValidator extends RangerValidator {
+
+	private static final Log LOG = LogFactory.getLog(RangerPolicyValidator.class);
+
+	public RangerPolicyValidator(ServiceStore store) {
+		super(store);
+	}
+
+	public void validate(RangerPolicy policy, Action action) throws Exception {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerValidator.validate(%s, %s)", policy, action));
+		}
+
+		List<ValidationFailureDetails> failures = new ArrayList<ValidationFailureDetails>();
+		boolean valid = isValid(policy, action, failures);
+		String message = "";
+		try {
+			if (!valid) {
+				message = serializeFailures(failures);
+				throw new Exception(message);
+			}
+		} finally {
+			if(LOG.isDebugEnabled()) {
+				LOG.debug(String.format("<== RangerValidator.validate(%s, %s): %s, reason[%s]", policy, action, valid, message));
+			}
+		}
+	}
+
+	@Override
+	boolean isValid(Long id, Action action, List<ValidationFailureDetails> failures) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValid(%s, %s, %s)", id, action, failures));
+		}
+
+		boolean valid = true;
+		if (action != Action.DELETE) {
+			failures.add(new ValidationFailureDetailsBuilder()
+				.isAnInternalError()
+				.becauseOf("isValid(Long) is only supported for DELETE")
+				.build());
+			valid = false;
+		} else if (id == null) {
+			failures.add(new ValidationFailureDetailsBuilder()
+				.field("id")
+				.isMissing()
+				.build());
+			valid = false;
+		} else if (getPolicy(id) == null) {
+			failures.add(new ValidationFailureDetailsBuilder()
+				.field("id")
+				.isSemanticallyIncorrect()
+				.becauseOf("no policy found for id[" + id + "]")
+				.build());
+			valid = false;
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s, %s): %s", id, action, failures, valid));
+		}
+		return valid;
+	}
+
+	boolean isValid(RangerPolicy policy, Action action, List<ValidationFailureDetails> failures) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValid(%s, %s, %s)", policy, action, failures));
+		}
+
+		if (!(action == Action.CREATE || action == Action.UPDATE)) {
+			throw new IllegalArgumentException("isValid(RangerPolicy, ...) is only supported for create/update");
+		}
+		boolean valid = true;
+		if (policy == null) {
+			String message = "policy object passed in was null";
+			LOG.debug(message);
+			failures.add(new ValidationFailureDetailsBuilder()
+				.field("policy")
+				.isMissing()
+				.becauseOf(message)
+				.build());
+			valid = false;
+		} else {
+			Long id = policy.getId();
+			if (action == Action.UPDATE) { // id is ignored for CREATE
+				if (id == null) {
+					String message = "policy id was null/empty/blank"; 
+					LOG.debug(message);
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("id")
+						.isMissing()
+						.becauseOf(message)
+						.build());
+					valid = false;
+				} else if (getPolicy(id) == null) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("id")
+						.isSemanticallyIncorrect()
+						.becauseOf("no policy exists with id[" + id +"]")
+						.build());
+					valid = false;
+				}
+			}
+			String policyName = policy.getName();
+			String serviceName = policy.getService();
+			if (StringUtils.isBlank(policyName)) {
+				String message = "policy name was null/empty/blank[" + policyName + "]"; 
+				LOG.debug(message);
+				failures.add(new ValidationFailureDetailsBuilder()
+					.field("name")
+					.isMissing()
+					.becauseOf(message)
+					.build());
+				valid = false;
+			} else {
+				List<RangerPolicy> policies = getPolicies(policyName, serviceName);
+				if (CollectionUtils.isNotEmpty(policies)) {
+					if (policies.size() > 1) {
+						failures.add(new ValidationFailureDetailsBuilder()
+							.isAnInternalError()
+							.becauseOf("multiple policies found with the name[" + policyName + "]")
+							.build());
+						valid = false;
+					} else if (action == Action.CREATE) { // size == 1
+						failures.add(new ValidationFailureDetailsBuilder()
+							.field("name")
+							.isSemanticallyIncorrect()
+							.becauseOf("policy already exists with name[" + policyName + "]; its id is[" + policies.iterator().next().getId() + "]")
+							.build());
+						valid = false;
+					} else if (policies.iterator().next().getId() != id) { // size == 1 && action == UPDATE
+						failures.add(new ValidationFailureDetailsBuilder()
+							.field("id/name")
+							.isSemanticallyIncorrect()
+							.becauseOf("id/name conflict: policy already exists with name[" + policyName + "], its id is[" + policies.iterator().next().getId() + "]")
+							.build());
+						valid = false;
+					}
+				}
+			}
+			RangerService service = null;
+			if (StringUtils.isBlank(serviceName)) {
+				failures.add(new ValidationFailureDetailsBuilder()
+				.field("service")
+				.isMissing()
+				.becauseOf("service name was null/empty/blank")
+				.build());
+				valid = false;
+			} else {
+				service = getService(serviceName);
+				if (service == null) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("service")
+						.isMissing()
+						.becauseOf("service name was null/empty/blank")
+						.build());
+					valid = false;
+				}
+			}
+			List<RangerPolicyItem> policyItems = policy.getPolicyItems();
+			boolean isAuditEnabled = getIsAuditEnabled(policy);
+			RangerServiceDef serviceDef = null;
+			String serviceDefName = null;
+			if (CollectionUtils.isEmpty(policyItems) && !isAuditEnabled) {
+				failures.add(new ValidationFailureDetailsBuilder()
+					.field("policy items")
+					.isMissing()
+					.becauseOf("at least one policy item must be specified if audit isn't enabled")
+					.build());
+				valid = false;
+			} else if (service != null) {
+				serviceDefName = service.getType();
+				serviceDef = getServiceDef(serviceDefName);
+				if (serviceDef == null) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("policy service def")
+						.isAnInternalError()
+						.becauseOf("Service def of policies service does not exist")
+						.build());
+					valid = false;
+				} else {
+					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;
+				}
+				Map<String, RangerPolicyResource> resourceMap = policy.getResources();
+				// errors about if empty resource collection is ok or not has already happened above, this check is still needed 
+				if (resourceMap != null && !resourceMap.isEmpty()) {
+					valid = isValidResourceValues(resourceMap, failures, serviceDef) && valid;
+				}
+			}
+		}
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s, %s): %s", policy, action, failures, valid));
+		}
+		return valid;
+	}
+	
+	boolean isValidResourceValues(Map<String, RangerPolicyResource> resourceMap, List<ValidationFailureDetails> failures, RangerServiceDef serviceDef) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValidResourceValues(%s, %s, %s)", resourceMap, failures, serviceDef));
+		}
+
+		boolean valid = true;
+		if (resourceMap == null) {
+			LOG.debug("isValidResourceValues: resourceMap is null");
+		} else if (serviceDef == null) {
+			LOG.debug("isValidResourceValues: service Def is null");
+		} else {
+			Map<String, String> validationRegExMap = getValidationRegExes(serviceDef);
+			for (Map.Entry<String, RangerPolicyResource> entry : resourceMap.entrySet()) {
+				String name = entry.getKey();
+				RangerPolicyResource policyResource = entry.getValue();
+				if (validationRegExMap.containsKey(name) && policyResource != null && CollectionUtils.isNotEmpty(policyResource.getValues())) {
+					String regEx = validationRegExMap.get(name);
+					for (String aValue : policyResource.getValues()) {
+						if (StringUtils.isBlank(aValue)) {
+							LOG.debug("resource value was blank");
+						} else if (!aValue.matches(regEx)) {
+							failures.add(new ValidationFailureDetailsBuilder()
+								.field("resource-values")
+								.subField(name)
+								.isSemanticallyIncorrect()
+								.becauseOf("resources value[" + aValue + "] does not match validation regex[" + regEx + "] defined on service-def[" + serviceDef.getName() + "]")
+								.build());
+							valid = false;
+						}
+					}
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValidResourceValues(%s, %s, %s): %s", resourceMap, failures, serviceDef, valid));
+		}
+		return valid;
+	}
+
+	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));
+		}
+
+		boolean valid = true;
+		if (CollectionUtils.isEmpty(policyItems)) {
+			LOG.debug("policy items collection was null/empty");
+		} else {
+			for (RangerPolicyItem policyItem : policyItems) {
+				if (policyItem == null) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("policy item")
+						.isMissing()
+						.becauseOf("policy items object was null")
+						.build());
+					valid = false;
+				} else {
+					// we want to go through all elements even though one may be bad so all failures are captured
+					valid = isValidPolicyItem(policyItem, failures, serviceDef) && valid;
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s, %s): %s", policyItems, failures, serviceDef, valid));
+		}
+		return valid;
+	}
+
+	boolean isValidPolicyItem(RangerPolicyItem policyItem, List<ValidationFailureDetails> failures, RangerServiceDef serviceDef) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValid(%s, %s, %s)", policyItem, failures, serviceDef));
+		}
+		
+		boolean valid = true;
+		if (policyItem == null) {
+			LOG.debug("policy item was null!");
+		} else {
+			// access items collection can't be empty and should be otherwise valid
+			if (CollectionUtils.isEmpty(policyItem.getAccesses())) {
+				failures.add(new ValidationFailureDetailsBuilder()
+					.field("policy item accesses")
+					.isMissing()
+					.becauseOf("policy items accesses collection was null")
+					.build());
+				valid = false;
+			} else {
+				valid = isValidItemAccesses(policyItem.getAccesses(), failures, serviceDef) && valid;
+			}
+			// both users and user-groups collections can't be empty
+			if (CollectionUtils.isEmpty(policyItem.getUsers()) && CollectionUtils.isEmpty(policyItem.getGroups())) {
+				failures.add(new ValidationFailureDetailsBuilder()
+					.field("policy item users/user-groups")
+					.isMissing()
+					.becauseOf("both users and user-groups collections on the policy item were null/empty")
+					.build());
+				valid = false;
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s, %s): %s", policyItem, failures, serviceDef, valid));
+		}
+		return valid;
+	}
+
+	boolean isValidItemAccesses(List<RangerPolicyItemAccess> accesses, List<ValidationFailureDetails> failures, RangerServiceDef serviceDef) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValid(%s, %s, %s)", accesses, failures, serviceDef));
+		}
+		
+		boolean valid = true;
+		if (CollectionUtils.isEmpty(accesses)) {
+			LOG.debug("policy item accesses collection was null/empty!");
+		} else {
+			Set<String> accessTypes = getAccessTypes(serviceDef);
+			for (RangerPolicyItemAccess access : accesses) {
+				if (access == null) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("policy item access")
+						.isMissing()
+						.becauseOf("policy items access object was null")
+						.build());
+					valid = false;
+				} else {
+					// we want to go through all elements even though one may be bad so all failures are captured
+					valid = isValidPolicyItemAccess(access, failures, accessTypes) && valid;
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValid(%s, %s): %s", accesses, failures, serviceDef, valid));
+		}
+		return valid;
+	}
+
+	boolean isValidPolicyItemAccess(RangerPolicyItemAccess access, List<ValidationFailureDetails> failures, Set<String> accessTypes) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerPolicyValidator.isValidPolicyItemAccess(%s, %s, %s)", access, failures, accessTypes));
+		}
+		
+		boolean valid = true;
+		if (CollectionUtils.isEmpty(accessTypes)) { // caller should firewall this argument!
+			LOG.debug("isValidPolicyItemAccess: accessTypes was null!");
+		} else if (access == null) {
+			LOG.debug("isValidPolicyItemAccess: policy item access was null!");
+		} else {
+			String accessType = access.getType();
+			if (StringUtils.isBlank(accessType)) {
+				failures.add(new ValidationFailureDetailsBuilder()
+					.field("policy item access type")
+					.isMissing()
+					.becauseOf("policy items access type's name was null/empty/blank")
+					.build());
+				valid = false;
+			} else {
+				if (!accessTypes.contains(accessType)) {
+					String message = String.format("access type[%s] not among valid types for service[%s]", accessType, accessTypes);
+					LOG.debug(message);
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("policy item access type")
+						.isSemanticallyIncorrect()
+						.becauseOf(message)
+						.build());
+					valid = false;
+				}
+			}
+			Boolean isAllowed = access.getIsAllowed();
+			// it can be null (which is treated as allowed) but not false
+			if (isAllowed != null && isAllowed == false) {
+				String message = "access type is set to deny.  Currently deny access types are not supported.";
+				LOG.debug(message);
+				failures.add(new ValidationFailureDetailsBuilder()
+					.field("policy item access type allowed")
+					.isSemanticallyIncorrect()
+					.becauseOf(message)
+					.build());
+				valid = false;
+			}
+		}
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerPolicyValidator.isValidPolicyItemAccess(%s, %s, %s): %s", access, failures, accessTypes, valid));
+		}
+		return valid;
+	}
+}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/main/java/org/apache/ranger/rest/RangerServiceValidator.java
----------------------------------------------------------------------
diff --git a/security-admin/src/main/java/org/apache/ranger/rest/RangerServiceValidator.java b/security-admin/src/main/java/org/apache/ranger/rest/RangerServiceValidator.java
index 08184c7..11e2682 100644
--- a/security-admin/src/main/java/org/apache/ranger/rest/RangerServiceValidator.java
+++ b/security-admin/src/main/java/org/apache/ranger/rest/RangerServiceValidator.java
@@ -19,6 +19,8 @@
 
 package org.apache.ranger.rest;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Set;
 
 import org.apache.commons.lang.StringUtils;
@@ -34,135 +36,147 @@ public class RangerServiceValidator extends RangerValidator {
 
 	private static final Log LOG = LogFactory.getLog(RangerServiceValidator.class);
 
-	public RangerServiceValidator(ServiceStore store, Action action) {
-		super(store, action);
+	public RangerServiceValidator(ServiceStore store) {
+		super(store);
 	}
 
-	public void validate(RangerService service) throws Exception {
+	public void validate(RangerService service, Action action) throws Exception {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug("==> RangerValidator.validate(" + service + ")");
 		}
 
-		if (isValid(service)) {
+		List<ValidationFailureDetails> failures = new ArrayList<ValidationFailureDetails>();
+		if (isValid(service, action, failures)) {
 			if(LOG.isDebugEnabled()) {
 				LOG.debug("<== RangerValidator.validate(" + service + "): valid");
 			}
 		} else {
-			String message = getFailureMessage();
+			String message = serializeFailures(failures);
 			LOG.debug("<== RangerValidator.validate(" + service + "): invalid, reason[" + message + "]");
 			throw new Exception(message);
 		}
 	}
 	
-	public void validate(long id) throws Exception {
-		if (isValid(id)) {
-			if(LOG.isDebugEnabled()) {
-				LOG.debug("<== RangerValidator.validate(" + id + "): valid");
-			}
-		} else {
-			String message = getFailureMessage();
-			LOG.debug("<== RangerValidator.validate(" + id + "): invalid, reason[" + message + "]");
-			throw new Exception(message);
-		}
-	}
-	
-	boolean isValid(Long id) {
+	boolean isValid(Long id, Action action, List<ValidationFailureDetails> failures) {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug("==> RangerServiceValidator.isValid(" + id + ")");
 		}
 
-		if (_action != Action.DELETE) {
-			addFailure(new ValidationFailureDetailsBuilder()
+		boolean valid = true;
+		if (action != Action.DELETE) {
+			failures.add(new ValidationFailureDetailsBuilder()
 				.isAnInternalError()
 				.becauseOf("isValid(Long) is only supported for DELETE")
 				.build());
+			valid = false;
 		} else if (id == null) {
-			addFailure(new ValidationFailureDetailsBuilder()
+			failures.add(new ValidationFailureDetailsBuilder()
 				.field("id")
 				.isMissing()
 				.build());
-		} else {
-			boolean found = false;
-			try {
-				if (_store.getService(id) != null) {
-					found = true;
-				}
-			} catch (Exception e) {
-				LOG.debug("Encountred exception while retrieving service from service store!", e);
-			}
-			if (!found) {
-				addFailure(new ValidationFailureDetailsBuilder()
-					.field("id")
-					.isSemanticallyIncorrect()
-					.becauseOf("no service found for id[" + id + "]")
-					.build());
-			}
+			valid = false;
+		} else if (getService(id) == null) {
+			failures.add(new ValidationFailureDetailsBuilder()
+				.field("id")
+				.isSemanticallyIncorrect()
+				.becauseOf("no service found for id[" + id + "]")
+				.build());
+			valid = false;
 		}
 
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("<== RangerServiceValidator.isValid(" + id + "): " + _valid);
+			LOG.debug("<== RangerServiceValidator.isValid(" + id + "): " + valid);
 		}
-		return _valid;
+		return valid;
 	}
 	
-	boolean isValid(RangerService service) {
+	boolean isValid(RangerService service, Action action, List<ValidationFailureDetails> failures) {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug("==> RangerServiceValidator.isValid(" + service + ")");
 		}
+		if (!(action == Action.CREATE || action == Action.UPDATE)) {
+			throw new IllegalArgumentException("isValid(RangerService, ...) is only supported for CREATE/UPDATE");
+		}
 		
+		boolean valid = true;
 		if (service == null) {
 			String message = "service object passed in was null";
 			LOG.debug(message);
-			addFailure(new ValidationFailureDetailsBuilder()
+			failures.add(new ValidationFailureDetailsBuilder()
 				.field("service")
 				.isMissing()
 				.becauseOf(message)
 				.build());
+			valid = false;
 		} else {
+			Long id = service.getId();
+			if (action == Action.UPDATE) { // id is ignored for CREATE
+				if (id == null) {
+					String message = "service id was null/empty/blank"; 
+					LOG.debug(message);
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("id")
+						.isMissing()
+						.becauseOf(message)
+						.build());
+					valid = false;
+				} else if (getService(id) == null) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("id")
+						.isSemanticallyIncorrect()
+						.becauseOf("no service exists with id[" + id +"]")
+						.build());
+					valid = false;
+				}
+			}
 			String name = service.getName();
-			String type = service.getType();
 			boolean nameSpecified = StringUtils.isNotBlank(name);
-			boolean typeSpecified = StringUtils.isNotBlank(type);
-			RangerService existingService = null;
 			RangerServiceDef serviceDef = null;
 			if (!nameSpecified) {
-				String message = "service name was null/empty/blank"; 
+				String message = "service name was null/empty/blank[" + name + "]"; 
 				LOG.debug(message);
-				addFailure(new ValidationFailureDetailsBuilder()
+				failures.add(new ValidationFailureDetailsBuilder()
 					.field("name")
 					.isMissing()
 					.becauseOf(message)
 					.build());
+				valid = false;
 			} else {
-				existingService = getService(name);
-				if (existingService != null && _action == Action.CREATE) {
-					addFailure(new ValidationFailureDetailsBuilder()
+				RangerService otherService = getService(name);
+				if (otherService != null && action == Action.CREATE) {
+					failures.add(new ValidationFailureDetailsBuilder()
 						.field("name")
 						.isSemanticallyIncorrect()
-						.becauseOf("service with the same name already exists")
+						.becauseOf("service already exists with name[" + name + "]")
 						.build());
-				} else if (existingService == null && _action == Action.UPDATE) {
-					addFailure(new ValidationFailureDetailsBuilder()
-						.field("name")
+					valid = false;
+				} else if (otherService != null && otherService.getId() !=null && otherService.getId() != id) {
+					failures.add(new ValidationFailureDetailsBuilder()
+						.field("id/name")
 						.isSemanticallyIncorrect()
-						.becauseOf("service with the same name doesn't exist")
+						.becauseOf("id/name conflict: service already exists with name[" + name + "], its id is [" + otherService.getId() + "]")
 						.build());
+					valid = false;
 				}
 			}
+			String type = service.getType();
+			boolean typeSpecified = StringUtils.isNotBlank(type);
 			if (!typeSpecified) {
-				addFailure(new ValidationFailureDetailsBuilder()
+				failures.add(new ValidationFailureDetailsBuilder()
 					.field("type")
 					.isMissing()
 					.becauseOf("service def was null/empty/blank")
 					.build());
+				valid = false;
 			} else {
 				serviceDef = getServiceDef(type);
 				if (serviceDef == null) {
-					addFailure(new ValidationFailureDetailsBuilder()
+					failures.add(new ValidationFailureDetailsBuilder()
 						.field("type")
 						.isSemanticallyIncorrect()
-						.becauseOf("service def not found")
+						.becauseOf("service def not found for type[" + type + "]")
 						.build());
+					valid = false;
 				}
 			}
 			if (nameSpecified && serviceDef != null) {
@@ -170,19 +184,20 @@ public class RangerServiceValidator extends RangerValidator {
 				Set<String> inputParameters = getServiceConfigParameters(service);
 				Set<String> missingParameters = Sets.difference(reqiredParameters, inputParameters);
 				if (!missingParameters.isEmpty()) {
-					addFailure(new ValidationFailureDetailsBuilder()
+					failures.add(new ValidationFailureDetailsBuilder()
 						.field("configuration")
 						.subField(missingParameters.iterator().next()) // we return any one parameter!
 						.isMissing()
 						.becauseOf("required configuration parameter is missing")
 						.build());
+					valid = false;
 				}
 			}
 		}
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("<== RangerServiceValidator.isValid(" + service + "): " + _valid);
+			LOG.debug("<== RangerServiceValidator.isValid(" + service + "): " + valid);
 		}
-		return _valid;
+		return valid;
 	}
 }

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/main/java/org/apache/ranger/rest/RangerValidator.java
----------------------------------------------------------------------
diff --git a/security-admin/src/main/java/org/apache/ranger/rest/RangerValidator.java b/security-admin/src/main/java/org/apache/ranger/rest/RangerValidator.java
index 3f25266..b6948dc 100644
--- a/security-admin/src/main/java/org/apache/ranger/rest/RangerValidator.java
+++ b/security-admin/src/main/java/org/apache/ranger/rest/RangerValidator.java
@@ -19,75 +19,100 @@
 
 package org.apache.ranger.rest;
 
+
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang.StringUtils;
 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.RangerService;
 import org.apache.ranger.plugin.model.RangerServiceDef;
+import org.apache.ranger.plugin.model.RangerServiceDef.RangerAccessTypeDef;
+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.SearchFilter;
 
 public abstract class RangerValidator {
 	
 	private static final Log LOG = LogFactory.getLog(RangerValidator.class);
 
 	ServiceStore _store;
-	boolean _valid = true;
-	List<ValidationFailureDetails> _failures;
-	Action _action;
 
 	public enum Action {
 		CREATE, UPDATE, DELETE;
 	};
 	
-	protected RangerValidator(ServiceStore store, Action action) {
+	protected RangerValidator(ServiceStore store) {
 		if (store == null) {
 			throw new IllegalArgumentException("ServiceValidator(): store is null!");
 		}
 		_store = store;
-		if (action == null) {
-			throw new IllegalArgumentException("ServiceValidator(): action is null!");
-		}
-		_action = action;
 	}
 
-	protected List<ValidationFailureDetails> getFailures() {
-		if (_valid) {
-			LOG.warn("getFailureDetails: called while _valid == true");
+	public void validate(Long id, Action action) throws Exception {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.validate(" + id + ")");
+		}
+
+		List<ValidationFailureDetails> failures = new ArrayList<ValidationFailureDetails>();
+		if (isValid(id, action, failures)) {
+			if(LOG.isDebugEnabled()) {
+				LOG.debug("<== RangerValidator.validate(" + id + "): valid");
+			}
+		} else {
+			String message = serializeFailures(failures);
+			LOG.debug("<== RangerValidator.validate(" + id + "): invalid, reason[" + message + "]");
+			throw new Exception(message);
 		}
-		return _failures;
 	}
 	
-	String getFailureMessage() {
-		if (_valid) {
-			LOG.warn("getFailureDetails: called while validator is true!");
-		}
-		if (_failures == null) {
-			LOG.warn("getFailureDetails: called while list of failures is null!");
-			return null;
+	/**
+	 * This method is expected to be overridden by sub-classes.  Default implementation provided to not burden implementers from having to implement methods that they know would never be called. 
+	 * @param id
+	 * @param action
+	 * @param failures
+	 * @return
+	 */
+	boolean isValid(Long id, Action action, List<ValidationFailureDetails> failures) {
+		failures.add(new ValidationFailureDetailsBuilder()
+				.isAnInternalError()
+				.becauseOf("unimplemented method called")
+				.build());
+		return false;
+	}
+
+	String serializeFailures(List<ValidationFailureDetails> failures) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getFailureMessage()");
 		}
-		StringBuilder builder = new StringBuilder();
-		for (ValidationFailureDetails aFailure : _failures) {
-			builder.append(aFailure.toString());
-			builder.append(";");
+
+		String message = null;
+		if (CollectionUtils.isEmpty(failures)) {
+			LOG.warn("serializeFailures: called while list of failures is null/empty!");
+		} else {
+			StringBuilder builder = new StringBuilder();
+			for (ValidationFailureDetails aFailure : failures) {
+				builder.append(aFailure.toString());
+				builder.append(";");
+			}
+			message = builder.toString();
 		}
-		return builder.toString();
-	}
 
-	void addFailure(ValidationFailureDetails aFailure) {
-		if (_failures == null) {
-			_failures = new ArrayList<ValidationFailureDetails>();
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.serializeFailures(): " + message);
 		}
-		_failures.add(aFailure);
-		_valid = false;
+		return message;
 	}
-	
+
 	Set<String> getServiceConfigParameters(RangerService service) {
 		if (service == null || service.getConfigs() == null) {
 			return new HashSet<String>();
@@ -98,7 +123,7 @@ public abstract class RangerValidator {
 
 	Set<String> getRequiredParameters(RangerServiceDef serviceDef) {
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("==> RangerServiceValidator.getRequiredParameters(" + serviceDef + ")");
+			LOG.debug("==> RangerValidator.getRequiredParameters(" + serviceDef + ")");
 		}
 
 		Set<String> result;
@@ -119,7 +144,7 @@ public abstract class RangerValidator {
 		}
 
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("<== RangerServiceValidator.getRequiredParameters(" + serviceDef + "): " + result);
+			LOG.debug("<== RangerValidator.getRequiredParameters(" + serviceDef + "): " + result);
 		}
 		return result;
 	}
@@ -127,7 +152,7 @@ public abstract class RangerValidator {
 	RangerServiceDef getServiceDef(String type) {
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("==> RangerServiceValidator.getServiceDef(" + type + ")");
+			LOG.debug("==> RangerValidator.getServiceDef(" + type + ")");
 		}
 		RangerServiceDef result = null;
 		try {
@@ -137,7 +162,25 @@ public abstract class RangerValidator {
 		}
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("<== RangerServiceValidator.getServiceDef(" + type + "): " + result);
+			LOG.debug("<== RangerValidator.getServiceDef(" + type + "): " + result);
+		}
+		return result;
+	}
+
+	RangerService getService(Long id) {
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getService(" + id + ")");
+		}
+		RangerService result = null;
+		try {
+			result = _store.getService(id);
+		} catch (Exception e) {
+			LOG.debug("Encountred exception while retrieving service from service store!", e);
+		}
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getService(" + id + "): " + result);
 		}
 		return result;
 	}
@@ -145,7 +188,7 @@ public abstract class RangerValidator {
 	RangerService getService(String name) {
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("==> RangerServiceValidator.getService(" + name + ")");
+			LOG.debug("==> RangerValidator.getService(" + name + ")");
 		}
 		RangerService result = null;
 		try {
@@ -155,8 +198,201 @@ public abstract class RangerValidator {
 		}
 		
 		if(LOG.isDebugEnabled()) {
-			LOG.debug("<== RangerServiceValidator.getService(" + name + "): " + result);
+			LOG.debug("<== RangerValidator.getService(" + name + "): " + result);
 		}
 		return result;
 	}
+
+	RangerPolicy getPolicy(Long id) {
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getPolicy(" + id + ")");
+		}
+		RangerPolicy result = null;
+		try {
+			result = _store.getPolicy(id);
+		} catch (Exception e) {
+			LOG.debug("Encountred exception while retrieving policy from service store!", e);
+		}
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getPolicy(" + id + "): " + result);
+		}
+		return result;
+	}
+
+	List<RangerPolicy> getPolicies(final String policyName, final String serviceName) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getPolicies(" + policyName + ", " + serviceName + ")");
+		}
+
+		List<RangerPolicy> policies = null;
+		try {
+			SearchFilter filter = new SearchFilter();
+			filter.setParam(SearchFilter.POLICY_NAME, policyName);
+			filter.setParam(SearchFilter.SERVICE_NAME, serviceName);
+			
+			policies = _store.getPolicies(filter);
+		} catch (Exception e) {
+			LOG.debug("Encountred exception while retrieving service from service store!", e);
+		}
+		
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getPolicies(" + policyName + ", " + serviceName + "): " + policies);
+		}
+		return policies;
+	}
+
+	Set<String> getAccessTypes(RangerServiceDef serviceDef) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getAccessTypes(" + serviceDef + ")");
+		}
+
+		Set<String> accessTypes = new HashSet<String>();
+		if (serviceDef == null) {
+			LOG.warn("serviceDef passed in was null!");
+		} else if (CollectionUtils.isEmpty(serviceDef.getAccessTypes())) {
+			LOG.warn("AccessTypeDef collection on serviceDef was null!");
+		} else {
+			for (RangerAccessTypeDef accessTypeDef : serviceDef.getAccessTypes()) {
+				if (accessTypeDef == null) {
+					LOG.warn("Access type def was null!");
+				} else {
+					String accessType = accessTypeDef.getName();
+					if (StringUtils.isBlank(accessType)) {
+						LOG.warn("Access type def name was null/empty/blank!");
+					} else {
+						accessTypes.add(accessType);
+					}
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getAccessTypes(" + serviceDef + "): " + accessTypes);
+		}
+		return accessTypes;
+	}
+	
+	/**
+	 * This function exists to encapsulates the current behavior of code which treats and unspecified audit preference to mean audit is enabled.
+	 * @param policy
+	 * @return
+	 */
+	boolean getIsAuditEnabled(RangerPolicy policy) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getIsAuditEnabled(" + policy + ")");
+		}
+
+		boolean isEnabled = false;
+		if (policy == null) {
+			LOG.warn("policy was null!");
+		} else if (policy.getIsAuditEnabled() == null) {
+			isEnabled = true;
+		} else {
+			isEnabled = policy.getIsAuditEnabled();
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getIsAuditEnabled(" + policy + "): " + isEnabled);
+		}
+		return isEnabled;
+	}
+	
+	Set<String> getMandatoryResourceNames(RangerServiceDef serviceDef) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getMandatoryResourceNames(" + serviceDef + ")");
+		}
+
+		Set<String> resourceNames = new HashSet<String>();
+		if (serviceDef == null) {
+			LOG.warn("serviceDef passed in was null!");
+		} else if (CollectionUtils.isEmpty(serviceDef.getResources())) {
+			LOG.warn("ResourceDef collection on serviceDef was null!");
+		} else {
+			for (RangerResourceDef resourceTypeDef : serviceDef.getResources()) {
+				if (resourceTypeDef == null) {
+					LOG.warn("resource type def was null!");
+				} else {
+					Boolean mandatory = resourceTypeDef.getMandatory();
+					if (mandatory != null && mandatory == true) {
+						String resourceName = resourceTypeDef.getName();
+						if (StringUtils.isBlank(resourceName)) {
+							LOG.warn("Resource def name was null/empty/blank!");
+						} else {
+							resourceNames.add(resourceName);
+						}
+					}
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getMandatoryResourceNames(" + serviceDef + "): " + resourceNames);
+		}
+		return resourceNames;
+	}
+
+	Set<String> getAllResourceNames(RangerServiceDef serviceDef) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("==> RangerValidator.getAllResourceNames(" + serviceDef + ")");
+		}
+
+		Set<String> resourceNames = new HashSet<String>();
+		if (serviceDef == null) {
+			LOG.warn("serviceDef passed in was null!");
+		} else if (CollectionUtils.isEmpty(serviceDef.getResources())) {
+			LOG.warn("ResourceDef collection on serviceDef was null!");
+		} else {
+			for (RangerResourceDef resourceTypeDef : serviceDef.getResources()) {
+				if (resourceTypeDef == null) {
+					LOG.warn("resource type def was null!");
+				} else {
+					String resourceName = resourceTypeDef.getName();
+					if (StringUtils.isBlank(resourceName)) {
+						LOG.warn("Resource def name was null/empty/blank!");
+					} else {
+						resourceNames.add(resourceName);
+					}
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug("<== RangerValidator.getAllResourceNames(" + serviceDef + "): " + resourceNames);
+		}
+		return resourceNames;
+	}
+	
+	Set<String> getPolicyResources(RangerPolicy policy) {
+		if (policy == null || policy.getResources() == null || policy.getResources().isEmpty()) {
+			return new HashSet<String>();
+		} else {
+			return policy.getResources().keySet();
+		}
+	}
+
+	Map<String, String> getValidationRegExes(RangerServiceDef serviceDef) {
+		if (serviceDef == null || CollectionUtils.isEmpty(serviceDef.getResources())) {
+			return new HashMap<String, String>();
+		} else {
+			Map<String, String> result = new HashMap<String, String>();
+			for (RangerResourceDef resourceDef : serviceDef.getResources()) {
+				if (resourceDef == null) {
+					LOG.warn("A resource def in resource def collection is null");
+				} else {
+					String name = resourceDef.getName();
+					String regEx = resourceDef.getValidationRegEx();
+					if (StringUtils.isBlank(name)) {
+						LOG.warn("resource name is null/empty/blank");
+					} else if (StringUtils.isBlank(regEx)) {
+						LOG.debug("validation regex is null/empty/blank");
+					} else {
+						result.put(name, regEx);
+					}
+				}
+			}
+			return result;
+		}
+	}
 }

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/main/java/org/apache/ranger/rest/RangerValidatorFactory.java
----------------------------------------------------------------------
diff --git a/security-admin/src/main/java/org/apache/ranger/rest/RangerValidatorFactory.java b/security-admin/src/main/java/org/apache/ranger/rest/RangerValidatorFactory.java
index 6c75a2f..01b0a7e 100644
--- a/security-admin/src/main/java/org/apache/ranger/rest/RangerValidatorFactory.java
+++ b/security-admin/src/main/java/org/apache/ranger/rest/RangerValidatorFactory.java
@@ -19,11 +19,15 @@
 
 package org.apache.ranger.rest;
 
+import org.apache.ranger.biz.ServiceDBStore;
 import org.apache.ranger.plugin.store.ServiceStore;
-import org.apache.ranger.rest.RangerValidator.Action;
 
 public class RangerValidatorFactory {
-	RangerServiceValidator getServiceValidator(ServiceStore store, Action action) {
-		return new RangerServiceValidator(store, action);
+	public RangerServiceValidator getServiceValidator(ServiceStore store) {
+		return new RangerServiceValidator(store);
+	}
+
+	public RangerPolicyValidator getPolicyValidator(ServiceDBStore store) {
+		return new RangerPolicyValidator(store);
 	}
 }

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
----------------------------------------------------------------------
diff --git a/security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java b/security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
index 821562f..e127b35 100644
--- a/security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
+++ b/security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
@@ -45,6 +45,13 @@ import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.apache.ranger.admin.client.datatype.RESTResponse;
+import org.apache.ranger.biz.AssetMgr;
+import org.apache.ranger.biz.ServiceDBStore;
+import org.apache.ranger.biz.ServiceMgr;
+import org.apache.ranger.common.RESTErrorUtil;
+import org.apache.ranger.common.ServiceUtil;
+import org.apache.ranger.entity.XXPolicyExportAudit;
 import org.apache.ranger.plugin.model.RangerPolicy;
 import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyItem;
 import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyItemAccess;
@@ -68,15 +75,6 @@ import org.springframework.security.access.prepost.PreAuthorize;
 import org.springframework.stereotype.Component;
 import org.springframework.transaction.annotation.Propagation;
 import org.springframework.transaction.annotation.Transactional;
-import org.apache.ranger.admin.client.datatype.RESTResponse;
-import org.apache.ranger.biz.AssetMgr;
-import org.apache.ranger.biz.ServiceMgr;
-import org.apache.ranger.biz.ServiceDBStore;
-import org.apache.ranger.common.MessageEnums;
-import org.apache.ranger.common.PropertiesUtil;
-import org.apache.ranger.common.RESTErrorUtil;
-import org.apache.ranger.common.ServiceUtil;
-import org.apache.ranger.entity.XXPolicyExportAudit;
 
 
 @Path("plugins")
@@ -281,8 +279,8 @@ public class ServiceREST {
 		RangerService ret = null;
 
 		try {
-			RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore, Action.CREATE);
-			validator.validate(service);
+			RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore);
+			validator.validate(service, Action.CREATE);
 			
 			ret = svcStore.createService(service);
 		} catch(Exception excp) {
@@ -310,8 +308,8 @@ public class ServiceREST {
 		RangerService ret = null;
 
 		try {
-			RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore, Action.UPDATE);
-			validator.validate(service);
+			RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore);
+			validator.validate(service, Action.UPDATE);
 			ret = svcStore.updateService(service);
 		} catch(Exception excp) {
 			LOG.error("updateService(" + service + ") failed", excp);
@@ -336,8 +334,8 @@ public class ServiceREST {
 		}
 
 		try {
-			RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore, Action.DELETE);
-			validator.validate(id);
+			RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore);
+			validator.validate(id, Action.DELETE);
 			svcStore.deleteService(id);
 		} catch(Exception excp) {
 			LOG.error("deleteService(" + id + ") failed", excp);
@@ -795,6 +793,8 @@ public class ServiceREST {
 		RangerPolicy ret = null;
 
 		try {
+			RangerPolicyValidator validator = validatorFactory.getPolicyValidator(svcStore);
+			validator.validate(policy, Action.CREATE);
 			ret = svcStore.createPolicy(policy);
 		} catch(Exception excp) {
 			LOG.error("createPolicy(" + policy + ") failed", excp);
@@ -820,6 +820,8 @@ public class ServiceREST {
 		RangerPolicy ret = null;
 
 		try {
+			RangerPolicyValidator validator = validatorFactory.getPolicyValidator(svcStore);
+			validator.validate(policy, Action.UPDATE);
 			ret = svcStore.updatePolicy(policy);
 		} catch(Exception excp) {
 			LOG.error("updatePolicy(" + policy + ") failed", excp);
@@ -843,6 +845,8 @@ public class ServiceREST {
 		}
 
 		try {
+			RangerPolicyValidator validator = validatorFactory.getPolicyValidator(svcStore);
+			validator.validate(id, Action.DELETE);
 			svcStore.deletePolicy(id);
 		} catch(Exception excp) {
 			LOG.error("deletePolicy(" + id + ") failed", excp);

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/test/java/org/apache/ranger/rest/TestRangerPolicyValidator.java
----------------------------------------------------------------------
diff --git a/security-admin/src/test/java/org/apache/ranger/rest/TestRangerPolicyValidator.java b/security-admin/src/test/java/org/apache/ranger/rest/TestRangerPolicyValidator.java
new file mode 100644
index 0000000..4e15753
--- /dev/null
+++ b/security-admin/src/test/java/org/apache/ranger/rest/TestRangerPolicyValidator.java
@@ -0,0 +1,471 @@
+package org.apache.ranger.rest;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.ranger.plugin.model.RangerPolicy;
+import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyItem;
+import org.apache.ranger.plugin.model.RangerPolicy.RangerPolicyItemAccess;
+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.store.ServiceStore;
+import org.apache.ranger.plugin.util.SearchFilter;
+import org.apache.ranger.rest.RangerValidator.Action;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Sets;
+
+public class TestRangerPolicyValidator {
+
+	@Before
+	public void setUp() throws Exception {
+		_store = mock(ServiceStore.class);
+		_policy = mock(RangerPolicy.class);
+		_validator = new RangerPolicyValidator(_store);
+		_serviceDef = mock(RangerServiceDef.class);
+	}
+	
+	final Action[] cu = new Action[] { Action.CREATE, Action.UPDATE };
+	final Object[] policyItemsData = new Object[] {
+			ImmutableMap.of(  // all good
+				"users", new String[] {"user1" ," user2"},
+				"groups", new String[] {"group1", "group2"},
+				"accesses", new String[] { "r", "w" },
+				"isAllowed", new Boolean[] { true, true }),
+			ImmutableMap.of(   // no users
+				"groups", new String[] {"group3", "group4"},
+				"accesses", new String[]{"w", "x"}, 
+				"isAllowed", new Boolean[] { true, true }),
+			ImmutableMap.of(   // no groups
+				"users", new String[] {"user3" ," user4"}, 
+				"accesses", new String[] { "r", "x" },
+				"isAllowed", new Boolean[] { true, true }),
+			ImmutableMap.of( // isallowed on access types is null
+				"users", new String[] {"user7" ," user6"},
+				"accesses", new String[] { "a" },
+				"isAllowed", new Boolean[] { null, null })
+	};
+	String[] accessTypes = new String[] { "r", "w", "x", "a" };
+	String[] accessTypes_bad = new String[] { "r", "w", "xx", }; // two missing (x, a), one new that isn't on bad (xx)
+	
+	final Object[][] resourceDefData = new Object[][] {
+			{ "db", true, "db\\d+" },        // valid: db1, db22, db983, etc.; invalid: db, db12x, ttx11, etc.
+			{ "tbl", true, null },           // anything goes
+			{ "col", false, "col\\d{1,2}" }  // valid: col1, col47, etc.; invalid: col, col238, col1, etc.
+	};
+	
+	final Map<String, String[]> policyResourceMap_good = ImmutableMap.of(
+			"db", new String[] { "db1", "db2" },
+			"tbl", new String[] { "tbl1", "tbl2" } );
+	final Map<String, String[]> policyResourceMap_bad = ImmutableMap.of(
+			"db", new String[] { "db1", "db2" },            // mandatory "tbl" missing
+			"col", new String[] { "col12", "col 1" },       // wrong format of value for "col"
+			"extra", new String[] { "extra1", "extra2" } ); // spurious "extra" specified
+
+	@Test
+	public final void testIsValid_long() throws Exception {
+		// this validation should be removed if we start supporting other than delete action
+		assertFalse(_validator.isValid(3L, Action.CREATE, _failures));
+		_utils.checkFailureForInternalError(_failures);
+		
+		// should fail with appropriate error message if id is null
+		_failures.clear(); _failures.clear(); assertFalse(_validator.isValid((Long)null, Action.DELETE, _failures));
+		_utils.checkFailureForMissingValue(_failures, "id");
+		
+		// should fail with appropriate error message if policy can't be found for the specified id
+		when(_store.getPolicy(1L)).thenReturn(null);
+		when(_store.getPolicy(2L)).thenThrow(new Exception());
+		RangerPolicy existingPolicy = mock(RangerPolicy.class);
+		when(_store.getPolicy(3L)).thenReturn(existingPolicy);
+		_failures.clear(); assertFalse(_validator.isValid(1L, Action.DELETE, _failures));
+		_utils.checkFailureForSemanticError(_failures, "id");
+		_failures.clear(); assertFalse(_validator.isValid(2L, Action.DELETE, _failures));
+		_utils.checkFailureForSemanticError(_failures, "id");
+
+		// if policy exists then delete validation should pass 
+		assertTrue(_validator.isValid(3L, Action.DELETE, _failures));
+	}
+	
+	@Test
+	public final void testIsValid_happyPath() throws Exception {
+		// valid policy has valid non-empty name and service name 
+		when(_policy.getService()).thenReturn("service-name");
+		// service name exists
+		RangerService service = mock(RangerService.class);
+		when(service.getType()).thenReturn("service-type");
+		when(_store.getServiceByName("service-name")).thenReturn(service);
+		// service points to a valid service-def
+		_serviceDef = _utils.createServiceDefWithAccessTypes(accessTypes);
+		when(_store.getServiceDefByName("service-type")).thenReturn(_serviceDef);
+		// a matching policy should exist for create when checked by id and not exist when checked by name.
+		when(_store.getPolicy(7L)).thenReturn(null);
+		RangerPolicy existingPolicy = mock(RangerPolicy.class);
+		when(existingPolicy.getId()).thenReturn(8L);
+		when(_store.getPolicy(8L)).thenReturn(existingPolicy);
+		SearchFilter createFilter = new SearchFilter();
+		createFilter.setParam(SearchFilter.POLICY_NAME, "service-type");
+		createFilter.setParam(SearchFilter.POLICY_NAME, "policy-name-1"); // this name would be used for create
+		when(_store.getPolicies(createFilter)).thenReturn(new ArrayList<RangerPolicy>());
+		// a matching policy should not exist for update.
+		SearchFilter updateFilter = new SearchFilter();
+		updateFilter.setParam(SearchFilter.POLICY_NAME, "service-type");
+		updateFilter.setParam(SearchFilter.POLICY_NAME, "policy-name-2"); // this name would be used for update
+		List<RangerPolicy> existingPolicies = new ArrayList<RangerPolicy>();
+		existingPolicies.add(existingPolicy);
+		when(_store.getPolicies(updateFilter)).thenReturn(existingPolicies);
+		// valid policy can have empty set of policy items if audit is turned on
+		// null value for audit is treated as audit on.
+		for (Action action : cu) {
+			for (Boolean auditEnabled : new Boolean[] { null, true } ) {
+				when(_policy.getIsAuditEnabled()).thenReturn(auditEnabled);
+				if (action == Action.CREATE) {
+					when(_policy.getId()).thenReturn(7L);
+					when(_policy.getName()).thenReturn("policy-name-1");
+					assertTrue("" + action + ", " + auditEnabled, _validator.isValid(_policy, action, _failures));
+					assertTrue(_failures.isEmpty());
+				} else {
+					// update should work both when by-name is found or not, since nothing found by-name means name is being updated.
+					when(_policy.getId()).thenReturn(8L);
+					when(_policy.getName()).thenReturn("policy-name-1");
+					assertTrue("" + action + ", " + auditEnabled, _validator.isValid(_policy, action, _failures));
+					assertTrue(_failures.isEmpty());
+
+					when(_policy.getName()).thenReturn("policy-name-2");
+					assertTrue("" + action + ", " + auditEnabled, _validator.isValid(_policy, action, _failures));
+					assertTrue(_failures.isEmpty());
+				}
+			}
+		}
+		// if audit is disabled then policy should have policy items and all of them should be valid
+		List<RangerPolicyItem> policyItems = _utils.createPolicyItems(policyItemsData);
+		when(_policy.getPolicyItems()).thenReturn(policyItems);
+		when(_policy.getIsAuditEnabled()).thenReturn(false);
+		for (Action action : cu) {
+			if (action == Action.CREATE) {
+				when(_policy.getId()).thenReturn(7L);
+				when(_policy.getName()).thenReturn("policy-name-1");
+			} else {
+				when(_policy.getId()).thenReturn(8L);
+				when(_policy.getName()).thenReturn("policy-name-2");
+			}
+			assertTrue("" + action , _validator.isValid(_policy, action, _failures));
+			assertTrue(_failures.isEmpty());
+		}
+		
+		// above succeeded as service def did not have any resources on it, mandatory or otherwise.
+		// policy should have all mandatory resources specified, and they should conform to the validation pattern in resource definition
+		List<RangerResourceDef> resourceDefs = _utils.createResourceDefs(resourceDefData);
+		when(_serviceDef.getResources()).thenReturn(resourceDefs);
+		Map<String, RangerPolicyResource> resourceMap = _utils.createPolicyResourceMap(policyResourceMap_good);
+		when(_policy.getResources()).thenReturn(resourceMap);
+
+		for (Action action : cu) {
+			if (action == Action.CREATE) {
+				when(_policy.getId()).thenReturn(7L);
+				when(_policy.getName()).thenReturn("policy-name-1");
+			} else {
+				when(_policy.getId()).thenReturn(8L);
+				when(_policy.getName()).thenReturn("policy-name-2");
+			}
+			assertTrue("" + action , _validator.isValid(_policy, action, _failures));
+			assertTrue(_failures.isEmpty());
+		}
+	}
+	
+	void checkFailure_isValid(Action action, String errorType, String field) {
+		checkFailure_isValid(action, errorType, field, null);
+	}
+	
+	void checkFailure_isValid(Action action, String errorType, String field, String subField) {
+		_failures.clear();
+		assertFalse(_validator.isValid(_policy, action, _failures));
+		switch (errorType) {
+		case "missing":
+			_utils.checkFailureForMissingValue(_failures, field, subField);
+			break;
+		case "semantic":
+			_utils.checkFailureForSemanticError(_failures, field, subField);
+			break;
+		case "internal error":
+			_utils.checkFailureForInternalError(_failures);
+			break;
+		default:
+			fail("Unsupported errorType[" + errorType + "]");
+			break;
+		}
+	}
+	
+	@Test
+	public final void testIsValid_failures() throws Exception {
+		for (Action action : cu) {
+			// passing in a null policy should fail with appropriate failure reason
+			_policy = null;
+			checkFailure_isValid(action, "missing", "policy");
+			
+			// policy must have a name on it
+			_policy = mock(RangerPolicy.class);
+			for (String name : new String[] { null, "  " }) {
+				when(_policy.getName()).thenReturn(name);
+				checkFailure_isValid(action, "missing", "name");
+			}
+			
+			// for update id is required!
+			if (action == Action.UPDATE) {
+				when(_policy.getId()).thenReturn(null);
+				checkFailure_isValid(action, "missing", "id");
+			}
+		}
+		/*
+		 * Id is ignored for Create but name should not belong to an existing policy.  For update, policy should exist for its id and should match its name.
+		 */
+		when(_policy.getName()).thenReturn("policy-name");
+		when(_policy.getService()).thenReturn("service-name");
+
+		RangerPolicy existingPolicy = mock(RangerPolicy.class);
+		when(existingPolicy.getId()).thenReturn(7L);
+		List<RangerPolicy> existingPolicies = new ArrayList<RangerPolicy>();
+		existingPolicies.add(existingPolicy);
+		SearchFilter filter = new SearchFilter();
+		filter.setParam(SearchFilter.SERVICE_NAME, "service-name");
+		filter.setParam(SearchFilter.POLICY_NAME, "policy-name");
+		when(_store.getPolicies(filter)).thenReturn(existingPolicies);
+		checkFailure_isValid(Action.CREATE, "semantic", "name");
+		
+		// update : does not exist for id
+		when(_policy.getId()).thenReturn(7L);
+		when(_store.getPolicy(7L)).thenReturn(null);
+		checkFailure_isValid(Action.UPDATE, "semantic", "id");
+
+		// Update: name should not point to an existing different policy, i.e. with a different id
+		when(_store.getPolicy(7L)).thenReturn(existingPolicy);
+		RangerPolicy anotherExistingPolicy = mock(RangerPolicy.class);
+		when(anotherExistingPolicy.getId()).thenReturn(8L);
+		existingPolicies.clear();
+		existingPolicies.add(anotherExistingPolicy);
+		when(_store.getPolicies(filter)).thenReturn(existingPolicies);
+		checkFailure_isValid(Action.UPDATE, "semantic", "id/name");
+
+		// more than one policies with same name is also an internal error
+		when(_policy.getName()).thenReturn("policy-name");
+		when(_store.getPolicies(filter)).thenReturn(existingPolicies);
+		existingPolicies.add(existingPolicy);
+		existingPolicy = mock(RangerPolicy.class);
+		existingPolicies.add(existingPolicy);
+		_failures.clear(); assertFalse(_validator.isValid(_policy, Action.UPDATE, _failures));
+		_utils.checkFailureForInternalError(_failures);
+		
+		// policy must have service name on it and it should be valid
+		when(_policy.getName()).thenReturn("policy-name");
+		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");
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "service");
+
+			when(_policy.getService()).thenReturn("another-service-name");
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "service");
+		}
+		
+		// policy must contain at least one policy item
+		List<RangerPolicyItem> policyItems = new ArrayList<RangerPolicy.RangerPolicyItem>();
+		when(_policy.getService()).thenReturn("service-name");
+		RangerService service = mock(RangerService.class);
+		when(_store.getServiceByName("service-name")).thenReturn(service);
+		for (Action action : cu) {
+			// when it is null
+			when(_policy.getPolicyItems()).thenReturn(null);
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "policy items");
+			// or when it is not null but empty.
+			when(_policy.getPolicyItems()).thenReturn(policyItems);
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "policy items");
+		}
+		
+		// these are known good policy items -- same as used above in happypath
+		policyItems = _utils.createPolicyItems(policyItemsData);
+		when(_policy.getPolicyItems()).thenReturn(policyItems);
+		// policy item check requires that service def should exist
+		when(service.getType()).thenReturn("service-type");
+		when(_store.getServiceDefByName("service-type")).thenReturn(null);
+		for (Action action : cu) {
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForInternalError(_failures, "policy service def");
+		}
+		
+		// service-def should contain the right access types on it.
+		_serviceDef = _utils.createServiceDefWithAccessTypes(accessTypes_bad);
+		when(_store.getServiceDefByName("service-type")).thenReturn(_serviceDef);
+		for (Action action : cu) {
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForSemanticError(_failures, "policy item access type");
+		}
+
+		// create the right service def with right resource defs - this is the same as in the happypath test above.
+		_serviceDef = _utils.createServiceDefWithAccessTypes(accessTypes);
+		when(_store.getPolicies(filter)).thenReturn(null);
+		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)
+		Map<String, RangerPolicyResource> policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad);
+		when(_policy.getResources()).thenReturn(policyResources);
+		for (Action action : cu) {
+			_failures.clear(); assertFalse(_validator.isValid(_policy, action, _failures));
+			_utils.checkFailureForMissingValue(_failures, "resources", "tbl"); // for missing resource: tbl
+			_utils.checkFailureForSemanticError(_failures, "resources", "extra"); // for spurious resource: "extra"
+			_utils.checkFailureForSemanticError(_failures, "resource-values", "col"); // for spurious resource: "extra"
+		}
+	}
+	
+	@Test
+	public void test_isValidResourceValues() {
+		List<RangerResourceDef> resourceDefs = _utils.createResourceDefs(resourceDefData);
+		when(_serviceDef.getResources()).thenReturn(resourceDefs);
+		Map<String, RangerPolicyResource> policyResources = _utils.createPolicyResourceMap(policyResourceMap_bad);
+		assertFalse(_validator.isValidResourceValues(policyResources, _failures, _serviceDef));
+		_utils.checkFailureForSemanticError(_failures, "resource-values", "col");
+		
+		policyResources = _utils.createPolicyResourceMap(policyResourceMap_good);
+		assertTrue(_validator.isValidResourceValues(policyResources, _failures, _serviceDef));
+	}
+	
+	@Test
+	public void test_isValidPolicyItems_failures() {
+		// null/empty list is good because there is nothing
+		assertTrue(_validator.isValidPolicyItems(null, _failures, _serviceDef));
+		_failures.isEmpty();
+
+		List<RangerPolicyItem> policyItems = new ArrayList<RangerPolicy.RangerPolicyItem>();
+		assertTrue(_validator.isValidPolicyItems(policyItems, _failures, _serviceDef));
+		_failures.isEmpty();
+		
+		// null elements in the list are flagged
+		policyItems.add(null);
+		assertFalse(_validator.isValidPolicyItems(policyItems, _failures, _serviceDef));
+		_utils.checkFailureForMissingValue(_failures, "policy item");
+	}
+	
+	@Test
+	public void test_isValidPolicyItem_failures() {
+
+		// empty access collections are invalid
+		RangerPolicyItem policyItem = mock(RangerPolicyItem.class);
+		when(policyItem.getAccesses()).thenReturn(null);
+		_failures.clear(); assertFalse(_validator.isValidPolicyItem(policyItem, _failures, _serviceDef));
+		_utils.checkFailureForMissingValue(_failures, "policy item accesses");
+
+		List<RangerPolicyItemAccess> accesses = new ArrayList<RangerPolicy.RangerPolicyItemAccess>();
+		when(policyItem.getAccesses()).thenReturn(accesses);
+		_failures.clear(); assertFalse(_validator.isValidPolicyItem(policyItem, _failures, _serviceDef));
+		_utils.checkFailureForMissingValue(_failures, "policy item accesses");
+		
+		// both user and groups can't be null
+		RangerPolicyItemAccess access = mock(RangerPolicyItemAccess.class);
+		accesses.add(access);
+		when(policyItem.getUsers()).thenReturn(null);
+		when(policyItem.getGroups()).thenReturn(new ArrayList<String>());
+		_failures.clear(); assertFalse(_validator.isValidPolicyItem(policyItem, _failures, _serviceDef));
+		_utils.checkFailureForMissingValue(_failures, "policy item users/user-groups");
+	}
+	
+	@Test
+	public void test_isValidItemAccesses_happyPath() {
+		
+		// happy path
+		Object[][] data = new Object[][] {
+				{ "a", null }, // valid
+				{ "b", true }, // valid
+				{ "c", true }, // valid
+		};
+		List<RangerPolicyItemAccess> accesses = _utils.createItemAccess(data);
+		_serviceDef = _utils.createServiceDefWithAccessTypes(new String[] { "a", "b", "c", "d" });
+		assertTrue(_validator.isValidItemAccesses(accesses, _failures, _serviceDef));
+		assertTrue(_failures.isEmpty());
+	}
+	
+	@Test
+	public void test_isValidItemAccesses_failure() {
+
+		// null policy item access values are an error
+		List<RangerPolicyItemAccess> accesses = new ArrayList<RangerPolicyItemAccess>();
+		accesses.add(null);
+		_failures.clear(); assertFalse(_validator.isValidItemAccesses(accesses, _failures, _serviceDef));
+		_utils.checkFailureForMissingValue(_failures, "policy item access");
+
+		// all items must be valid for this call to be valid
+		Object[][] data = new Object[][] {
+				{ "a", null }, // valid
+				{ null, null }, // invalid - name can't be null
+				{ "c", true }, // valid
+		};
+		accesses = _utils.createItemAccess(data);
+		_serviceDef = _utils.createServiceDefWithAccessTypes(new String[] { "a", "b", "c", "d" });
+		_failures.clear(); assertFalse(_validator.isValidItemAccesses(accesses, _failures, _serviceDef));
+	}
+	
+	@Test
+	public void test_isValidPolicyItemAccess_happyPath() {
+		
+		RangerPolicyItemAccess access = mock(RangerPolicyItemAccess.class);
+		when(access.getType()).thenReturn("anAccess"); // valid
+
+		Set<String> validAccesses = Sets.newHashSet(new String[] { "anAccess", "anotherAccess" });
+		
+		// both null or true access types are the same and valid
+		for (Boolean allowed : new Boolean[] { null, true } ) {
+			when(access.getIsAllowed()).thenReturn(allowed);
+			assertTrue(_validator.isValidPolicyItemAccess(access, _failures, validAccesses));
+			assertTrue(_failures.isEmpty());
+		}
+	}
+	
+	@Test
+	public void test_isValidPolicyItemAccess_failures() {
+		
+		Set<String> validAccesses = Sets.newHashSet(new String[] { "anAccess", "anotherAccess" });
+		// null/empty names are invalid
+		RangerPolicyItemAccess access = mock(RangerPolicyItemAccess.class);
+		when(access.getIsAllowed()).thenReturn(null); // valid since null == true
+		for (String type : new String[] { null, " 	"}) {
+			when(access.getType()).thenReturn(type); // invalid
+			// null/empty validAccess set skips all checks
+			assertTrue(_validator.isValidPolicyItemAccess(access, _failures, null));
+			assertTrue(_validator.isValidPolicyItemAccess(access, _failures, new HashSet<String>()));
+			_failures.clear(); assertFalse(_validator.isValidPolicyItemAccess(access, _failures, validAccesses));
+			_utils.checkFailureForMissingValue(_failures, "policy item access type");
+		}
+		
+		when(access.getType()).thenReturn("anAccess"); // valid
+		when(access.getIsAllowed()).thenReturn(false); // invalid
+		_failures.clear();assertFalse(_validator.isValidPolicyItemAccess(access, _failures, validAccesses));
+		_utils.checkFailureForSemanticError(_failures, "policy item access type allowed");
+		
+		when(access.getType()).thenReturn("newAccessType"); // invalid
+		_failures.clear(); assertFalse(_validator.isValidPolicyItemAccess(access, _failures, validAccesses));
+		_utils.checkFailureForSemanticError(_failures, "policy item access type");
+	}
+
+	private ValidationTestUtils _utils = new ValidationTestUtils();
+	private List<ValidationFailureDetails> _failures = new ArrayList<ValidationFailureDetails>();
+	private ServiceStore _store;
+	private RangerPolicy _policy;
+	private RangerPolicyValidator _validator;
+	private RangerServiceDef _serviceDef;
+}

http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/f721abec/security-admin/src/test/java/org/apache/ranger/rest/TestRangerServiceValidator.java
----------------------------------------------------------------------
diff --git a/security-admin/src/test/java/org/apache/ranger/rest/TestRangerServiceValidator.java b/security-admin/src/test/java/org/apache/ranger/rest/TestRangerServiceValidator.java
index 3bbb123..3bad133 100644
--- a/security-admin/src/test/java/org/apache/ranger/rest/TestRangerServiceValidator.java
+++ b/security-admin/src/test/java/org/apache/ranger/rest/TestRangerServiceValidator.java
@@ -25,6 +25,7 @@ import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
@@ -46,122 +47,88 @@ public class TestRangerServiceValidator {
 	public void before() {
 		_store = mock(ServiceStore.class);
 		_action = Action.CREATE; // by default we set action to create
-		_validator = new RangerServiceValidator(_store, _action);
+		_validator = new RangerServiceValidator(_store);
 	}
 
+	void checkFailure_isValid(RangerServiceValidator validator, RangerService service, Action action, List<ValidationFailureDetails> failures, String errorType, String field) {
+		checkFailure_isValid(validator, service, action, failures, errorType, field, null);
+	}
+	
+	void checkFailure_isValid(RangerServiceValidator validator, RangerService service, Action action, List<ValidationFailureDetails> failures, String errorType, String field, String subField) {
+		failures.clear();
+		assertFalse(validator.isValid(service, action, failures));
+		switch (errorType) {
+		case "missing":
+			_utils.checkFailureForMissingValue(failures, field, subField);
+			break;
+		case "semantic":
+			_utils.checkFailureForSemanticError(failures, field, subField);
+			break;
+		case "internal error":
+			_utils.checkFailureForInternalError(failures);
+			break;
+		default:
+			fail("Unsupported errorType[" + errorType + "]");
+			break;
+		}
+	}
+	
 	@Test
 	public void testIsValid_failures() throws Exception {
 		RangerService service = mock(RangerService.class);
-		List<ValidationFailureDetails> failures;
-		// create/update/delete: null, empty of blank name renders a service invalid
-		for (Action action : cud) {
-			_validator = new RangerServiceValidator(_store, action);
-			when(service.getName()).thenReturn(null);
-			assertFalse(_validator.isValid(service));
-			// let's verify the sort of error information that is returned (for one of these)
-			failures = _validator.getFailures();
-			// assert that among the failure reason is one about field name being missing.
-			boolean found = false;
-			for (ValidationFailureDetails f : failures) {
-				if ("name".equals(f.getFieldName()) && 
-						f._internalError == false && 
-						f._missing == true &&
-						f._semanticError == false) {
-					found = true;
-				}
-			}
-			assertTrue("Matching failure located", found);
-			// let's assert behavior for other flavors of this condition, too.
-			when(service.getName()).thenReturn("");
-			assertFalse(_validator.isValid(service));
-			when(service.getName()).thenReturn("  "); // spaces
-			assertFalse(_validator.isValid(service));
-		}
-		
-		// Create/update: null, empty or blank type is also invalid
+		// passing in a null service to the check itself is an error
+		assertFalse(_validator.isValid((RangerService)null, _action, _failures));
+		_utils.checkFailureForMissingValue(_failures, "service");
+
+		// id is required for update
+		when(service.getId()).thenReturn(null);
+		// let's verify the failure and the sort of error information that is returned (for one of these)
+		// assert that among the failure reason is one about id being missing.
+		checkFailure_isValid(_validator, service, Action.UPDATE, _failures, "missing", "id");
+		when(service.getId()).thenReturn(7L);
+
 		for (Action action : cu) {
-			_validator = new RangerServiceValidator(_store, action);
-			when(service.getName()).thenReturn("aName");
-			when(service.getType()).thenReturn(null);
-			assertFalse(_validator.isValid(service));
-			when(service.getType()).thenReturn("");
-			assertFalse(_validator.isValid(service));
-			when(service.getType()).thenReturn("	"); // a tab
-			assertFalse(_validator.isValid(service));
-			// let's verify the error information returned (for the last scenario)
-			failures = _validator.getFailures();
-			boolean found = false;
-			for (ValidationFailureDetails f : failures) {
-				if ("type".equals(f._fieldName) && 
-						f._missing == true && 
-						f._semanticError == false) {
-					found = true;
-				}
+			// null, empty of blank name renders a service invalid
+			for (String name : new String[] { null, "", " 	" }) { // spaces and tabs
+				when(service.getName()).thenReturn(name);
+				checkFailure_isValid(_validator, service, action, _failures, "missing", "name");
+			}
+			// same is true for the type
+			for (String type : new String[] { null, "", "    " }) {
+				when(service.getType()).thenReturn(type);
+				checkFailure_isValid(_validator, service, action, _failures, "missing", "type");
 			}
-			assertTrue("Matching failure located", found);
 		}
+		when(service.getName()).thenReturn("aName");
 
-		// Create/update: if non-empty, the type should also exist!
+		// if non-empty, then the type should exist!
+		when(_store.getServiceDefByName("null-type")).thenReturn(null);
+		when(_store.getServiceDefByName("throwing-type")).thenThrow(new Exception());
 		for (Action action : cu) {
-			_validator = new RangerServiceValidator(_store, action);
-			when(service.getName()).thenReturn("aName");
-			when(service.getType()).thenReturn("aType");
-			when(_store.getServiceDefByName("aType")).thenReturn(null);
-			assertFalse(_validator.isValid(service));
-			// let's verify the error information returned (for the last scenario)
-			failures = _validator.getFailures();
-			boolean found = false;
-			for (ValidationFailureDetails f : failures) {
-				if ("type".equals(f._fieldName) && 
-						f._missing == false && 
-						f._semanticError == true) {
-					found = true;
-				}
+			for (String type : new String[] { "null-type", "throwing-type" }) {
+				when(service.getType()).thenReturn(type);
+				checkFailure_isValid(_validator, service, action, _failures, "semantic", "type");
 			}
-			assertTrue("Matching failure located", found);
 		}
-		
-		// Create: if service already exists then that such a service should be considered invalid by create
-		when(service.getName()).thenReturn("aName");
 		when(service.getType()).thenReturn("aType");
 		RangerServiceDef serviceDef = mock(RangerServiceDef.class);
 		when(_store.getServiceDefByName("aType")).thenReturn(serviceDef);
-		// test both when service exists and when it doesn't -- the result is opposite for the two cases
-		RangerService existingService = mock(RangerService.class);
-		when(_store.getServiceByName("aName")).thenReturn(existingService);
-
-		_validator = new RangerServiceValidator(_store, Action.CREATE);
-		assertFalse(_validator.isValid(service));
-		
-		// check the error returned: it is a semantic error about service's name
-		failures = _validator.getFailures();
-		boolean found = false;
-		for (ValidationFailureDetails f : failures) {
-			if ("name".equals(f._fieldName) && 
-					f._missing == false && 
-					f._semanticError == true) {
-				found = true;
-			}
-		}
-		assertTrue("Matching failure located", found);
-		
-		// Update: Exact inverse is true, i.e. service must exist!
-		when(_store.getServiceByName("anotherName")).thenReturn(null);
-		when(service.getName()).thenReturn("anotherName");
 		
-		_validator = new RangerServiceValidator(_store, Action.UPDATE);
-		assertFalse(_validator.isValid(service));
-		// check the error returned: it is a semantic error about service's name
-		failures = _validator.getFailures();
-		found = false;
-		for (ValidationFailureDetails f : failures) {
-			if ("name".equals(f._fieldName) && 
-					f._missing == false && 
-					f._semanticError == true) {
-				found = true;
-			}
-		}
-		assertTrue("Matching failure located", found);
+		// Create: No service should exist matching its id and/or name
+		RangerService anExistingService = mock(RangerService.class);
+		when(_store.getServiceByName("aName")).thenReturn(anExistingService);
+		checkFailure_isValid(_validator, service, Action.CREATE, _failures, "semantic", "name");
+
+		// Update: service should exist matching its id and name specified should not belong to a different service
+		when(_store.getService(7L)).thenReturn(null);
+		when(_store.getServiceByName("aName")).thenReturn(anExistingService);
+		checkFailure_isValid(_validator, service, Action.UPDATE, _failures, "semantic", "id");
+
+		when(_store.getService(7L)).thenReturn(anExistingService);
+		RangerService anotherExistingService = mock(RangerService.class);
+		when(anotherExistingService.getId()).thenReturn(49L);
+		when(_store.getServiceByName("aName")).thenReturn(anotherExistingService);
+		checkFailure_isValid(_validator, service, Action.UPDATE, _failures, "semantic", "id/name");
 	}
 	
 	@Test
@@ -190,21 +157,7 @@ public class TestRangerServiceValidator {
 		when(_store.getServiceByName("aService")).thenReturn(null);
 		for (Action action : cu) {
 			// it should be invalid
-			_validator = new RangerServiceValidator(_store, action);
-			assertFalse(_validator.isValid(service));
-			// check the error message
-			List<ValidationFailureDetails> failures = _validator.getFailures();
-			boolean found = false;
-			for (ValidationFailureDetails f : failures) {
-				if ("configuration".equals(f.getFieldName()) &&
-						"param2".equals(f._subFieldName) &&
-						f._missing == true &&
-						f._internalError == false && 
-						f._semanticError == false) {
-					found = true;
-				}
-			}
-			assertTrue(found);
+			checkFailure_isValid(_validator, service, action, _failures, "missing", "configuration", "param2");
 		}
 	}
 
@@ -230,33 +183,56 @@ public class TestRangerServiceValidator {
 		Map<String, String> configMap = _utils.createMap(configs);  
 		when(service.getConfigs()).thenReturn(configMap);
 		// wire then into the store
-		try {
-			// service does not exists
-			when(_store.getServiceByName("aName")).thenReturn(null);
-			// service def exists
-			when(_store.getServiceDefByName("aType")).thenReturn(serviceDef);
-		} catch (Exception e) {
-			e.printStackTrace();
-			fail("Unexpected error encountered while mocking!");
-		}
-		_validator = new RangerServiceValidator(_store, Action.CREATE);
-		assertTrue(_validator.isValid(service));
-		// for update to work the only additional requirement is that service should exist
+		// service does not exists
+		when(_store.getServiceByName("aName")).thenReturn(null);
+		// service def exists
+		when(_store.getServiceDefByName("aType")).thenReturn(serviceDef);
+
+		assertTrue(_validator.isValid(service, Action.CREATE, _failures));
+
+		// for update to work the only additional requirement is that id is required and service should exist
+		// if name is not null and it points to a service then it should match the id
+		when(service.getId()).thenReturn(7L);
 		RangerService existingService = mock(RangerService.class);
+		when(existingService.getId()).thenReturn(7L);
+		when(_store.getService(7L)).thenReturn(existingService);
 		when(_store.getServiceByName("aName")).thenReturn(existingService);
-		_validator = new RangerServiceValidator(_store, Action.UPDATE);
-		assertTrue(_validator.isValid(service));
+		assertTrue(_validator.isValid(service, Action.UPDATE, _failures));
+		// name need not point to a service for update to work, of course.
+		when(_store.getServiceByName("aName")).thenReturn(null);
+		assertTrue(_validator.isValid(service, Action.UPDATE, _failures));
 	}
 
-	ValidationFailureDetails getFailure(List<ValidationFailureDetails> failures) {
-		if (failures == null || failures.size() == 0) {
-			return null;
-		} else {
-			return failures.iterator().next();
-		}
+	@Test
+	public void test_isValid_withId_errorConditions() throws Exception {
+		// api that takes in long is only supported for delete currently
+		assertFalse(_validator.isValid(1L, Action.CREATE, _failures));
+		_utils.checkFailureForInternalError(_failures);
+		// passing in a null id is a failure!
+		_validator = new RangerServiceValidator(_store);
+		_failures.clear(); assertFalse(_validator.isValid((Long)null, Action.DELETE, _failures));
+		_utils.checkFailureForMissingValue(_failures, "id");
+		// if service with that id does not exist then that, too, is a failure
+		when(_store.getService(1L)).thenReturn(null);
+		when(_store.getService(2L)).thenThrow(new Exception());
+		_failures.clear(); assertFalse(_validator.isValid(1L, Action.DELETE, _failures));
+		_utils.checkFailureForSemanticError(_failures, "id");
+
+		_failures.clear(); assertFalse(_validator.isValid(2L, Action.DELETE, _failures));
+		_utils.checkFailureForSemanticError(_failures, "id");
 	}
+	
+	@Test
+	public void test_isValid_withId_happyPath() throws Exception {
+		_validator = new RangerServiceValidator(_store);
+		RangerService service = mock(RangerService.class);
+		when(_store.getService(1L)).thenReturn(service);
+		assertTrue(_validator.isValid(1L, Action.DELETE, _failures));
+	}
+	
 	private ServiceStore _store;
 	private RangerServiceValidator _validator;
 	private Action _action;
 	private ValidationTestUtils _utils = new ValidationTestUtils();
+	private List<ValidationFailureDetails> _failures = new ArrayList<ValidationFailureDetails>();
 }