From 99b3b93efa49c50dac1dd11e33810d1e9a02e792 Mon Sep 17 00:00:00 2001 From: open-schnick Date: Wed, 25 Nov 2020 11:10:16 +0100 Subject: [PATCH 1/4] FF-174 Create UserId generation --- .../user/business/UserBusinessService.java | 19 ++++++++++++++++++- .../business/UserBusinessServiceUnitTest.java | 12 +++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java b/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java index 9b55d35b..783afc23 100644 --- a/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java +++ b/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java @@ -38,6 +38,9 @@ public class UserBusinessService { private static final Logger LOG = LoggerFactory.getLogger(UserBusinessService.class); + public static final long USER_ID_MIN = 10000000; + public static final long USER_ID_MAX = 99999999; + @Value("${filefighter.disable-password-check}") public boolean passwordCheckDisabled; @@ -138,7 +141,7 @@ public void registerNewUser(UserRegisterForm newUser) { .username(username) .password(password) .refreshToken(AccessTokenBusinessService.generateRandomTokenValue()) - .userId(getUserCount() + 1) + .userId(generateRandomUserId()) .build()); } @@ -234,5 +237,19 @@ public void updateUser(long userId, UserRegisterForm userToUpdate, User authenti query.addCriteria(Criteria.where("userId").is(userId)); mongoTemplate.findAndModify(query, newUpdate, UserEntity.class); } + + public long generateRandomUserId(){ + long possibleUserId = 0L; + boolean userIdIsFree = false; + + while(!userIdIsFree){ + possibleUserId = (long) Math.floor(Math.random() * (USER_ID_MAX - USER_ID_MIN ))+ USER_ID_MIN; + UserEntity userEntity = userRepository.findByUserId(possibleUserId); + if(null == userEntity) + userIdIsFree = true; + } + + return possibleUserId; + } } diff --git a/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java b/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java index 3168422b..c38d425f 100644 --- a/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java +++ b/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java @@ -208,7 +208,7 @@ void registerNewUserWorks() { String username = "username"; String password = "validPassword1234"; String confPassword = "validPassword1234"; - long[] groups = null; + long[] groups = new long[]{0}; UserRegisterForm userRegisterForm = UserRegisterForm.builder() .username(username) @@ -291,7 +291,7 @@ void updatePasswordThrows() { assertThrows(UserNotUpdatedException.class, () -> userBusinessService.updateUser(userId, userRegisterForm, authenticatedUser), "Passwords do not match."); - String validPassword ="ValidPassword1234!="; + String validPassword = "ValidPassword1234!="; userRegisterForm.setPassword(validPassword); userRegisterForm.setConfirmationPassword(validPassword); when(userRepositoryMock.findByUserId(userId)).thenReturn(dummyEntity); @@ -325,7 +325,7 @@ void updateGroupsThrows() { assertThrows(UserNotUpdatedException.class, () -> userBusinessService.updateUser(userId, userRegisterForm, authenticatedUser)); - groups = new long[]{123032,1230213}; + groups = new long[]{123032, 1230213}; userRegisterForm.setGroupIds(groups); when(userRepositoryMock.findByUserId(userId)).thenReturn(dummyEntity); when(groupRepositoryMock.getGroupsByIds(groups)).thenThrow(new IllegalArgumentException("id doesnt belong to a group")); @@ -346,4 +346,10 @@ void updateGroupsWorks() { when(groupRepositoryMock.getGroupsByIds(groups)).thenReturn(new Groups[]{Groups.FAMILY}); assertDoesNotThrow(() -> userBusinessService.updateUser(userId, userRegisterForm, authenticatedUser)); } + + @Test + void generateRandomUserIdWorks() { + long actualValue = userBusinessService.generateRandomUserId(); + assertTrue(UserBusinessService.USER_ID_MIN <= actualValue && actualValue <= UserBusinessService.USER_ID_MAX); + } } \ No newline at end of file From 3d30cd38b8e0fd4786c1ee2954b359ecdde92e07 Mon Sep 17 00:00:00 2001 From: open-schnick Date: Wed, 25 Nov 2020 11:19:07 +0100 Subject: [PATCH 2/4] Rewrote decoding exception catch. --- .../domain/user/business/UserAuthorizationService.java | 8 ++++---- .../user/business/UserAuthorizationServiceUnitTest.java | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java b/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java index cff28fae..df413009 100644 --- a/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java +++ b/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java @@ -12,7 +12,6 @@ import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; -import java.io.UnsupportedEncodingException; import java.nio.charset.StandardCharsets; import java.util.Base64; @@ -39,9 +38,10 @@ public User authenticateUserWithUsernameAndPassword(String base64encodedUserAndP String decodedUsernameAndPassword = ""; try { byte[] decodedValue = Base64.getDecoder().decode(base64encodedUserAndPassword); - decodedUsernameAndPassword = new String(decodedValue, StandardCharsets.UTF_8.toString()); - } catch (UnsupportedEncodingException ex) { - LOG.warn("Found UnsupportedEncodingException {} in {}",ex.getMessage(), base64encodedUserAndPassword); + decodedUsernameAndPassword = new String(decodedValue, StandardCharsets.UTF_8); + } catch (IllegalArgumentException ex) { + LOG.warn("Found {} in {}",ex.getMessage(), base64encodedUserAndPassword); + throw new RequestDidntMeetFormalRequirementsException("Found unsupported character in header."); } String[] split = decodedUsernameAndPassword.split(":"); diff --git a/src/test/java/de/filefighter/rest/domain/user/business/UserAuthorizationServiceUnitTest.java b/src/test/java/de/filefighter/rest/domain/user/business/UserAuthorizationServiceUnitTest.java index f13dbb90..f2714058 100644 --- a/src/test/java/de/filefighter/rest/domain/user/business/UserAuthorizationServiceUnitTest.java +++ b/src/test/java/de/filefighter/rest/domain/user/business/UserAuthorizationServiceUnitTest.java @@ -29,9 +29,10 @@ void authenticateUserWithUsernameAndPasswordThrows() { String matchesButDoesNotMeetRequirements = AUTHORIZATION_BASIC_PREFIX + "dWdhYnVnYQ=="; String matchesButUserWasNotFound = AUTHORIZATION_BASIC_PREFIX + "dXNlcjpwYXNzd29yZA=="; - assertThrows(RuntimeException.class, () -> + assertThrows(RequestDidntMeetFormalRequirementsException.class, () -> userAuthorizationService.authenticateUserWithUsernameAndPassword(matchesButIsNotSupportedEncoding) ); + assertThrows(RequestDidntMeetFormalRequirementsException.class, () -> userAuthorizationService.authenticateUserWithUsernameAndPassword(matchesButDoesNotMeetRequirements) ); From 42fcb2f247856d09407adf5de9eb632d861ffed3 Mon Sep 17 00:00:00 2001 From: open-schnick Date: Wed, 25 Nov 2020 11:21:09 +0100 Subject: [PATCH 3/4] Rearranged Code --- .../rest/domain/user/business/UserAuthorizationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java b/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java index df413009..4f60fdf3 100644 --- a/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java +++ b/src/main/java/de/filefighter/rest/domain/user/business/UserAuthorizationService.java @@ -40,7 +40,7 @@ public User authenticateUserWithUsernameAndPassword(String base64encodedUserAndP byte[] decodedValue = Base64.getDecoder().decode(base64encodedUserAndPassword); decodedUsernameAndPassword = new String(decodedValue, StandardCharsets.UTF_8); } catch (IllegalArgumentException ex) { - LOG.warn("Found {} in {}",ex.getMessage(), base64encodedUserAndPassword); + LOG.warn("Found {} in {}", ex.getMessage(), base64encodedUserAndPassword); throw new RequestDidntMeetFormalRequirementsException("Found unsupported character in header."); } From 15b2e52e73477eab5e9629fac3935a62990ea5e5 Mon Sep 17 00:00:00 2001 From: open-schnick Date: Wed, 25 Nov 2020 11:38:14 +0100 Subject: [PATCH 4/4] Fixed Security Issue. --- .../rest/domain/user/business/UserBusinessService.java | 8 ++++---- .../domain/user/business/UserBusinessServiceUnitTest.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java b/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java index 783afc23..41c0a4e3 100644 --- a/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java +++ b/src/main/java/de/filefighter/rest/domain/user/business/UserBusinessService.java @@ -22,6 +22,7 @@ import org.springframework.data.mongodb.core.query.Update; import org.springframework.stereotype.Service; +import java.security.SecureRandom; import java.util.Arrays; import java.util.regex.Pattern; @@ -38,8 +39,7 @@ public class UserBusinessService { private static final Logger LOG = LoggerFactory.getLogger(UserBusinessService.class); - public static final long USER_ID_MIN = 10000000; - public static final long USER_ID_MAX = 99999999; + public static final int USER_ID_MAX = 99999999; @Value("${filefighter.disable-password-check}") @@ -243,9 +243,9 @@ public long generateRandomUserId(){ boolean userIdIsFree = false; while(!userIdIsFree){ - possibleUserId = (long) Math.floor(Math.random() * (USER_ID_MAX - USER_ID_MIN ))+ USER_ID_MIN; + possibleUserId = new SecureRandom().nextInt(UserBusinessService.USER_ID_MAX); UserEntity userEntity = userRepository.findByUserId(possibleUserId); - if(null == userEntity) + if(null == userEntity && possibleUserId > 0) userIdIsFree = true; } diff --git a/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java b/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java index c38d425f..fb1c9c54 100644 --- a/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java +++ b/src/test/java/de/filefighter/rest/domain/user/business/UserBusinessServiceUnitTest.java @@ -350,6 +350,6 @@ void updateGroupsWorks() { @Test void generateRandomUserIdWorks() { long actualValue = userBusinessService.generateRandomUserId(); - assertTrue(UserBusinessService.USER_ID_MIN <= actualValue && actualValue <= UserBusinessService.USER_ID_MAX); + assertTrue(0 <= actualValue && actualValue <= UserBusinessService.USER_ID_MAX); } } \ No newline at end of file