From 8f64781a0031858fa9fd384c09367cd5e772c988 Mon Sep 17 00:00:00 2001 From: Patrick Hobusch Date: Mon, 11 May 2026 16:22:40 +0800 Subject: [PATCH] Fix three null-safety bugs introduced with the map-based endpoints refactor - setUsers passed map values directly to setUser, which called findUser with a null username when the UserModel had no username field set; now passes the map key as the lookup username via a new setUser(directoryId, username, model) overload, with the model's username used only as the desired new name on update (rename) - addUserToGroups silently omitted groups from the response when the user was already a member (MembershipAlreadyExistsException); the group is now included - ApplicationLinkModelUtil.toApplicationLinkDetails passed a null Boolean to ApplicationLinkDetails.Builder.isPrimary(boolean), causing NPE when the primary field is absent in the request; now defaults to false via Boolean.TRUE.equals() --- .../model/util/ApplicationLinkModelUtil.java | 2 +- .../util/ApplicationLinkModelUtilTest.java | 14 +++++ .../crowd/service/UsersServiceImpl.java | 20 +++++-- .../crowd/service/UsersServiceTest.java | 55 +++++++++++++++++-- 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/commons/src/main/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtil.java b/commons/src/main/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtil.java index 47825241..43a7faac 100644 --- a/commons/src/main/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtil.java +++ b/commons/src/main/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtil.java @@ -79,7 +79,7 @@ public static ApplicationLinkDetails toApplicationLinkDetails( .name(applicationLinkModel.getName()) .displayUrl(applicationLinkModel.getDisplayUrl()) .rpcUrl(applicationLinkModel.getRpcUrl()) - .isPrimary(applicationLinkModel.getPrimary()) + .isPrimary(Boolean.TRUE.equals(applicationLinkModel.getPrimary())) .build(); } diff --git a/commons/src/test/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtilTest.java b/commons/src/test/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtilTest.java index 4075bad1..9a60d500 100644 --- a/commons/src/test/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtilTest.java +++ b/commons/src/test/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtilTest.java @@ -59,6 +59,20 @@ void testToApplicationLinkDetails() throws Exception { assertEquals(bean.getPrimary(), linkDetails.isPrimary()); } + @Test + void testToApplicationLinkDetailsWithNullPrimary() throws Exception { + final ApplicationLinkModel bean = ApplicationLinkModel.builder() + .name(ApplicationLinkModel.EXAMPLE_1.getName()) + .displayUrl(ApplicationLinkModel.EXAMPLE_1.getDisplayUrl()) + .rpcUrl(ApplicationLinkModel.EXAMPLE_1.getRpcUrl()) + .primary(null) + .build(); + final ApplicationLinkDetails linkDetails = ApplicationLinkModelUtil.toApplicationLinkDetails(bean); + + assertNotNull(linkDetails); + assertFalse(linkDetails.isPrimary()); + } + @Test void testToApplicationLinkAuthTypeDisabled() { assertEquals(ApplicationLinkModel.ApplicationLinkAuthType.DISABLED, diff --git a/crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceImpl.java b/crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceImpl.java index 51a0028f..4d0330b7 100644 --- a/crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceImpl.java +++ b/crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceImpl.java @@ -66,10 +66,22 @@ public UserModel setUser( final long directoryId, final UserModel userModel) { - final User user = findUser(directoryId, userModel.getUsername()); + if (userModel.getUsername() == null) { + throw new BadRequestException("Cannot set user, username is required"); + } + + return setUser(directoryId, userModel.getUsername(), userModel); + } + + UserModel setUser( + final long directoryId, + final String username, + final UserModel userModel) { + + final User user = findUser(directoryId, username); if (user == null) { - return addUser(directoryId, userModel); + return addUser(directoryId, username, userModel); } return updateUser(directoryId, user.getName(), userModel); @@ -86,7 +98,7 @@ public Map setUsers( final Map resultUserModels = new LinkedHashMap<>(); for (Map.Entry entry : userModels.entrySet()) { - final UserModel resultUserModel = setUser(directoryId, entry.getValue()); + final UserModel resultUserModel = setUser(directoryId, entry.getKey(), entry.getValue()); resultUserModels.put(resultUserModel.getUsername(), resultUserModel); } return resultUserModels; @@ -415,7 +427,7 @@ Map addUserToGroups( OperationFailedException e) { throw new InternalServerErrorException(e); } catch (MembershipAlreadyExistsException e) { - // ignore + resultGroupModels.add(resultGroupModel); } } } diff --git a/crowd/src/test/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceTest.java b/crowd/src/test/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceTest.java index ca1dd103..21e5f020 100644 --- a/crowd/src/test/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceTest.java +++ b/crowd/src/test/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceTest.java @@ -100,15 +100,21 @@ public void testGetUserNotFoundAnyDirectory() throws Exception { }); } + @Test + public void testSetUserNoUsername() { + assertThrows(BadRequestException.class, () -> + usersService.setUser(1L, UserModel.builder().build())); + } + @Test public void testSetUserAddNew() { final User user = getTestUser(); final UserModel userModel = UserModelUtil.toUserModel(user); final UsersServiceImpl spy = spy(usersService); - doReturn(userModel).when(spy).addUser(anyLong(), any(UserModel.class)); + doReturn(userModel).when(spy).addUser(anyLong(), anyString(), any(UserModel.class)); spy.setUser(user.getDirectoryId(), userModel); - verify(spy).addUser(anyLong(), any(UserModel.class)); + verify(spy).addUser(anyLong(), anyString(), any(UserModel.class)); } @Test @@ -125,6 +131,17 @@ public void testSetUserUpdateExisting() throws CrowdException { verify(spy).updateUser(anyLong(), anyString(), any()); } + @Test + public void testSetUserWithMapKeyUsernameWhenModelUsernameIsNull() { + final User user = getTestUser(); + final UserModel userModel = UserModel.builder().build(); // no username set + final UsersServiceImpl spy = spy(usersService); + doReturn(userModel).when(spy).addUser(anyLong(), anyString(), any(UserModel.class)); + + spy.setUser(user.getDirectoryId(), user.getName(), userModel); + verify(spy).addUser(anyLong(), eq(user.getName()), any(UserModel.class)); + } + @Test public void testSetUsers() { final User user = getTestUser(); @@ -134,10 +151,22 @@ public void testSetUsers() { final Map 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()); + doAnswer(invocation -> invocation.getArguments()[2]).when(spy).setUser(anyLong(), anyString(), any()); + + spy.setUsers(user.getDirectoryId(), userModels); + verify(spy, times(userModels.size())).setUser(anyLong(), anyString(), any()); + } + + @Test + public void testSetUsersWithNullUsernameInModelUsesMapKey() { + final User user = getTestUser(); + final UserModel userModel = UserModel.builder().build(); // no username set + final Map userModels = Collections.singletonMap(user.getName(), userModel); + final UsersServiceImpl spy = spy(usersService); + doAnswer(invocation -> UserModelUtil.toUserModel(user)).when(spy).setUser(anyLong(), anyString(), any()); spy.setUsers(user.getDirectoryId(), userModels); - verify(spy, times(userModels.size())).setUser(anyLong(), any()); + verify(spy).setUser(anyLong(), eq(user.getName()), eq(userModel)); } @Test @@ -192,6 +221,24 @@ public void testAddUserWithGroups() throws CrowdException, DirectoryPermissionEx verify(groupsService, times(groupModels.size())).setGroup(anyLong(), anyString(), any()); } + @Test + public void testAddUserWithGroupsAlreadyMember() 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()); + doThrow(new MembershipAlreadyExistsException(1L, "test", GroupModel.EXAMPLE_1.getName())) + .when(directoryManager).addUserToGroup(anyLong(), anyString(), anyString()); + + final UserModel userModel = UserModelUtil.toUserModel(getTestUser()); + final Map groupModels = Stream.of(GroupModel.EXAMPLE_1).collect(Collectors.toMap(GroupModel::getName, Function.identity())); + userModel.setPassword("12345"); + userModel.setGroups(groupModels); + + final UserModel result = usersService.addUser(1L, userModel); + assertNotNull(result.getGroups()); + assertEquals(groupModels.size(), result.getGroups().size()); + } + @Test public void testAddUserAlreadyExists() throws CrowdException { final User user = getTestUser();