From 723fb50d67feb3ee2277dfa43e90603028779d7a Mon Sep 17 00:00:00 2001 From: Alexandru-Eusebiu Epure Date: Thu, 5 Oct 2017 17:25:14 +0300 Subject: [PATCH] REPO-1882: Create a group or a person with an id that contains backslash does not throw an error Added an char array containing illegall characters (/,\,\n,\r) to Groups and People API's, and adapted the checking of the previous illlegal character (/), to verify all of them. Added two tests (one for group and one for people), creating using invalid id's. --- .../alfresco/rest/api/impl/GroupsImpl.java | 13 +++-- .../alfresco/rest/api/impl/PeopleImpl.java | 52 ++++++++++--------- .../alfresco/rest/api/tests/GroupsTest.java | 9 ++++ .../alfresco/rest/api/tests/TestPeople.java | 21 +++++++- 4 files changed, 66 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java b/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java index 77dd0fc57a..22e824404e 100644 --- a/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java +++ b/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java @@ -86,6 +86,7 @@ public class GroupsImpl implements Groups private static final String DISPLAY_NAME = "displayName"; private static final String AUTHORITY_NAME = "authorityName"; private static final String ERR_MSG_MODIFY_FIXED_AUTHORITY = "Trying to modify a fixed authority"; + private static final char[] illegalCharacters = {'/', '\\', '\r', '\n'}; private final static Map SORT_PARAMS_TO_NAMES; static @@ -933,17 +934,21 @@ public class GroupsImpl implements Groups if (!isUpdate) { - if (group.getId() == null || group.getId().isEmpty()) + String groupId = group.getId(); + if (groupId == null || groupId.isEmpty()) { throw new InvalidArgumentException("groupId is null or empty"); } - if (group.getId().indexOf('/') != -1) + for (char illegalCharacter : illegalCharacters) { - throw new IllegalArgumentException("groupId contains characters that are not permitted."); + if (groupId.indexOf(illegalCharacter) != -1) + { + throw new IllegalArgumentException("groupId contains characters that are not permitted: "+groupId.charAt(groupId.indexOf(illegalCharacter))); + } } - if (groupAuthorityExists(group.getId())) + if (groupAuthorityExists(groupId)) { throw new ConstraintViolatedException("Group '" + group.getId() + "' already exists."); } diff --git a/src/main/java/org/alfresco/rest/api/impl/PeopleImpl.java b/src/main/java/org/alfresco/rest/api/impl/PeopleImpl.java index 10e09246fe..3dd8f92844 100644 --- a/src/main/java/org/alfresco/rest/api/impl/PeopleImpl.java +++ b/src/main/java/org/alfresco/rest/api/impl/PeopleImpl.java @@ -97,14 +97,15 @@ public class PeopleImpl implements People NamespaceService.SYSTEM_MODEL_1_0_URI, "http://www.alfresco.org/model/user/1.0", NamespaceService.CONTENT_MODEL_1_0_URI); - private static final List EXCLUDED_ASPECTS = Arrays.asList(); - private static final List EXCLUDED_PROPS = Arrays.asList(); + private static final List EXCLUDED_ASPECTS = Arrays.asList(); + private static final List EXCLUDED_PROPS = Arrays.asList(); private static final int USERNAME_MAXLENGTH = 100; private static final String[] RESERVED_AUTHORITY_PREFIXES = { PermissionService.GROUP_PREFIX, PermissionService.ROLE_PREFIX }; + private static final char[] illegalCharacters = {'/', '\\', '\r', '\n'}; protected Nodes nodes; protected Sites sites; @@ -544,10 +545,10 @@ public class PeopleImpl implements People return person; } - @Override - public Person create(Person person) - { - validateCreatePersonData(person); + @Override + public Person create(Person person) + { + validateCreatePersonData(person); if (! isAdminAuthority()) { @@ -556,15 +557,15 @@ public class PeopleImpl implements People throw new PermissionDeniedException(); } - // Unfortunately PersonService.createPerson(...) only throws an AlfrescoRuntimeException - // rather than a more specific exception and does not use a message ID either, so there's - // no sensible way to know that it was thrown due to the user already existing - hence this check here. - if (personService.personExists(person.getUserName())) - { - throw new ConstraintViolatedException("Person '"+person.getUserName()+"' already exists."); - } + // Unfortunately PersonService.createPerson(...) only throws an AlfrescoRuntimeException + // rather than a more specific exception and does not use a message ID either, so there's + // no sensible way to know that it was thrown due to the user already existing - hence this check here. + if (personService.personExists(person.getUserName())) + { + throw new ConstraintViolatedException("Person '" + person.getUserName() + "' already exists."); + } - // set enabled default value true + // set enabled default value true if (person.isEnabled() == null) { person.setEnabled(true); @@ -627,17 +628,17 @@ public class PeopleImpl implements People }); } - private void validateCreatePersonData(Person person) - { - // Mandatory field checks first - checkRequiredField("id", person.getUserName()); - checkRequiredField("firstName", person.getFirstName()); - checkRequiredField("email", person.getEmail()); - checkRequiredField("password", person.getPassword()); + private void validateCreatePersonData(Person person) + { + // Mandatory field checks first + checkRequiredField("id", person.getUserName()); + checkRequiredField("firstName", person.getFirstName()); + checkRequiredField("email", person.getEmail()); + checkRequiredField("password", person.getPassword()); validateUsername(person.getUserName()); validateNamespaces(person.getAspectNames(), person.getProperties()); - } + } private void validateUsername(String username) { @@ -646,9 +647,12 @@ public class PeopleImpl implements People throw new InvalidArgumentException("Username exceeds max length of " + USERNAME_MAXLENGTH + " characters."); } - if (username.indexOf('/') != -1) + for (char illegalCharacter : illegalCharacters) { - throw new IllegalArgumentException("Username contains characters that are not permitted."); + if (username.indexOf(illegalCharacter) != -1) + { + throw new IllegalArgumentException("Username contains characters that are not permitted: "+username.charAt(username.indexOf(illegalCharacter))); + } } for (String prefix : RESERVED_AUTHORITY_PREFIXES) diff --git a/src/test/java/org/alfresco/rest/api/tests/GroupsTest.java b/src/test/java/org/alfresco/rest/api/tests/GroupsTest.java index 5cd2532585..91bcedd9e6 100644 --- a/src/test/java/org/alfresco/rest/api/tests/GroupsTest.java +++ b/src/test/java/org/alfresco/rest/api/tests/GroupsTest.java @@ -1438,6 +1438,15 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest groupsProxy.createGroup(group, null, HttpServletResponse.SC_CONFLICT); } + // Create Group with an id that contains "\" should return an error. + { + setRequestContext(networkOne.getId(), networkAdmin, DEFAULT_ADMIN_PWD); + + Group group = new Group(); + group.setId("te\\st"); + groupsProxy.createGroup(group, null, HttpServletResponse.SC_BAD_REQUEST); + } + // Create subgroup with invalid parent. { setRequestContext(networkOne.getId(), networkAdmin, DEFAULT_ADMIN_PWD); diff --git a/src/test/java/org/alfresco/rest/api/tests/TestPeople.java b/src/test/java/org/alfresco/rest/api/tests/TestPeople.java index 14000dc273..34f6e606c3 100644 --- a/src/test/java/org/alfresco/rest/api/tests/TestPeople.java +++ b/src/test/java/org/alfresco/rest/api/tests/TestPeople.java @@ -608,7 +608,26 @@ public class TestPeople extends AbstractBaseApiTest people.create(person, 400); } } - + + /** + * Created for REPO-1882, '\\', '\r' and '\n' marked as invalid character for personId + */ + @Test + public void testCreatePersonWithInvalidCharacter () throws Exception + { + publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin")); + + Person person = new Person(); + String personId = UUID.randomUUID().toString()+"\\"; + person.setUserName(personId); + person.setFirstName("Joe"); + person.setEmail(personId+"@"+account1.getId()); + person.setEnabled(true); + person.setPassword("password123"); + + people.create(person, 400); + } + @Test public void testGetPerson_withCustomProps() throws PublicApiException {