RED-10619: Multiple bugs when creating or editing users

src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java

@@ -239,33 +239,67 @@ public class UserService {
     public void validateSufficientRoles(String userId, Set<String> userRoles, Set<String> newRoles, Set<String> currentUserRoles) {
 
         var roleMapping = tenantApplicationTypeService.getCurrentProperties().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<String> 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<String> 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<String> untouchableRoles, Set<String> 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.");
         }
     }
@@ -655,6 +689,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);

src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java

@@ -30,8 +30,10 @@ 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.TenantApplicationTypeService;
+import com.knecon.fforesight.tenantusermanagement.service.UserService;
 
 import feign.FeignException;
+import lombok.NonNull;
 import lombok.SneakyThrows;
 
 public class UserTest extends AbstractTenantUserManagementIntegrationTest {
@@ -48,6 +50,9 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest {
     @Autowired
     private TenantApplicationTypeService tenantApplicationTypeService;
 
+    @Autowired
+    private UserService userService;
+
 
     @Test
     public void testUsers() {
@@ -588,12 +593,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);
@@ -708,6 +717,113 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest {
     }
 
 
+    @Test
+    public void testUpdateProfileForUserWithAllRoles() {
+
+        TenantContext.setTenantId(AbstractTenantUserManagementIntegrationTest.TEST_TENANT_ID);
+        tokenService.setUser("admin@knecon.com", "secret");
+
+        var allRoles = tenantApplicationTypeService.getCurrentProperties().getKcRoleMapping().getAllRoles();
+        Set<String> 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 = tenantApplicationTypeService.getCurrentProperties().getKcRoleMapping().getAllRoles();
+        Set<String> 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<User> allUsers = userClient.getAllUsers(true);
+        assertTrue(allUsers.stream()
+                           .noneMatch(u -> u.getUserId().equals(kneconAdminuser.getUserId())));
+        List<User> 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();
@@ -730,7 +846,11 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest {
 
     private void addRoles(String userId, Set<String> roles) {
 
-        getUserResource(userId).roles().realmLevel().add(roles.stream().map(this::getRoleRepresentation).toList());
+        getUserResource(userId).roles()
+                .realmLevel()
+                .add(roles.stream()
+                             .map(this::getRoleRepresentation)
+                             .toList());
     }
 
 }