You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ranger.apache.org by me...@apache.org on 2018/09/27 06:59:03 UTC

[2/5] ranger git commit: RANGER-2218: Added validations for names duing service def updates

RANGER-2218: Added validations for names duing service def updates


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

Branch: refs/heads/ranger-1
Commit: b7c84df7f792acd7ed8ec0654b6b969b3a407a67
Parents: 1cc4b1e
Author: Sailaja Polavarapu <sp...@hortonworks.com>
Authored: Wed Sep 26 15:06:26 2018 -0700
Committer: Mehul Parikh <me...@apache.org>
Committed: Thu Sep 27 12:06:02 2018 +0530

----------------------------------------------------------------------
 .../validation/RangerServiceDefValidator.java   | 126 +++++++++++++++++--
 .../TestRangerServiceDefValidator.java          |  38 +++---
 .../java/org/apache/ranger/biz/XUserMgr.java    |  11 +-
 3 files changed, 149 insertions(+), 26 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/ranger/blob/b7c84df7/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefValidator.java
----------------------------------------------------------------------
diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefValidator.java
index d73210e..45821e8 100644
--- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefValidator.java
+++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefValidator.java
@@ -26,6 +26,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.HashMap;
 
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.StringUtils;
@@ -40,6 +41,7 @@ import org.apache.ranger.plugin.model.RangerServiceDef.RangerEnumElementDef;
 import org.apache.ranger.plugin.model.RangerServiceDef.RangerPolicyConditionDef;
 import org.apache.ranger.plugin.model.RangerServiceDef.RangerResourceDef;
 import org.apache.ranger.plugin.model.RangerServiceDef.RangerServiceConfigDef;
+import org.apache.ranger.plugin.model.RangerServiceDef.RangerDataMaskTypeDef;
 import org.apache.ranger.plugin.store.ServiceStore;
 
 import com.google.common.collect.ImmutableSet;
@@ -137,8 +139,8 @@ public class RangerServiceDefValidator extends RangerValidator {
 			Long id = serviceDef.getId();
 			valid = isValidServiceDefId(id, action, failures) && valid;
 			valid = isValidServiceDefName(serviceDef.getName(), id, action, failures) && valid;
-			valid = isValidAccessTypes(serviceDef.getAccessTypes(), failures) && valid;
-			if (isValidResources(serviceDef, failures)) {
+			valid = isValidAccessTypes(serviceDef.getId(), serviceDef.getAccessTypes(), failures, action) && valid;
+			if (isValidResources(serviceDef, failures, action)) {
 				// Semantic check of resource graph can only be done if resources are "syntactically" valid
 				valid = isValidResourceGraph(serviceDef, failures) && valid;
 			} else {
@@ -151,7 +153,8 @@ public class RangerServiceDefValidator extends RangerValidator {
 			} else {
 				valid = false;
 			}
-			valid = isValidPolicyConditions(serviceDef.getPolicyConditions(), failures) && valid;
+			valid = isValidPolicyConditions(serviceDef.getId(), serviceDef.getPolicyConditions(), failures, action) && valid;
+			valid = isValidDataMaskTypes(serviceDef.getId(), serviceDef.getDataMaskDef().getMaskTypes(), failures, action) && valid;
 		}
 		
 		if(LOG.isDebugEnabled()) {
@@ -238,7 +241,8 @@ public class RangerServiceDefValidator extends RangerValidator {
 		return valid;
 	}
 	
-	boolean isValidAccessTypes(final List<RangerAccessTypeDef> accessTypeDefs, final List<ValidationFailureDetails> failures) {
+	boolean isValidAccessTypes(final Long serviceDefId, final List<RangerAccessTypeDef> accessTypeDefs,
+							   final List<ValidationFailureDetails> failures, final Action action) {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug(String.format("==> RangerServiceDefValidator.isValidAccessTypes(%s, %s)", accessTypeDefs, failures));
 		}
@@ -254,13 +258,32 @@ public class RangerServiceDefValidator extends RangerValidator {
 				.build());
 			valid = false;
 		} else {
+			Map<Long, String> existingAccessTypeIDNameMap = new HashMap<>();
+			if (action == Action.UPDATE) {
+				List<RangerAccessTypeDef> existingAccessTypes = this.getServiceDef(serviceDefId).getAccessTypes();
+				for (RangerAccessTypeDef existingAccessType : existingAccessTypes) {
+					existingAccessTypeIDNameMap.put(existingAccessType.getItemId(), existingAccessType.getName());
+				}
+			}
+			if(LOG.isDebugEnabled()) {
+				LOG.debug("accessType names from db = " + existingAccessTypeIDNameMap.values());
+			}
 			List<RangerAccessTypeDef> defsWithImpliedGrants = new ArrayList<>();
 			Set<String> accessNames = new HashSet<>();
 			Set<Long> ids = new HashSet<>();
 			for (RangerAccessTypeDef def : accessTypeDefs) {
 				String name = def.getName();
+				Long itemId = def.getItemId();
 				valid = isUnique(name, accessNames, "access type name", "access types", failures) && valid;
 				valid = isUnique(def.getItemId(), ids, "access type itemId", "access types", failures) && valid;
+				if (action == Action.UPDATE) {
+					if (existingAccessTypeIDNameMap.get(itemId) != null && !existingAccessTypeIDNameMap.get(itemId).equals(name)) {
+						ValidationErrorCode error;
+						error = ValidationErrorCode.SERVICE_DEF_VALIDATION_ERR_SERVICE_DEF_NAME_CONFICT;
+						failures.add((new ValidationFailureDetailsBuilder()).field("access type name").isSemanticallyIncorrect().errorCode(error.getErrorCode()).becauseOf(String.format("changing %s[%s] in %s is not supported", "access type name", name, "access types")).build());
+						valid = false;
+					}
+				}
 				if (CollectionUtils.isNotEmpty(def.getImpliedGrants())) {
 					defsWithImpliedGrants.add(def);
 				}
@@ -302,7 +325,8 @@ public class RangerServiceDefValidator extends RangerValidator {
 		return valid;
 	}
 
-	boolean isValidPolicyConditions(List<RangerPolicyConditionDef> policyConditions, List<ValidationFailureDetails> failures) {
+	boolean isValidPolicyConditions(Long serviceDefId, List<RangerPolicyConditionDef> policyConditions,
+									List<ValidationFailureDetails> failures, final Action action) {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug(String.format("==> RangerServiceDefValidator.isValidPolicyConditions(%s, %s)", policyConditions, failures));
 		}
@@ -311,12 +335,31 @@ public class RangerServiceDefValidator extends RangerValidator {
 		if (CollectionUtils.isEmpty(policyConditions)) {
 			LOG.debug("Configs collection was null/empty! ok");
 		} else {
+			Map<Long, String> existingPolicyCondIDNameMap = new HashMap<>();
+			if (action == Action.UPDATE) {
+				List<RangerPolicyConditionDef> existingPolicyConditions = this.getServiceDef(serviceDefId).getPolicyConditions();
+				for (RangerPolicyConditionDef existingPolicyCondition : existingPolicyConditions) {
+					existingPolicyCondIDNameMap.put(existingPolicyCondition.getItemId(), existingPolicyCondition.getName());
+				}
+			}
+			if(LOG.isDebugEnabled()) {
+				LOG.debug("policy condition names from db = " + existingPolicyCondIDNameMap.values());
+			}
 			Set<Long> ids = new HashSet<>();
 			Set<String> names = new HashSet<>();
 			for (RangerPolicyConditionDef conditionDef : policyConditions) {
-				valid = isUnique(conditionDef.getItemId(), ids, "policy condition def itemId", "policy condition defs", failures) && valid;
+				Long itemId = conditionDef.getItemId();
+				valid = isUnique(itemId, ids, "policy condition def itemId", "policy condition defs", failures) && valid;
 				String name = conditionDef.getName();
 				valid = isUnique(name, names, "policy condition def name", "policy condition defs", failures) && valid;
+				if (action == Action.UPDATE) {
+					if (existingPolicyCondIDNameMap.get(itemId) != null && !existingPolicyCondIDNameMap.get(itemId).equals(name)) {
+						ValidationErrorCode error;
+						error = ValidationErrorCode.SERVICE_DEF_VALIDATION_ERR_SERVICE_DEF_NAME_CONFICT;
+						failures.add((new ValidationFailureDetailsBuilder()).field("policy condition def name").isSemanticallyIncorrect().errorCode(error.getErrorCode()).becauseOf(String.format("changing %s[%s] in %s is not supported", "policy condition def name", name, "policy condition defs")).build());
+						valid = false;
+					}
+				}
 				if (StringUtils.isBlank(conditionDef.getEvaluator())) {
 					ValidationErrorCode error = ValidationErrorCode.SERVICE_DEF_VALIDATION_ERR_POLICY_CONDITION_NULL_EVALUATOR;
 					failures.add(new ValidationFailureDetailsBuilder()
@@ -452,7 +495,7 @@ public class RangerServiceDefValidator extends RangerValidator {
 		return valid;
 	}
 
-	boolean isValidResources(RangerServiceDef serviceDef, List<ValidationFailureDetails> failures) {
+	boolean isValidResources(RangerServiceDef serviceDef, List<ValidationFailureDetails> failures, final Action action) {
 		if(LOG.isDebugEnabled()) {
 			LOG.debug(String.format("==> RangerServiceDefValidator.isValidResources(%s, %s)", serviceDef, failures));
 		}
@@ -469,6 +512,17 @@ public class RangerServiceDefValidator extends RangerValidator {
 					.build());
 			valid = false;
 		} else {
+			Map<Long, String> existingResourceIDNameMap = new HashMap<>();
+			if (action == Action.UPDATE) {
+				List<RangerResourceDef> existingResources = this.getServiceDef(serviceDef.getId()).getResources();
+				for (RangerResourceDef existingResource : existingResources) {
+					existingResourceIDNameMap.put(existingResource.getItemId(), existingResource.getName());
+				}
+			}
+			if(LOG.isDebugEnabled()) {
+				LOG.debug("resource names from db = " + existingResourceIDNameMap.values());
+			}
+
 			Set<String> names = new HashSet<String>(resources.size());
 			Set<Long> ids = new HashSet<Long>(resources.size());
 			for (RangerResourceDef resource : resources) {
@@ -477,8 +531,18 @@ public class RangerServiceDefValidator extends RangerValidator {
 				/*
 				 * While id is the natural key, name is a surrogate key.  At several places code expects resource name to be unique within a service.
 				 */
-				valid = isUnique(resource.getName(), names, "resource name", "resources", failures) && valid;
-				valid = isUnique(resource.getItemId(), ids, "resource itemId", "resources", failures) && valid;
+				String name = resource.getName();
+				Long itemId = resource.getItemId();
+				valid = isUnique(name, names, "resource name", "resources", failures) && valid;
+				valid = isUnique(itemId, ids, "resource itemId", "resources", failures) && valid;
+				if (action == Action.UPDATE) {
+					if (existingResourceIDNameMap.get(itemId) != null && !existingResourceIDNameMap.get(itemId).equals(name)) {
+						ValidationErrorCode error;
+						error = ValidationErrorCode.SERVICE_DEF_VALIDATION_ERR_SERVICE_DEF_NAME_CONFICT;
+						failures.add((new ValidationFailureDetailsBuilder()).field("resource name").isSemanticallyIncorrect().errorCode(error.getErrorCode()).becauseOf(String.format("changing %s[%s] in %s is not supported", "resource name", name, "resources")).build());
+						valid = false;
+					}
+				}
 			}
 		}
 
@@ -640,4 +704,48 @@ public class RangerServiceDefValidator extends RangerValidator {
 		}
 		return valid;
 	}
+
+	boolean isValidDataMaskTypes(Long serviceDefId, List<RangerDataMaskTypeDef> dataMaskTypes, List<ValidationFailureDetails> failures, final Action action) {
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("==> RangerServiceDefValidator.isValidDataMaskTypes(%s, %s)", dataMaskTypes, failures));
+		}
+		boolean valid = true;
+
+		if (CollectionUtils.isEmpty(dataMaskTypes)) {
+			LOG.debug("Configs collection was null/empty! ok");
+		} else {
+			Map<Long, String> existingDataMaskTypeIDNameMap = new HashMap<>();
+			if (action == Action.UPDATE) {
+				List<RangerDataMaskTypeDef> existingDataMaskTypes = this.getServiceDef(serviceDefId).getDataMaskDef().getMaskTypes();
+				for (RangerDataMaskTypeDef existingDataMaskType : existingDataMaskTypes) {
+					existingDataMaskTypeIDNameMap.put(existingDataMaskType.getItemId(), existingDataMaskType.getName());
+				}
+			}
+			if(LOG.isDebugEnabled()) {
+				LOG.debug("data mask type names from db = " + existingDataMaskTypeIDNameMap.values());
+			}
+
+			Set<Long> ids = new HashSet<Long>();
+			Set<String> names = new HashSet<String>();
+			for (RangerDataMaskTypeDef dataMaskType : dataMaskTypes) {
+				String name = dataMaskType.getName();
+				Long itemId = dataMaskType.getItemId();
+				valid = isUnique(itemId, ids, "data mask type def itemId", "data mask type defs", failures) && valid;
+				valid = isUnique(name, names, "data mask type def name", "data mask type defs", failures) && valid;
+				if (action == Action.UPDATE) {
+					if (existingDataMaskTypeIDNameMap.get(itemId) != null && !existingDataMaskTypeIDNameMap.get(itemId).equals(name)) {
+						ValidationErrorCode error;
+						error = ValidationErrorCode.SERVICE_DEF_VALIDATION_ERR_SERVICE_DEF_NAME_CONFICT;
+						failures.add((new ValidationFailureDetailsBuilder()).field("data mask type def name").isSemanticallyIncorrect().errorCode(error.getErrorCode()).becauseOf(String.format("changing %s[%s] in %s is not supported", "data mask type def name", name, "data mask type defs")).build());
+						valid = false;
+					}
+				}
+			}
+		}
+
+		if(LOG.isDebugEnabled()) {
+			LOG.debug(String.format("<== RangerServiceDefValidator.isValidDataMaskTypes(%s, %s): %s", dataMaskTypes, failures, valid));
+		}
+		return valid;
+	}
 }

http://git-wip-us.apache.org/repos/asf/ranger/blob/b7c84df7/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefValidator.java
----------------------------------------------------------------------
diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefValidator.java
index decf07c..f4e29c7 100644
--- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefValidator.java
+++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceDefValidator.java
@@ -211,46 +211,50 @@ public class TestRangerServiceDefValidator {
 
 	@Test
 	public final void test_isValidAccessTypes_happyPath() {
+		long id = 7;
+		when(_serviceDef.getId()).thenReturn(id);
 		List<RangerAccessTypeDef> input = _utils.createAccessTypeDefs(accessTypes_good);
-		assertTrue(_validator.isValidAccessTypes(input, _failures));
+		assertTrue(_validator.isValidAccessTypes(id, input, _failures, Action.CREATE));
 		assertTrue(_failures.isEmpty());
 	}
 	
 	@Test
 	public final void test_isValidAccessTypes_failures() {
+		long id = 7;
+		when(_serviceDef.getId()).thenReturn(id);
 		// null or empty access type defs
 		List<RangerAccessTypeDef> accessTypeDefs = null;
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "access types");
 		
 		accessTypeDefs = new ArrayList<>();
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "access types");
 
 		// null/empty access types
 		accessTypeDefs = _utils.createAccessTypeDefs(new String[] { null, "", "		" });
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "access type name");
 		
 		// duplicate access types
 		accessTypeDefs = _utils.createAccessTypeDefs(new String[] { "read", "write", "execute", "read" } );
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForSemanticError(_failures, "access type name", "read");
 		
 		// duplicate access types - case-insensitive
 		accessTypeDefs = _utils.createAccessTypeDefs(new String[] { "read", "write", "execute", "READ" } );
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForSemanticError(_failures, "access type name", "READ");
 		
 		// unknown access type in implied grants list
 		accessTypeDefs = _utils.createAccessTypeDefs(accessTypes_bad_unknownType);
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForSemanticError(_failures, "implied grants", "execute");
 		_utils.checkFailureForSemanticError(_failures, "access type itemId", "1"); // id 1 is duplicated
 		
 		// access type with implied grant referring to itself
 		accessTypeDefs = _utils.createAccessTypeDefs(accessTypes_bad_selfReference);
-		_failures.clear(); assertFalse(_validator.isValidAccessTypes(accessTypeDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidAccessTypes(id, accessTypeDefs, _failures, Action.CREATE));
 		_utils.checkFailureForSemanticError(_failures, "implied grants", "admin");
 	}
 	
@@ -403,23 +407,23 @@ public class TestRangerServiceDefValidator {
 	public final void test_isValidResources() {
 		// null/empty resources are an error
 		when(_serviceDef.getResources()).thenReturn(null);
-		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures));
+		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "resources");
 		
 		List<RangerResourceDef> resources = new ArrayList<>();
 		when(_serviceDef.getResources()).thenReturn(resources);
-		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures));
+		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "resources");
 		
 		resources.addAll(_utils.createResourceDefsWithIds(invalidResources));
-		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures));
+		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "resource name");
 		_utils.checkFailureForMissingValue(_failures, "resource itemId");
 		_utils.checkFailureForSemanticError(_failures, "resource itemId", "1"); // id 1 is duplicate
 		_utils.checkFailureForSemanticError(_failures, "resource name", "DataBase");
 
 		resources.clear(); resources.addAll(_utils.createResourceDefsWithIds(mixedCaseResources));
-		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures));
+		_failures.clear(); assertFalse(_validator.isValidResources(_serviceDef, _failures, Action.CREATE));
 		_utils.checkFailure(_failures, null, null, null, "DBase",null);
 		_utils.checkFailure(_failures, null, null, null, "TABLE",null);
 		_utils.checkFailure(_failures, null, null, null, "Column",null);
@@ -499,7 +503,7 @@ public class TestRangerServiceDefValidator {
 		};
 		List<RangerResourceDef> resources = _utils.createResourceDefsWithIds(data);
 		when(_serviceDef.getResources()).thenReturn(resources);
-		assertTrue(_validator.isValidResources(_serviceDef, _failures));
+		assertTrue(_validator.isValidResources(_serviceDef, _failures, Action.CREATE));
 		assertTrue(_failures.isEmpty());
 	}
 
@@ -533,11 +537,13 @@ public class TestRangerServiceDefValidator {
 
 	@Test
 	public final void test_isValidPolicyConditions() {
+		long id = 7;
+		when(_serviceDef.getId()).thenReturn(id);
 		// null/empty policy conditions are ok
-		assertTrue(_validator.isValidPolicyConditions(null, _failures));
+		assertTrue(_validator.isValidPolicyConditions(id,null, _failures, Action.CREATE));
 		assertTrue(_failures.isEmpty());
 		List<RangerPolicyConditionDef> conditionDefs = new ArrayList<>();
-		assertTrue(_validator.isValidPolicyConditions(conditionDefs, _failures));
+		assertTrue(_validator.isValidPolicyConditions(id, conditionDefs, _failures, Action.CREATE));
 		assertTrue(_failures.isEmpty());
 		
 		Object[][] policyCondition_data = {
@@ -549,7 +555,7 @@ public class TestRangerServiceDefValidator {
 		};
 		
 		conditionDefs.addAll(_utils.createPolicyConditionDefs(policyCondition_data));
-		_failures.clear(); assertFalse(_validator.isValidPolicyConditions(conditionDefs, _failures));
+		_failures.clear(); assertFalse(_validator.isValidPolicyConditions(id, conditionDefs, _failures, Action.CREATE));
 		_utils.checkFailureForMissingValue(_failures, "policy condition def itemId");
 		_utils.checkFailureForMissingValue(_failures, "policy condition def name");
 		_utils.checkFailureForMissingValue(_failures, "policy condition def evaluator");

http://git-wip-us.apache.org/repos/asf/ranger/blob/b7c84df7/security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java
----------------------------------------------------------------------
diff --git a/security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java b/security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java
index b1ea280..23b3fe4 100644
--- a/security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java
+++ b/security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java
@@ -347,11 +347,15 @@ public class XUserMgr extends XUserMgrBase {
                 xaBizUtil.blockAuditorRoleUser();
 		VXPortalUser oldUserProfile = userMgr.getUserProfileByLoginId(vXUser
 				.getName());
+		if (oldUserProfile == null) {
+			throw restErrorUtil.createRESTException(
+					"user " + vXUser.getName() + " does not exist.",
+					MessageEnums.INVALID_INPUT_DATA);
+		}
 		VXPortalUser vXPortalUser = new VXPortalUser();
 		if (oldUserProfile != null && oldUserProfile.getId() != null) {
 			vXPortalUser.setId(oldUserProfile.getId());
 		}
-		// TODO : There is a possibility that old user may not exist.
 
 		vXPortalUser.setFirstName(vXUser.getFirstName());
 		if("null".equalsIgnoreCase(vXPortalUser.getFirstName())){
@@ -855,6 +859,11 @@ public class XUserMgr extends XUserMgrBase {
 		checkAdminAccess();
                 xaBizUtil.blockAuditorRoleUser();
 		XXGroup xGroup = daoManager.getXXGroup().getById(vXGroup.getId());
+		if (vXGroup != null && xGroup != null && !vXGroup.getName().equals(xGroup.getName())) {
+			throw restErrorUtil.createRESTException(
+					"group name updates are not allowed.",
+					MessageEnums.INVALID_INPUT_DATA);
+		}
 		List<XXTrxLog> trxLogList = xGroupService.getTransactionLog(vXGroup,
 				xGroup, "update");
 		xaBizUtil.createTrxLog(trxLogList);