Skip to content

Commit

Permalink
RANGER-2181 : Code Improvement To Follow Best Practices for saving se…
Browse files Browse the repository at this point in the history 
…rvices

Signed-off-by: Mehul Parikh <mehul@apache.org>
(cherry picked from commit 2dfd1ea)
  • Loading branch information
@bhavikpatel9977 @pradeepagrawal8184
bhavikpatel9977 authored and pradeepagrawal8184 committed Sep 28, 2018
1 parent 46c6cf8 commit b050618
Show file tree
Hide file tree
Showing 7 changed files with 276 additions and 16 deletions.
1 change: 1 addition & 0 deletions agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}]"),
Expand Down
59 changes: 45 additions & 14 deletions ...ommon/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
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;
Expand All @@ -35,9 +35,10 @@
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);
}
Expand Down Expand Up @@ -151,25 +152,36 @@ boolean isValid(RangerService service, Action action, List<ValidationFailureDeta
.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()
.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;
}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();
Expand Down Expand Up @@ -260,4 +272,23 @@ boolean isValid(RangerService service, Action action, List<ValidationFailureDeta
}
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;
}
}
}
212 changes: 212 additions & 0 deletions ...n/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerServiceValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

Expand All @@ -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() {
Expand Down Expand Up @@ -72,6 +74,216 @@ void checkFailure_isValid(RangerServiceValidator validator, RangerService servic
}

@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
Expand Down
10 changes: 10 additions & 0 deletions security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,12 +626,17 @@ public RangerService createService(RangerService service) {
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());
if(session != null && !session.isSpnegoEnabled()){
Expand Down Expand Up @@ -683,12 +688,17 @@ public RangerService updateService(RangerService service,
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");

// TODO: As of now we are allowing SYS_ADMIN to create all the
Expand Down
2 changes: 1 addition & 1 deletion security-admin/src/main/webapp/scripts/models/RangerServiceDef.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,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',
Expand Down
6 changes: 6 additions & 0 deletions security-admin/src/main/webapp/styles/xa.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2221,3 +2221,9 @@ td.subgrid-custom-cell{
text-overflow: ellipsis;
max-width: 95%;
}
.serviceNameEllipsis {
max-width: 250px;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
2 changes: 1 addition & 1 deletion security-admin/src/main/webapp/templates/helpers/XAHelpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,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>';
});
}
Expand Down

0 comments on commit b050618

Please sign in to comment.