Fix null username passed to addUserToGroup and refactor addUser username resolution

commons/src/main/java/com/deftdevs/bootstrapi/commons/model/UserModel.java

@@ -2,14 +2,14 @@
 
 import com.deftdevs.bootstrapi.commons.constants.BootstrAPI;
 import lombok.AllArgsConstructor;
+import lombok.Builder;
 import lombok.Data;
 import lombok.EqualsAndHashCode;
-import lombok.Builder;
 import lombok.NoArgsConstructor;
 
 import javax.xml.bind.annotation.XmlElement;
 import javax.xml.bind.annotation.XmlRootElement;
-import java.util.List;
+import java.util.Map;
 
 /**
  * Model for users REST requests.
@@ -44,7 +44,7 @@ public class UserModel {
     private String password;
 
     @XmlElement
-    private List<GroupModel> groups;
+    private Map<String, GroupModel> groups;
 
     // Example instances for documentation and tests
 

confluence/Models/UserModel.md

@@ -10,7 +10,7 @@
 | **email** | **String** |  | [optional] [default to null] |
 | **active** | **Boolean** |  | [optional] [default to null] |
 | **password** | **String** |  | [optional] [default to null] |
-| **groups** | [**List**](GroupModel.md) |  | [optional] [default to null] |
+| **groups** | [**Map**](GroupModel.md) |  | [optional] [default to null] |
 
 [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)
 

crowd/Models/UserModel.md

@@ -10,7 +10,7 @@
 | **email** | **String** |  | [optional] [default to null] |
 | **active** | **Boolean** |  | [optional] [default to null] |
 | **password** | **String** |  | [optional] [default to null] |
-| **groups** | [**List**](GroupModel.md) |  | [optional] [default to null] |
+| **groups** | [**Map**](GroupModel.md) |  | [optional] [default to null] |
 
 [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)
 

crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/DirectoriesServiceImpl.java

@@ -172,7 +172,10 @@ List<Directory> findAllDirectories() {
         return directoryManager.searchDirectories(allDirectoriesEntityQuery);
     }
 
-    private void handleGroupsAndUsers(AbstractDirectoryModel directoryModel, AbstractDirectoryModel resultDirectoryModel) {
+    private void handleGroupsAndUsers(
+            final AbstractDirectoryModel directoryModel,
+            final AbstractDirectoryModel resultDirectoryModel) {
+
         if (DirectoryInternalModel.class.equals(directoryModel.getClass()) && directoryModel.getClass().equals(resultDirectoryModel.getClass())) {
             final DirectoryInternalModel directoryInternalModel = (DirectoryInternalModel) directoryModel;
             final DirectoryInternalModel resultDirectoryInternalModel = (DirectoryInternalModel) resultDirectoryModel;

crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceImpl.java

@@ -27,6 +27,7 @@
 import java.time.LocalDateTime;
 import java.time.ZoneId;
 import java.util.*;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import static com.atlassian.crowd.model.user.UserConstants.*;
@@ -103,22 +104,20 @@ UserModel addUser(
             final String username,
             final UserModel userModel) {
 
-        final User existingUser = findUser(directoryId, userModel.getUsername());
+        final String effectiveUsername = userModel.getUsername() != null ? userModel.getUsername() : username;
 
-        if (existingUser != null) {
-            throw new BadRequestException(String.format("User '%s' already exists", userModel.getUsername()));
+        if (effectiveUsername == null) {
+            throw new BadRequestException("Cannot create user, username is required");
         }
 
-        if (userModel.getUsername() == null) {
-            if (username == null) {
-                throw new BadRequestException("Cannot create user, username is required");
-            }
-
-            userModel.setUsername(username);
+        if (username != null && !effectiveUsername.equals(username)) {
+            throw new BadRequestException("Cannot create user, two different usernames provided");
         }
 
-        if (username != null && !userModel.getUsername().equals(username)) {
-            throw new BadRequestException("Cannot create user, two different usernames provided");
+        final User existingUser = findUser(directoryId, effectiveUsername);
+
+        if (existingUser != null) {
+            throw new BadRequestException(String.format("User '%s' already exists", effectiveUsername));
         }
 
         if (userModel.getFirstName() == null || userModel.getLastName() == null || userModel.getFullName() == null || userModel.getEmail() == null) {
@@ -129,7 +128,7 @@ UserModel addUser(
             throw new BadRequestException("Cannot create user, password is required");
         }
 
-        final UserTemplateWithAttributes userTemplate = new UserTemplateWithAttributes(userModel.getUsername(), directoryId);
+        final UserTemplateWithAttributes userTemplate = new UserTemplateWithAttributes(effectiveUsername, directoryId);
         userTemplate.setFirstName(userModel.getFirstName());
         userTemplate.setLastName(userModel.getLastName());
         userTemplate.setDisplayName(userModel.getFullName());
@@ -140,7 +139,7 @@ UserModel addUser(
         final UserModel resultUserModel = UserModelUtil.toUserModel(user);
 
         if (userModel.getGroups() != null) {
-            final List<GroupModel> resultGroupModels = addUserToGroups(directoryId, userModel.getUsername(), userModel.getGroups());
+            final Map<String, GroupModel> resultGroupModels = addUserToGroups(directoryId, effectiveUsername, userModel.getGroups());
             resultUserModel.setGroups(resultGroupModels);
         }
 
@@ -173,7 +172,7 @@ UserModel updateUser(
         final UserModel resultUserModel = UserModelUtil.toUserModel(user);
 
         if (userModel.getGroups() != null) {
-            final List<GroupModel> resultGroupModels = addUserToGroups(directoryId, userModel.getUsername(), userModel.getGroups());
+            final Map<String, GroupModel> resultGroupModels = addUserToGroups(directoryId, user.getName(), userModel.getGroups());
             resultUserModel.setGroups(resultGroupModels);
         }
 
@@ -395,19 +394,19 @@ void resetUserPasswordAttributes(
         }
     }
 
-    List<GroupModel> addUserToGroups(
+    Map<String, GroupModel> addUserToGroups(
             final long directoryId,
             final String username,
-            final List<GroupModel> groupModels) {
+            final Map<String, GroupModel> groupModels) {
 
         final List<GroupModel> resultGroupModels = new ArrayList<>();
 
         if (groupModels != null) {
-            for (GroupModel groupModel : groupModels) {
-                final GroupModel resultGroupModel = groupsService.setGroup(directoryId, groupModel.getName(), groupModel);
+            for (Map.Entry<String, GroupModel> groupModelEntry : groupModels.entrySet()) {
+                final GroupModel resultGroupModel = groupsService.setGroup(directoryId, groupModelEntry.getKey(), groupModelEntry.getValue());
 
                 try {
-                    directoryManager.addUserToGroup(directoryId, username, groupModel.getName());
+                    directoryManager.addUserToGroup(directoryId, username, resultGroupModel.getName());
                     resultGroupModels.add(resultGroupModel);
                 } catch (DirectoryPermissionException | ReadOnlyGroupException e) {
                     throw new BadRequestException(e);
@@ -421,7 +420,7 @@ List<GroupModel> addUserToGroups(
             }
         }
 
-        return resultGroupModels;
+        return resultGroupModels.stream().collect(Collectors.toMap(GroupModel::getName, Function.identity()));
     }
 
     private static UserTemplate getUserTemplate(

crowd/src/test/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceTest.java

@@ -27,6 +27,9 @@
 
 import javax.ws.rs.WebApplicationException;
 import java.util.*;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static com.atlassian.crowd.model.user.UserConstants.*;
 import static org.junit.jupiter.api.Assertions.*;
@@ -128,9 +131,8 @@ public void testSetUsers() {
         final UserModel userModel = UserModelUtil.toUserModel(user);
         userModel.setPassword("s3cr3t");
 
-        final Map<String, UserModel> userModels = new LinkedHashMap<>();
-        userModels.put(userModel.getUsername(), userModel);
-        userModels.put(UserModel.EXAMPLE_1.getUsername(), UserModel.EXAMPLE_1);
+        final Map<String, UserModel> userModels = Stream.of(userModel, UserModel.EXAMPLE_1)
+                .collect(Collectors.toMap(UserModel::getUsername, Function.identity()));
         final UsersServiceImpl spy = spy(usersService);
         doAnswer(invocation -> invocation.getArguments()[1]).when(spy).setUser(anyLong(), any());
 
@@ -140,7 +142,7 @@ public void testSetUsers() {
 
     @Test
     public void testSetUsersNull() {
-        assertEquals(Collections.emptyMap(), usersService.setUsers(getTestDirectory().getId(), (Map<String, UserModel>) null));
+        assertEquals(Collections.emptyMap(), usersService.setUsers(getTestDirectory().getId(), null));
     }
 
     @Test
@@ -179,9 +181,10 @@ public void testAddUserActiveByDefault() throws CrowdException, DirectoryPermiss
     public void testAddUserWithGroups() throws CrowdException, DirectoryPermissionException {
         // return the same user as the one we are adding
         doAnswer(invocation -> invocation.getArguments()[1]).when(directoryManager).addUser(anyLong(), any(), any());
+        doAnswer(invocation -> invocation.getArguments()[2]).when(groupsService).setGroup(anyLong(), anyString(), any());
 
         final UserModel userModel = UserModelUtil.toUserModel(getTestUser());
-        final List<GroupModel> groupModels = Collections.singletonList(GroupModel.EXAMPLE_1);
+        final Map<String, GroupModel> groupModels = Stream.of(GroupModel.EXAMPLE_1).collect(Collectors.toMap(GroupModel::getName, Function.identity()));
         userModel.setPassword("12345");
         userModel.setGroups(groupModels);
 
@@ -215,8 +218,6 @@ public void testAddUserNoName() throws CrowdException {
     @Test
     public void testAddUserTwoDifferentNames() throws CrowdException {
         final User user = getTestUser();
-        doReturn(user).when(directoryManager).findUserByName(user.getDirectoryId(), user.getName());
-
         final UserModel userModel = UserModelUtil.toUserModel(user);
 
         assertThrows(BadRequestException.class, () -> {
@@ -279,15 +280,34 @@ public void updateUserWithGroups() throws CrowdException, DirectoryPermissionExc
         doReturn(user).when(directoryManager).findUserByName(user.getDirectoryId(), user.getName());
         // return the same user as the one we are updating
         doAnswer(invocation -> invocation.getArguments()[1]).when(directoryManager).updateUser(anyLong(), any());
+        doAnswer(invocation -> invocation.getArguments()[2]).when(groupsService).setGroup(anyLong(), anyString(), any());
 
         final UserModel userModel = UserModelUtil.toUserModel(getTestUser());
-        final List<GroupModel> groupModels = Collections.singletonList(GroupModel.EXAMPLE_1);
+        final Map<String, GroupModel> groupModels = Stream.of(GroupModel.EXAMPLE_1)
+                .collect(Collectors.toMap(GroupModel::getName, Function.identity()));
         userModel.setGroups(groupModels);
 
         usersService.updateUser(1L, user.getName(), userModel);
         verify(groupsService, times(groupModels.size())).setGroup(anyLong(), anyString(), any());
     }
 
+    @Test
+    public void updateUserWithGroupsAndNoUsernameInModel() throws CrowdException, DirectoryPermissionException {
+        final User user = getTestUser();
+        doReturn(user).when(directoryManager).findUserByName(user.getDirectoryId(), user.getName());
+        // return the same user as the one we are updating
+        doAnswer(invocation -> invocation.getArguments()[1]).when(directoryManager).updateUser(anyLong(), any());
+        doAnswer(invocation -> invocation.getArguments()[2]).when(groupsService).setGroup(anyLong(), anyString(), any());
+
+        final UserModel userModel = UserModel.builder()
+                .fullName("Other Full Name")
+                .groups(Stream.of(GroupModel.EXAMPLE_1).collect(Collectors.toMap(GroupModel::getName, Function.identity())))
+                .build();
+
+        usersService.updateUser(1L, user.getName(), userModel);
+        verify(directoryManager).addUserToGroup(anyLong(), eq(user.getName()), anyString());
+    }
+
     @Test
     public void testUpdateUserNoOp() throws CrowdException, PermissionException {
         final User user = getTestUser();

jira/Models/UserModel.md

@@ -10,7 +10,7 @@
 | **email** | **String** |  | [optional] [default to null] |
 | **active** | **Boolean** |  | [optional] [default to null] |
 | **password** | **String** |  | [optional] [default to null] |
-| **groups** | [**List**](GroupModel.md) |  | [optional] [default to null] |
+| **groups** | [**Map**](GroupModel.md) |  | [optional] [default to null] |
 
 [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)