Repository: ranger Updated Branches: refs/heads/ranger-1 bbd94bd8e -> 1e2e15d48
RANGER-2181 : Code Improvement To Follow Best Practices Signed-off-by: Mehul Parikh <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/ranger/repo Commit: http://git-wip-us.apache.org/repos/asf/ranger/commit/1e2e15d4 Tree: http://git-wip-us.apache.org/repos/asf/ranger/tree/1e2e15d4 Diff: http://git-wip-us.apache.org/repos/asf/ranger/diff/1e2e15d4 Branch: refs/heads/ranger-1 Commit: 1e2e15d4813cd8b66cb490ac448311290d6b1854 Parents: bbd94bd Author: Bhavik Patel <[email protected]> Authored: Tue Aug 7 18:11:36 2018 +0530 Committer: Mehul Parikh <[email protected]> Committed: Wed Aug 8 10:59:07 2018 +0530 ---------------------------------------------------------------------- .../plugin/errors/ValidationErrorCode.java | 1 + .../validation/RangerServiceValidator.java | 63 ++++-- .../validation/TestRangerServiceValidator.java | 212 +++++++++++++++++++ .../org/apache/ranger/rest/ServiceREST.java | 10 + .../webapp/scripts/models/RangerServiceDef.js | 2 +- security-admin/src/main/webapp/styles/xa.css | 6 + .../main/webapp/templates/helpers/XAHelpers.js | 2 +- 7 files changed, 278 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java b/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java index 41052dd..3cd7876 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java @@ -34,6 +34,7 @@ public enum ValidationErrorCode { SERVICE_VALIDATION_ERR_INVALID_SERVICE_ID(1005, "No service found for id [{0}]"), SERVICE_VALIDATION_ERR_INVALID_SERVICE_NAME(1006, "Missing service name"), SERVICE_VALIDATION_ERR_SERVICE_NAME_CONFICT(1007, "Duplicate service name: name=[{0}]"), + SERVICE_VALIDATION_ERR_SPECIAL_CHARACTERS_SERVICE_NAME(3031, "Name should not start with space, it should be less than 256 characters and special characters are not allowed(except _ - and space). : name=[{0}]"), SERVICE_VALIDATION_ERR_ID_NAME_CONFLICT(1008, "Duplicate service name: name=[{0}], id=[{1}]"), SERVICE_VALIDATION_ERR_MISSING_SERVICE_DEF(1009, "Missing service def"), SERVICE_VALIDATION_ERR_INVALID_SERVICE_DEF(1010, "Service def not found: service-def-name=[{0}]"), http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java index 1893fb9..b64de32 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java @@ -22,7 +22,7 @@ package org.apache.ranger.plugin.model.validation; import java.util.ArrayList; import java.util.List; import java.util.Set; - +import java.util.regex.Pattern; import org.apache.commons.lang.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -35,13 +35,14 @@ import org.apache.ranger.plugin.store.ServiceStore; import com.google.common.collect.Sets; public class RangerServiceValidator extends RangerValidator { - private static final Log LOG = LogFactory.getLog(RangerServiceValidator.class); - + static final public String VALIDATION_SERVICE_NAME = "^[a-zA-Z0-9_-][a-zA-Z0-9\\s_-]{0,254}"; + + static Pattern serviceNameCompiledRegEx; public RangerServiceValidator(ServiceStore store) { super(store); } - + public void validate(RangerService service, Action action) throws Exception { if(LOG.isDebugEnabled()) { LOG.debug(String.format("==> RangerServiceValidator.validate(%s, %s)", service, action)); @@ -151,9 +152,8 @@ public class RangerServiceValidator extends RangerValidator { .build()); valid = false; } else { - RangerService otherService = getService(name); - if (otherService != null && action == Action.CREATE) { - ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_SERVICE_NAME_CONFICT; + if(!validateString(VALIDATION_SERVICE_NAME, name)){ + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_SPECIAL_CHARACTERS_SERVICE_NAME; failures.add(new ValidationFailureDetailsBuilder() .field("name") .isSemanticallyIncorrect() @@ -161,15 +161,27 @@ public class RangerServiceValidator extends RangerValidator { .becauseOf(error.getMessage(name)) .build()); valid = false; - } else if (otherService != null && otherService.getId() !=null && !otherService.getId().equals(id)) { - ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_ID_NAME_CONFLICT; - failures.add(new ValidationFailureDetailsBuilder() - .field("id/name") - .isSemanticallyIncorrect() - .errorCode(error.getErrorCode()) - .becauseOf(error.getMessage(name, otherService.getId())) - .build()); - valid = false; + }else{ + RangerService otherService = getService(name); + if (otherService != null && action == Action.CREATE) { + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_SERVICE_NAME_CONFICT; + failures.add(new ValidationFailureDetailsBuilder() + .field("name") + .isSemanticallyIncorrect() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(name)) + .build()); + valid = false; + } else if (otherService != null && otherService.getId() !=null && !otherService.getId().equals(id)) { + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_ID_NAME_CONFLICT; + failures.add(new ValidationFailureDetailsBuilder() + .field("id/name") + .isSemanticallyIncorrect() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(name, otherService.getId())) + .build()); + valid = false; + } } } String type = service.getType(); @@ -260,4 +272,23 @@ public class RangerServiceValidator extends RangerValidator { } return valid; } + + public boolean regExPatternMatch(String expression, String inputStr) { + Pattern pattern = serviceNameCompiledRegEx; + if (pattern == null) { + pattern = Pattern.compile(expression, Pattern.CASE_INSENSITIVE); + serviceNameCompiledRegEx = pattern; + } + + return pattern != null ? pattern.matcher(inputStr).matches() : false; + } + + public boolean validateString(String regExStr, String str) { + try { + return regExPatternMatch(regExStr, str); + } catch (Throwable t) { + LOG.error("Error validating string. str=" + str + " due to reason " + t.getMessage() + ". Stack Trace : " + t.getStackTrace()); + return false; + } + } } http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java index 64ccb7d..c271dd9 100644 --- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java +++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -40,6 +41,7 @@ public class TestRangerServiceValidator { final Action[] cud = new Action[] { Action.CREATE, Action.UPDATE, Action.DELETE }; final Action[] cu = new Action[] { Action.CREATE, Action.UPDATE }; final Action[] ud = new Action[] { Action.UPDATE, Action.DELETE }; + String serviceNameValidationErrorMessage = "Name should not start with space, it should be less than 256 characters and special characters are not allowed(except _ - and space). "; @Before public void before() { @@ -72,6 +74,216 @@ public class TestRangerServiceValidator { } @Test + public void testIsValidServiceNameCreationWithOutSpecialCharacters() throws Exception{ + RangerService rangerService = new RangerService(); + rangerService.setName("c1_yarn"); + rangerService.setType("yarn"); + rangerService.setTagService(""); + + RangerServiceConfigDef configDef = new RangerServiceConfigDef(); + configDef.setMandatory(true); + + List<RangerServiceConfigDef> listRangerServiceConfigDef = new ArrayList<RangerServiceDef.RangerServiceConfigDef>(); + listRangerServiceConfigDef.add(configDef); + + + configDef.setName("myconfig1"); + + Map<String,String> testMap = new HashMap<String, String>(); + testMap.put("myconfig1", "myconfig1"); + + rangerService.setConfigs(testMap); + + + RangerServiceDef rangerServiceDef = new RangerServiceDef(); + rangerServiceDef.setConfigs(listRangerServiceConfigDef); + + when(_store.getServiceDefByName("yarn")).thenReturn(rangerServiceDef); + boolean valid = _validator.isValid(rangerService, Action.CREATE, _failures); + Assert.assertEquals(0, _failures.size()); + Assert.assertTrue(valid); + + } + + @Test + public void testIsValidServiceNameUpdationWithOutSpecialCharacters() throws Exception{ + RangerService rangerService = new RangerService(); + rangerService.setId(1L); + rangerService.setName("c1_yarn"); + rangerService.setType("yarn"); + rangerService.setTagService(""); + + RangerServiceConfigDef configDef = new RangerServiceConfigDef(); + configDef.setMandatory(true); + + List<RangerServiceConfigDef> listRangerServiceConfigDef = new ArrayList<RangerServiceDef.RangerServiceConfigDef>(); + listRangerServiceConfigDef.add(configDef); + + + configDef.setName("myconfig1"); + + Map<String,String> testMap = new HashMap<String, String>(); + testMap.put("myconfig1", "myconfig1"); + + rangerService.setConfigs(testMap); + + + RangerServiceDef rangerServiceDef = new RangerServiceDef(); + rangerServiceDef.setConfigs(listRangerServiceConfigDef); + + when(_store.getService(1L)).thenReturn(rangerService); + when(_store.getServiceDefByName("yarn")).thenReturn(rangerServiceDef); + boolean valid = _validator.isValid(rangerService, Action.UPDATE, _failures); + Assert.assertEquals(0, _failures.size()); + Assert.assertTrue(valid); + + } + + @Test + public void testIsValidServiceNameUpdationWithSpecialCharacters() throws Exception{ + RangerService rangerService = new RangerService(); + rangerService.setId(1L); + rangerService.setName("<alert>c1_yarn</alert>"); + rangerService.setType("yarn"); + rangerService.setTagService(""); + + RangerServiceConfigDef configDef = new RangerServiceConfigDef(); + configDef.setMandatory(true); + + List<RangerServiceConfigDef> listRangerServiceConfigDef = new ArrayList<RangerServiceDef.RangerServiceConfigDef>(); + listRangerServiceConfigDef.add(configDef); + + + configDef.setName("myconfig1"); + + Map<String,String> testMap = new HashMap<String, String>(); + testMap.put("myconfig1", "myconfig1"); + + rangerService.setConfigs(testMap); + + + RangerServiceDef rangerServiceDef = new RangerServiceDef(); + rangerServiceDef.setConfigs(listRangerServiceConfigDef); + + when(_store.getService(1L)).thenReturn(rangerService); + when(_store.getServiceDefByName("yarn")).thenReturn(rangerServiceDef); + boolean valid = _validator.isValid(rangerService, Action.UPDATE, _failures); + ValidationFailureDetails failureMessage = _failures.get(0); + Assert.assertFalse(valid); + Assert.assertEquals("name",failureMessage.getFieldName()); + Assert.assertEquals(serviceNameValidationErrorMessage + ": name=[<alert>c1_yarn</alert>]",failureMessage._reason); + Assert.assertEquals(3031, failureMessage._errorCode); + + } + + @Test + public void testIsValidServiceNameCreationWithSpecialCharacters() throws Exception{ + RangerService rangerService = new RangerService(); + rangerService.setName("<script>c1_yarn</script>"); + rangerService.setType("yarn"); + rangerService.setTagService(""); + + RangerServiceConfigDef configDef = new RangerServiceConfigDef(); + configDef.setMandatory(true); + + List<RangerServiceConfigDef> listRangerServiceConfigDef = new ArrayList<RangerServiceDef.RangerServiceConfigDef>(); + listRangerServiceConfigDef.add(configDef); + + + configDef.setName("myconfig1"); + + Map<String,String> testMap = new HashMap<String, String>(); + testMap.put("myconfig1", "myconfig1"); + + rangerService.setConfigs(testMap); + + + RangerServiceDef rangerServiceDef = new RangerServiceDef(); + rangerServiceDef.setConfigs(listRangerServiceConfigDef); + + when(_store.getServiceDefByName("yarn")).thenReturn(rangerServiceDef); + boolean valid = _validator.isValid(rangerService, _action, _failures); + ValidationFailureDetails failureMessage = _failures.get(0); + Assert.assertFalse(valid); + Assert.assertEquals("name",failureMessage.getFieldName()); + Assert.assertEquals(serviceNameValidationErrorMessage + ": name=[<script>c1_yarn</script>]",failureMessage._reason); + Assert.assertEquals(3031, failureMessage._errorCode); + + } + + @Test + public void testIsValidServiceNameCreationWithGreater255Characters() throws Exception{ + RangerService rangerService = new RangerService(); + rangerService.setName("c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1"); + rangerService.setType("yarn"); + rangerService.setTagService(""); + + RangerServiceConfigDef configDef = new RangerServiceConfigDef(); + configDef.setMandatory(true); + + List<RangerServiceConfigDef> listRangerServiceConfigDef = new ArrayList<RangerServiceDef.RangerServiceConfigDef>(); + listRangerServiceConfigDef.add(configDef); + + + configDef.setName("myconfig1"); + + Map<String,String> testMap = new HashMap<String, String>(); + testMap.put("myconfig1", "myconfig1"); + + rangerService.setConfigs(testMap); + + + RangerServiceDef rangerServiceDef = new RangerServiceDef(); + rangerServiceDef.setConfigs(listRangerServiceConfigDef); + + when(_store.getServiceDefByName("yarn")).thenReturn(rangerServiceDef); + boolean valid = _validator.isValid(rangerService, _action, _failures); + ValidationFailureDetails failureMessage = _failures.get(0); + Assert.assertFalse(valid); + Assert.assertEquals("name",failureMessage.getFieldName()); + Assert.assertEquals(serviceNameValidationErrorMessage + ": name=[c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1]",failureMessage._reason); + Assert.assertEquals(3031, failureMessage._errorCode); + + } + + @Test + public void testIsValidServiceNameUpdationWithGreater255Characters() throws Exception{ + RangerService rangerService = new RangerService(); + rangerService.setId(1L); + rangerService.setName("c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1"); + rangerService.setType("yarn"); + rangerService.setTagService(""); + + RangerServiceConfigDef configDef = new RangerServiceConfigDef(); + configDef.setMandatory(true); + + List<RangerServiceConfigDef> listRangerServiceConfigDef = new ArrayList<RangerServiceDef.RangerServiceConfigDef>(); + listRangerServiceConfigDef.add(configDef); + + + configDef.setName("myconfig1"); + + Map<String,String> testMap = new HashMap<String, String>(); + testMap.put("myconfig1", "myconfig1"); + + rangerService.setConfigs(testMap); + + + RangerServiceDef rangerServiceDef = new RangerServiceDef(); + rangerServiceDef.setConfigs(listRangerServiceConfigDef); + + when(_store.getService(1L)).thenReturn(rangerService); + when(_store.getServiceDefByName("yarn")).thenReturn(rangerServiceDef); + boolean valid = _validator.isValid(rangerService, Action.UPDATE, _failures); + ValidationFailureDetails failureMessage = _failures.get(0); + Assert.assertFalse(valid); + Assert.assertEquals("name",failureMessage.getFieldName()); + Assert.assertEquals(serviceNameValidationErrorMessage +": name=[c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1_yarn_c1]",failureMessage._reason); + Assert.assertEquals(3031, failureMessage._errorCode); + + } + + @Test public void testIsValid_failures() throws Exception { RangerService service = mock(RangerService.class); // passing in a null service to the check itself is an error http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/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 bd8d43e..5f8a05a 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 @@ -635,11 +635,16 @@ public class ServiceREST { RangerPerfTracer perf = null; try { + if(RangerPerfTracer.isPerfTraceEnabled(PERF_LOG)) { perf = RangerPerfTracer.getPerfTracer(PERF_LOG, "ServiceREST.createService(serviceName=" + service.getName() + ")"); } RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore); validator.validate(service, Action.CREATE); + + if(!StringUtils.isEmpty(service.getName().trim())){ + service.setName(service.getName().trim()); + } UserSessionBase session = ContextUtil.getCurrentUserSession(); XXServiceDef xxServiceDef = daoManager.getXXServiceDef().findByName(service.getType()); @@ -693,11 +698,16 @@ public class ServiceREST { RangerPerfTracer perf = null; try { + if(RangerPerfTracer.isPerfTraceEnabled(PERF_LOG)) { perf = RangerPerfTracer.getPerfTracer(PERF_LOG, "ServiceREST.updateService(serviceName=" + service.getName() + ")"); } RangerServiceValidator validator = validatorFactory.getServiceValidator(svcStore); validator.validate(service, Action.UPDATE); + + if(!StringUtils.isEmpty(service.getName().trim())){ + service.setName(service.getName().trim()); + } bizUtil.hasAdminPermissions("Services"); http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/security-admin/src/main/webapp/scripts/models/RangerServiceDef.js ---------------------------------------------------------------------- diff --git a/security-admin/src/main/webapp/scripts/models/RangerServiceDef.js b/security-admin/src/main/webapp/scripts/models/RangerServiceDef.js index 9945044..d008f40 100644 --- a/security-admin/src/main/webapp/scripts/models/RangerServiceDef.js +++ b/security-admin/src/main/webapp/scripts/models/RangerServiceDef.js @@ -57,7 +57,7 @@ define(function(require){ name : { type : 'Text', title : 'Service Name *', - validators : ['required',{type:'regexp',regexp:/^[a-zA-Z0-9\s_-]{1,512}$/,message :"Name should be less than 512 characters and special characters are not allowed."}], + validators : ['required',{type:'regexp',regexp:/^[a-zA-Z0-9_-][a-zA-Z0-9\s_-]{0,254}/,message :"Name should not start with space, it should be less than 256 characters and special characters are not allowed(except _ - and space)."}], }, description : { type : 'TextArea', http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/security-admin/src/main/webapp/styles/xa.css ---------------------------------------------------------------------- diff --git a/security-admin/src/main/webapp/styles/xa.css b/security-admin/src/main/webapp/styles/xa.css index db96ed2..c601d54 100644 --- a/security-admin/src/main/webapp/styles/xa.css +++ b/security-admin/src/main/webapp/styles/xa.css @@ -2390,4 +2390,10 @@ textarea:read-only{ .borderNone { border: none; padding: 0px; +} +.serviceNameEllipsis { + max-width: 250px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; } \ No newline at end of file http://git-wip-us.apache.org/repos/asf/ranger/blob/1e2e15d4/security-admin/src/main/webapp/templates/helpers/XAHelpers.js ---------------------------------------------------------------------- diff --git a/security-admin/src/main/webapp/templates/helpers/XAHelpers.js b/security-admin/src/main/webapp/templates/helpers/XAHelpers.js index 8ace4d7..27de701 100644 --- a/security-admin/src/main/webapp/templates/helpers/XAHelpers.js +++ b/security-admin/src/main/webapp/templates/helpers/XAHelpers.js @@ -522,7 +522,7 @@ </div>' } tr += '<tr><td><div>\ - <a data-id="'+serv.id+'" href="#!/service/'+serv.id+'/policies/'+policyType+'">'+_.escape(serv.attributes.name)+'</a>'+serviceOperationDiv+'\ + <a class="pull-left serviceNameEllipsis" data-id="'+serv.id+'" href="#!/service/'+serv.id+'/policies/'+policyType+'" title="'+_.escape(serv.attributes.name)+'">'+_.escape(serv.attributes.name)+'</a>'+serviceOperationDiv+'\ </div></td></tr>'; }); }
