From e11413beda8cf91da635ddcb6cf2ebcef16b8353 Mon Sep 17 00:00:00 2001 From: Maverick Studer Date: Tue, 10 Dec 2024 15:02:16 +0100 Subject: [PATCH] RED-10619: Multiple bugs when creating or editing users --- .../service/UserService.java | 57 ++++++-- .../tenantusermanagement/tests/UserTest.java | 130 +++++++++++++++++- 2 files changed, 171 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java b/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java index 1806e39..fc11b68 100644 --- a/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java +++ b/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java @@ -237,33 +237,67 @@ public class UserService { public void validateSufficientRoles(String userId, Set userRoles, Set newRoles, Set currentUserRoles) { var roleMapping = tenantUserManagementProperties.getKcRoleMapping(); - var maxRank = currentUserRoles.stream() + + int maxCurrentUserRank = currentUserRoles.stream() .map(r -> roleMapping.getRole(r).getRank()) .max(Integer::compare) .orElse(-1); - var newRolesRank = newRoles.stream() - .map(r -> roleMapping.getRole(r).getRank()) - .toList(); - var maxNewRolesRank = newRolesRank.stream() - .max(Integer::compare) - .orElse(-1); - var untouchableRoles = userRoles.stream() + Set untouchableRoles = userRoles.stream() .filter(roleMapping::isValidRole) .map(roleMapping::getRole) - .filter(r -> r.getRank() > maxRank || ApplicationRoles.isKneconRole(r.getName())) + .filter(r -> r.getRank() > maxCurrentUserRank && !ApplicationRoles.isKneconRole(r.getName())) .map(KCRole::getName) .collect(Collectors.toSet()); - if (maxNewRolesRank > maxRank) { + Set kneconRoles = userRoles.stream() + .filter(roleMapping::isValidRole) + .map(roleMapping::getRole) + .map(KCRole::getName) + .filter(ApplicationRoles::isKneconRole) + .collect(Collectors.toSet()); + + int maxNewRolesRank = newRoles.stream() + .map(r -> roleMapping.getRole(r).getRank()) + .max(Integer::compare) + .orElse(-1); + + newRoles.addAll(kneconRoles); + + int maxNewRolesRankIncludingKnecon = newRoles.stream() + .map(r -> roleMapping.getRole(r).getRank()) + .max(Integer::compare) + .orElse(-1); + + ensureNoHigherRankAssigned(maxCurrentUserRank, maxNewRolesRank); + ensureUntouchableRolesPreserved(untouchableRoles, newRoles); + ensureHighestRankNotRemovedFromSelf(userId, maxCurrentUserRank, maxNewRolesRankIncludingKnecon, roleMapping.getMaxRank()); + } + + + private void ensureNoHigherRankAssigned(int maxCurrentUserRank, int maxNewRolesRank) { + + if (maxNewRolesRank > maxCurrentUserRank) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Cannot assign this role to that user. Insufficient rights"); } + } + + + private void ensureUntouchableRolesPreserved(Set untouchableRoles, Set newRoles) { if (!newRoles.containsAll(untouchableRoles)) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Cannot modify some roles for this user. Insufficient rights"); } + } - if (userId.equalsIgnoreCase(KeycloakSecurity.getUserId()) && maxRank.equals(roleMapping.getMaxRank()) && !maxNewRolesRank.equals(maxRank)) { + + private void ensureHighestRankNotRemovedFromSelf(String userId, int maxCurrentUserRank, int maxNewRolesRankIncludingKnecon, int overallMaxRank) { + + boolean isSelf = userId.equalsIgnoreCase(KeycloakSecurity.getUserId()); + boolean isUserHighestRank = maxCurrentUserRank == overallMaxRank; + boolean highestRankRemoved = !Integer.valueOf(maxNewRolesRankIncludingKnecon).equals(maxCurrentUserRank); + + if (isSelf && isUserHighestRank && highestRankRemoved) { throw new ResponseStatusException(HttpStatus.CONFLICT, "Cannot remove highest ranking role from self."); } } @@ -653,6 +687,7 @@ public class UserService { .max(Integer::compare) .orElse(-1); var targetRank = userRoles.stream() + .filter(ApplicationRoles::isNoKneconRole) .map(r -> roleMapping.getRole(r).getRank()) .max(Integer::compare) .orElse(-1); diff --git a/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java b/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java index 8e93622..7c3c357 100644 --- a/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java +++ b/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java @@ -29,8 +29,10 @@ import com.knecon.fforesight.tenantusermanagement.model.User; import com.knecon.fforesight.tenantusermanagement.permissions.ApplicationRoles; import com.knecon.fforesight.tenantusermanagement.properties.TenantUserManagementProperties; import com.knecon.fforesight.tenantusermanagement.service.RealmService; +import com.knecon.fforesight.tenantusermanagement.service.UserService; import feign.FeignException; +import lombok.NonNull; import lombok.SneakyThrows; public class UserTest extends AbstractTenantUserManagementIntegrationTest { @@ -44,6 +46,9 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { @Autowired private TenantUserManagementProperties tenantUserManagementProperties; + @Autowired + private UserService userService; + @Test public void testUsers() { @@ -501,11 +506,11 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { tokenService.setUser("admin@knecon.com", "secret"); // this should still not be possible - e = assertThrows(FeignException.class, () -> userClient.setRoles(onlyKneconUser.getUserId(), allRoles)); + e = assertThrows(FeignException.class, () -> userClient.setRoles(onlyKneconUser.getUserId(), allRoles)); assertEquals(404, e.status()); // and also not this - e = assertThrows(FeignException.class, () -> userClient.setRoles(user.getUserId(), allRoles)); + e = assertThrows(FeignException.class, () -> userClient.setRoles(user.getUserId(), allRoles)); assertEquals(400, e.status()); // we can also poll the user @@ -584,12 +589,16 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { addRoles(user4.getUserId(), allButKneconRoles); allUsers = userClient.getAllUsers(true); - var user4AfterShenanigansOpt = allUsers.stream().filter(u -> u.getUserId().equals(user4.getUserId())).findFirst(); + var user4AfterShenanigansOpt = allUsers.stream() + .filter(u -> u.getUserId().equals(user4.getUserId())) + .findFirst(); assertTrue(user4AfterShenanigansOpt.isPresent()); user4AfterShenanigansOpt.get().setRoles(new HashSet<>()); assertEquals(user4AfterShenanigansOpt.get(), user4); - var stillOnlyKneconUserOpt = allUsers.stream().filter(u -> u.getUserId().equals(onlyKneconUser.getUserId())).findFirst(); + var stillOnlyKneconUserOpt = allUsers.stream() + .filter(u -> u.getUserId().equals(onlyKneconUser.getUserId())) + .findFirst(); assertTrue(stillOnlyKneconUserOpt.isPresent()); stillOnlyKneconUserOpt.get().setRoles(new HashSet<>()); assertEquals(stillOnlyKneconUserOpt.get(), onlyKneconUser); @@ -704,6 +713,113 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { } + @Test + public void testUpdateProfileForUserWithAllRoles() { + + TenantContext.setTenantId(AbstractTenantUserManagementIntegrationTest.TEST_TENANT_ID); + tokenService.setUser("admin@knecon.com", "secret"); + + var allRoles = tenantUserManagementProperties.getKcRoleMapping().getAllRoles(); + Set allButKneconRoles = allRoles.stream() + .filter(ApplicationRoles::isNoKneconRole) + .collect(Collectors.toSet()); + + CreateUserRequest createUserRequest = new CreateUserRequest(); + createUserRequest.setEmail("all.roles.user@knecon.com"); + createUserRequest.setUsername("all.roles.user@knecon.com"); + createUserRequest.setFirstName("All"); + createUserRequest.setLastName("Roles"); + + var allRolesuser = userClient.createUser(createUserRequest); + addRoles(allRolesuser.getUserId(), allRoles); + assertThat(allRolesuser).isNotNull(); + + UpdateProfileRequest updateProfileRequest = UpdateProfileRequest.builder() + .email("all.roles.user@knecon.com") + .firstName("All") + .lastName("NewLastName") + .roles(allButKneconRoles) + .build(); + + var updatedUser = userClient.updateProfile(allRolesuser.getUserId(), updateProfileRequest); + + assertThat(updatedUser).isNotNull(); + assertThat(updatedUser.getLastName()).isEqualTo("NewLastName"); + + tokenService.setUser("test@fforesight.com", "secret"); + + updateProfileRequest.setLastName("AnotherNewLastName"); + updatedUser = userClient.updateProfile(allRolesuser.getUserId(), updateProfileRequest); + + assertThat(updatedUser).isNotNull(); + assertThat(updatedUser.getLastName()).isEqualTo("AnotherNewLastName"); + + createUserRequest.setEmail("less.super.user.1@knecon.com"); + createUserRequest.setUsername(createUserRequest.getEmail()); + createUserRequest.setRoles(Set.of("LESS_SUPER_USER")); + var lessSuperUser = userClient.createUser(createUserRequest); + + userClient.resetPassword(lessSuperUser.getUserId(), ResetPasswordRequest.builder().password("Secret@secured!23").build()); + tokenService.setUser("less.super.user.1@knecon.com", "Secret@secured!23"); + + FeignException feignException = assertThrows(FeignException.class, () -> userClient.updateProfile(allRolesuser.getUserId(), updateProfileRequest)); + assertEquals(400, feignException.status()); + assertTrue(feignException.getMessage().contains("Cannot assign this role to that user. Insufficient rights")); + + tokenService.setUser("admin@knecon.com", "secret"); + userClient.deleteUser(lessSuperUser.getUserId()); + userClient.deleteUser(allRolesuser.getUserId()); + } + + + @Test + public void testDeleteKneconRolesUserAsNormalAdmin() { + + TenantContext.setTenantId(AbstractTenantUserManagementIntegrationTest.TEST_TENANT_ID); + tokenService.setUser("admin@knecon.com", "secret"); + + var allRoles = tenantUserManagementProperties.getKcRoleMapping().getAllRoles(); + Set allButKneconRoles = allRoles.stream() + .filter(ApplicationRoles::isNoKneconRole) + .collect(Collectors.toSet()); + + CreateUserRequest createUserRequest = new CreateUserRequest(); + createUserRequest.setEmail("normalAdmin@knecon.com"); + createUserRequest.setUsername("normalAdmin@knecon.com"); + createUserRequest.setFirstName("Mister"); + createUserRequest.setLastName("Admin"); + + var adminUser = userClient.createUser(createUserRequest); + addRoles(adminUser.getUserId(), allButKneconRoles); + assertThat(adminUser).isNotNull(); + + createUserRequest = new CreateUserRequest(); + createUserRequest.setEmail("kneconAdmin@knecon.com"); + createUserRequest.setUsername("kneconAdmin@knecon.com"); + createUserRequest.setFirstName("Knecon"); + createUserRequest.setLastName("Admin"); + + var kneconAdminuser = userClient.createUser(createUserRequest); + addRoles(kneconAdminuser.getUserId(), allRoles); + assertThat(kneconAdminuser).isNotNull(); + + userClient.resetPassword(adminUser.getUserId(), ResetPasswordRequest.builder().password("Secret@secured!23").build()); + tokenService.setUser("normalAdmin@knecon.com", "Secret@secured!23"); + + userClient.deleteUser(kneconAdminuser.getUserId()); + + List allUsers = userClient.getAllUsers(true); + assertTrue(allUsers.stream() + .noneMatch(u -> u.getUserId().equals(kneconAdminuser.getUserId()))); + List unfilteredUsers = userService.getAllUsers(); + assertTrue(unfilteredUsers.stream() + .anyMatch(u -> u.getUserId().equals(kneconAdminuser.getUserId()))); + + tokenService.setUser("admin@knecon.com", "secret"); + userClient.deleteUser(adminUser.getUserId()); + } + + private UsersResource getTenantUsersResource() { return realmService.realm(TenantContext.getTenantId()).users(); @@ -726,7 +842,11 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { private void addRoles(String userId, Set roles) { - getUserResource(userId).roles().realmLevel().add(roles.stream().map(this::getRoleRepresentation).toList()); + getUserResource(userId).roles() + .realmLevel() + .add(roles.stream() + .map(this::getRoleRepresentation) + .toList()); } }