diff --git a/docs/manual/docs/administrator-guide/managing-users-and-groups/index.md b/docs/manual/docs/administrator-guide/managing-users-and-groups/index.md index c61c93f35ddc..2dfede5b515d 100644 --- a/docs/manual/docs/administrator-guide/managing-users-and-groups/index.md +++ b/docs/manual/docs/administrator-guide/managing-users-and-groups/index.md @@ -55,10 +55,14 @@ Rights associated with the roles are illustrated in detail in the list below: 2. **User Administrator Profile** - The user administrator is the administrator of his/her own group(s) with the following privileges: + The user administrator manages the users of their own groups. Their own groups are the groups where they have the User Administrator role, which can be fewer than the groups they belong to. - - Full rights on creating new users within their own groups. - - Rights to change users profiles within their own groups. + Within their own groups they can: + + - Create a user, with any profile up to User Administrator. + - Change a user's details, and enable or disable the account. + - Add or remove a user's membership of their own groups. + - Delete a user. 3. **Content Reviewer Profile** diff --git a/services/src/main/java/org/fao/geonet/api/users/UsersApi.java b/services/src/main/java/org/fao/geonet/api/users/UsersApi.java index 35dfd4d24360..706b08266ef3 100644 --- a/services/src/main/java/org/fao/geonet/api/users/UsersApi.java +++ b/services/src/main/java/org/fao/geonet/api/users/UsersApi.java @@ -79,7 +79,6 @@ import static org.fao.geonet.kernel.setting.Settings.SYSTEM_SECURITY_PASSWORD_ALLOWADMINRESET; import static org.fao.geonet.kernel.setting.Settings.SYSTEM_USERS_IDENTICON; -import static org.fao.geonet.repository.specification.UserGroupSpecs.hasProfile; import static org.fao.geonet.repository.specification.UserGroupSpecs.hasUserId; import static org.fao.geonet.repository.specification.UserGroupSpecs.hasUserIdAndProfile; import static org.springframework.data.jpa.domain.Specification.where; @@ -315,9 +314,15 @@ public ResponseEntity deleteUser( if (myProfile == Profile.UserAdmin) { + // A useradmin never administers an administrator, whatever groups they share + Optional userToCheck = userRepository.findById(userIdentifier); + if (userToCheck.isPresent() && Profile.Administrator.equals(userToCheck.get().getProfile())) { + throw new IllegalArgumentException( + "You don't have rights to delete this user because the user is not part of your group"); + } + final int iMyUserId = Integer.parseInt(myUserId); - final List groupIdsSessionUser = userGroupRepository - .findGroupIds(where(hasUserId(iMyUserId))); + final List groupIdsSessionUser = getGroupIdsWhereUserIsUserAdmin(iMyUserId); final List groupIdsUserToDelete = userGroupRepository .findGroupIds(where(hasUserId(userIdentifier))); @@ -446,10 +451,6 @@ public ResponseEntity createUser( Profile profile = Profile.findProfileIgnoreCase(userDto.getProfile()); - if (Profile.Administrator.equals(profile)) { - checkIfAtLeastOneAdminIsEnabled(userDto, userRepository); - } - // TODO: CheckAccessRights if (!myProfile.getProfileAndAllChildren().contains(profile)) { @@ -486,12 +487,12 @@ public ResponseEntity createUser( + userDto.getUsername() + " ignore case already exists"); } - List groups = new LinkedList<>(); + List groups = collectRequestedGroups(userDto); - groups.addAll(processGroups(userDto.getGroupsRegisteredUser(), Profile.RegisteredUser)); - groups.addAll(processGroups(userDto.getGroupsEditor(), Profile.Editor)); - groups.addAll(processGroups(userDto.getGroupsReviewer(), Profile.Reviewer)); - groups.addAll(processGroups(userDto.getGroupsUserAdmin(), Profile.UserAdmin)); + if (!Profile.Administrator.equals(myProfile)) { + checkGroupsAreAdministeredBy(groups, + getGroupIdsWhereUserIsUserAdmin(Integer.parseInt(session.getUserId()))); + } User user = new User(); if (userDto.getPassword() != null) { @@ -542,21 +543,54 @@ public ResponseEntity updateUser( Profile myProfile = session.getProfile(); String myUserId = session.getUserId(); - if (!Profile.Administrator.equals(myProfile) && !Profile.UserAdmin.equals(myProfile) && !myUserId.equals(Integer.toString(userIdentifier))) { + boolean isSelfUpdate = myUserId.equals(Integer.toString(userIdentifier)); + + if (!Profile.Administrator.equals(myProfile) && !Profile.UserAdmin.equals(myProfile) && !isSelfUpdate) { throw new IllegalArgumentException("You don't have rights to do this"); } - if (Profile.Administrator.equals(profile)) { - checkIfAtLeastOneAdminIsEnabled(userDto, userRepository); + // The record to update is identified by the path variable. Reject a body carrying a + // different identifier rather than letting the two disagree. + if (StringUtils.isNotEmpty(userDto.getId()) + && !userDto.getId().equals(Integer.toString(userIdentifier))) { + throw new IllegalArgumentException(String.format( + "The user identifier in the request body (%s) does not match the one in the path (%d)", + userDto.getId(), userIdentifier)); } // TODO: CheckAccessRights - User user = userRepository.findById(userIdentifier).get(); - if (user == null) { - throw new IllegalArgumentException("No user found with id: " - + userDto.getId()); + Optional userOptional = userRepository.findById(userIdentifier); + if (!userOptional.isPresent()) { + throw new IllegalArgumentException(String.format("No user found with id: %d", userIdentifier)); } + User user = userOptional.get(); + + // Check the caller is entitled to act on this record before validating the request + // itself, so that the validation errors say nothing about users they cannot see. + List myUserAdminGroups = Collections.emptyList(); + List userToUpdateGroups = Collections.emptyList(); + + if (!Profile.Administrator.equals(myProfile) && !isSelfUpdate) { + // A useradmin never administers an administrator, whatever groups they share + if (Profile.Administrator.equals(user.getProfile())) { + throw new IllegalArgumentException("You don't have rights to do this"); + } + + myUserAdminGroups = getGroupIdsWhereUserIsUserAdmin(Integer.parseInt(myUserId)); + userToUpdateGroups = userGroupRepository.findAll(hasUserId(userIdentifier)); + + List userToUpdateGroupIds = userToUpdateGroups.stream() + .map(ug -> ug.getId().getGroupId()) + .collect(Collectors.toList()); + + // UserAdmin can't update users that are not in the groups administered + if (myUserAdminGroups.stream().noneMatch(userToUpdateGroupIds::contains)) { + throw new IllegalArgumentException("You don't have rights to do this"); + } + } + + checkIfAtLeastOneAdminIsEnabled(user, profile, userDto.isEnabled()); // Check no duplicated username and if we are adding a duplicate existing name with other case combination List usersWithUsernameIgnoreCase = userRepository.findByUsernameIgnoreCase(userDto.getUsername()); @@ -588,37 +622,24 @@ public ResponseEntity updateUser( List groups = new LinkedList<>(); - groups.addAll(processGroups(userDto.getGroupsRegisteredUser(), Profile.RegisteredUser)); - groups.addAll(processGroups(userDto.getGroupsEditor(), Profile.Editor)); - groups.addAll(processGroups(userDto.getGroupsReviewer(), Profile.Reviewer)); - groups.addAll(processGroups(userDto.getGroupsUserAdmin(), Profile.UserAdmin)); - - //If it is a useradmin updating, - //maybe we don't know all the groups the user is part of - if (!Profile.Administrator.equals(myProfile)) { - List myUserAdminGroups = userGroupRepository.findGroupIds(Specification.where( - hasProfile(myProfile)).and(hasUserId(Integer.parseInt(myUserId)))); - - List usergroups = - userGroupRepository.findAll(Specification.where( - hasUserId(Integer.parseInt(userDto.getId())))); - - List userToUpdateGroupIds = usergroups.stream() - .map(ug -> ug.getId().getGroupId()) - .collect(Collectors.toList()); - - Set groupsInCommon = myUserAdminGroups.stream() - .distinct() - .filter(userToUpdateGroupIds::contains) - .collect(Collectors.toSet()); - - // UserAdmin can't update users that are not in the groups administered - if (groupsInCommon.isEmpty()) { - throw new IllegalArgumentException("You don't have rights to do this"); + if (Profile.Administrator.equals(myProfile)) { + groups.addAll(collectRequestedGroups(userDto)); + } else if (isSelfUpdate) { + // Only an administrator may change group assignments on their own account. For + // everybody else the existing assignments are kept, so that editing one's own + // details through the UI does not alter them. + for (UserGroup ug : userGroupRepository.findAll(hasUserId(userIdentifier))) { + groups.add(new GroupElem(ug.getProfile().name(), ug.getGroup().getId())); } + } else { + // A useradmin only sees part of the catalog, so the update is restricted to the + // groups they administer and the groups they cannot see are left untouched. + List requestedGroups = collectRequestedGroups(userDto); + checkGroupsAreAdministeredBy(requestedGroups, myUserAdminGroups); + groups.addAll(requestedGroups); //keep unknown groups as is - for (UserGroup ug : usergroups) { + for (UserGroup ug : userToUpdateGroups) { if (!myUserAdminGroups.contains(ug.getGroup().getId())) { groups.add(new GroupElem(ug.getProfile().name(), ug.getGroup().getId())); @@ -891,6 +912,34 @@ private void setUserGroups(final User user, List userGroups) } + /** + * Collect the group assignments carried by the request, all profiles together. + */ + private List collectRequestedGroups(UserDto userDto) { + List groups = new LinkedList<>(); + groups.addAll(processGroups(userDto.getGroupsRegisteredUser(), Profile.RegisteredUser)); + groups.addAll(processGroups(userDto.getGroupsEditor(), Profile.Editor)); + groups.addAll(processGroups(userDto.getGroupsReviewer(), Profile.Reviewer)); + groups.addAll(processGroups(userDto.getGroupsUserAdmin(), Profile.UserAdmin)); + return groups; + } + + /** + * Check that the requested assignments only concern groups the caller administers. + * + * @param requestedGroups the assignments carried by the request. + * @param administeredGroupIds the groups the caller is user administrator of. + * @throws IllegalArgumentException thrown on the first group outside that list. + */ + private void checkGroupsAreAdministeredBy(List requestedGroups, List administeredGroupIds) { + for (GroupElem requestedGroup : requestedGroups) { + if (!administeredGroupIds.contains(requestedGroup.getId())) { + throw new IllegalArgumentException( + "You don't have rights to assign a user to the group " + requestedGroup.getId()); + } + } + } + private List processGroups(List groupsToProcessList, Profile profile) { List groups = new LinkedList<>(); for (String g : groupsToProcessList) { @@ -954,24 +1003,26 @@ private void fillUserFromParams(User user, UserDto userDto) { } /** - * Check if removing userDto from the admins there are still at least one user administrator in the system. . + * Check that the update keeps at least one enabled administrator in the system, whether the + * account is being disabled or moved to a lower profile. * - * @param userDto the user to check. - * @param userRepository user repository to retrieve users from. - * @throws IllegalArgumentException thrown if userDto is the last administrator user in the system. + * @param user the user being updated, as currently stored. + * @param newProfile the profile the update would set. + * @param enabled the enabled state the update would set. + * @throws IllegalArgumentException thrown if the user is the last enabled administrator in the system. */ - private void checkIfAtLeastOneAdminIsEnabled(UserDto userDto, UserRepository userRepository) { - // Check at least 1 administrator is enabled - if (StringUtils.isNotEmpty(userDto.getId()) && (!userDto.isEnabled())) { - List adminEnabledList = userRepository.findAll( - Specification.where(UserSpecs.hasProfile(Profile.Administrator)).and(UserSpecs.hasEnabled(true))); - if (adminEnabledList.size() == 1) { - User adminUser = adminEnabledList.get(0); - if (adminUser.getId() == Integer.parseInt(userDto.getId())) { - throw new IllegalArgumentException( - "Trying to disable all administrator users is not allowed"); - } - } + private void checkIfAtLeastOneAdminIsEnabled(User user, Profile newProfile, boolean enabled) { + if (!Profile.Administrator.equals(user.getProfile())) { + return; + } + if (enabled && Profile.Administrator.equals(newProfile)) { + return; + } + List adminEnabledList = userRepository.findAll( + Specification.where(UserSpecs.hasProfile(Profile.Administrator)).and(UserSpecs.hasEnabled(true))); + if (adminEnabledList.size() == 1 && adminEnabledList.get(0).getId() == user.getId()) { + throw new IllegalArgumentException( + "Trying to disable all administrator users is not allowed"); } } } diff --git a/services/src/test/java/org/fao/geonet/api/users/UsersApiTest.java b/services/src/test/java/org/fao/geonet/api/users/UsersApiTest.java index 1410d8671021..5459cbd11574 100644 --- a/services/src/test/java/org/fao/geonet/api/users/UsersApiTest.java +++ b/services/src/test/java/org/fao/geonet/api/users/UsersApiTest.java @@ -27,9 +27,12 @@ import com.google.gson.Gson; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.UUID; +import java.util.stream.Collectors; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang.StringUtils; import org.fao.geonet.api.users.model.PasswordResetDto; @@ -53,6 +56,7 @@ import org.springframework.web.context.WebApplicationContext; import static org.fao.geonet.repository.specification.UserGroupSpecs.hasUserId; +import static org.fao.geonet.repository.specification.UserGroupSpecs.hasUserIdAndProfile; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; @@ -69,6 +73,12 @@ * including creation, deletion, updating, and retrieval of user accounts and their associated data. */ public class UsersApiTest extends AbstractServiceIntegrationTest { + /** + * An identifier the generator cannot reach, so that it stays unused whatever the number + * of users the tests create. + */ + private static final int NON_EXISTING_USER_ID = Integer.MAX_VALUE; + @Autowired private WebApplicationContext wac; @@ -104,7 +114,7 @@ public void getNonExistingUser() throws Exception { this.mockHttpSession = loginAsAdmin(); - this.mockMvc.perform(get("/srv/api/users/222") + this.mockMvc.perform(get("/srv/api/users/" + NON_EXISTING_USER_ID) .session(this.mockHttpSession) .accept(MediaType.parseMediaType("application/json"))) .andExpect(status().is(404)) @@ -216,7 +226,7 @@ public void deleteExistingUser() throws Exception { @Test public void deleteNonExistingUser() throws Exception { - Optional userToDelete = _userRepo.findById(222); + Optional userToDelete = _userRepo.findById(NON_EXISTING_USER_ID); Assert.assertFalse(userToDelete.isPresent()); this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); @@ -224,7 +234,7 @@ public void deleteNonExistingUser() throws Exception { this.mockHttpSession = loginAsAdmin(); // Check 404 is returned - this.mockMvc.perform(delete("/srv/api/users/222") + this.mockMvc.perform(delete("/srv/api/users/" + NON_EXISTING_USER_ID) .session(this.mockHttpSession) .accept(MediaType.parseMediaType("application/json"))) .andExpect(status().is(404)) @@ -279,6 +289,65 @@ public void deleteUserNotAllowedToUserAdmin() throws Exception { .andExpect(content().contentType(API_JSON_EXPECTED_ENCODING)); } + @Test + public void deleteUserInGroupNotAdministeredByTheCaller() throws Exception { + Group testGroup = _groupRepo.findByName("test"); + Assert.assertNotNull(testGroup); + + // The caller administers the sample group, and is only an editor of the test group + final User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + _userGroupRepo.save(new UserGroup().setGroup(testGroup) + .setProfile(Profile.Editor).setUser(userAdmin)); + + // The user to delete belongs to the test group only, so the two share a group + // without the caller administering it + final User userToDelete = _userRepo.findOneByUsername("testuser-reviewer"); + Assert.assertNotNull(userToDelete); + Assert.assertTrue(CollectionUtils.isNotEmpty(CollectionUtils.intersection( + _userGroupRepo.findGroupIds(hasUserId(userAdmin.getId())), + _userGroupRepo.findGroupIds(hasUserId(userToDelete.getId()))))); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(delete("/srv/api/users/" + userToDelete.getId()) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is("You don't have rights to delete this user because the user is not part of your group"))) + .andExpect(content().contentType(API_JSON_EXPECTED_ENCODING)); + + Assert.assertTrue(_userRepo.findById(userToDelete.getId()).isPresent()); + } + + @Test + public void deleteUserInGroupAdministeredByTheCaller() throws Exception { + Group sampleGroup = _groupRepo.findByName("sample"); + Assert.assertNotNull(sampleGroup); + + final User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + Assert.assertTrue(_userGroupRepo.findGroupIds( + hasUserIdAndProfile(userAdmin.getId(), Profile.UserAdmin)).contains(sampleGroup.getId())); + + // Member of the sample group, which the caller administers + final User userToDelete = _userRepo.findOneByUsername("testuser-editor"); + Assert.assertNotNull(userToDelete); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(delete("/srv/api/users/" + userToDelete.getId()) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(204)); + + Assert.assertFalse(_userRepo.findById(userToDelete.getId()).isPresent()); + } + @Test public void createUser() throws Exception { @@ -795,6 +864,400 @@ public void updateUserAlreadyExistingUsernameCase() throws Exception { .andExpect(jsonPath("$.message", is("Another user with username 'testuser-editor' ignore case already exists"))); } + @Test + public void updateUserWithIdentifierNotMatchingThePath() throws Exception { + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + // Not part of any group administered by testuser-useradmin + User userToUpdate = _userRepo.findOneByUsername("testuser-reviewer"); + Assert.assertNotNull(userToUpdate); + + UserDto user = new UserDto(); + // The body claims to be about the caller, the path points at somebody else + user.setId(Integer.toString(userAdmin.getId())); + user.setUsername(userToUpdate.getUsername()); + user.setName(userToUpdate.getName()); + user.setProfile(Profile.UserAdmin.name()); + user.setEmail(new ArrayList(userToUpdate.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(put("/srv/api/users/" + userToUpdate.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is(String.format( + "The user identifier in the request body (%d) does not match the one in the path (%d)", + userAdmin.getId(), userToUpdate.getId())))); + + // The target is left untouched + User userAfter = _userRepo.findOneByUsername("testuser-reviewer"); + Assert.assertEquals(Profile.Reviewer, userAfter.getProfile()); + } + + @Test + public void updateUserWithGroupNotAdministeredByTheCaller() throws Exception { + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + // Part of the sample group, which testuser-useradmin administers + User userToUpdate = _userRepo.findOneByUsername("testuser-editor"); + Assert.assertNotNull(userToUpdate); + + Group testGroup = _groupRepo.findByName("test"); + Assert.assertNotNull(testGroup); + + UserDto user = new UserDto(); + user.setId(Integer.toString(userToUpdate.getId())); + user.setUsername(userToUpdate.getUsername()); + user.setName(userToUpdate.getName()); + user.setProfile(Profile.UserAdmin.name()); + // Group administered by somebody else + user.setGroupsUserAdmin(Collections.singletonList(Integer.toString(testGroup.getId()))); + user.setEmail(new ArrayList(userToUpdate.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(put("/srv/api/users/" + userToUpdate.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is( + "You don't have rights to assign a user to the group " + testGroup.getId()))); + + List groupIds = _userGroupRepo.findGroupIds(hasUserId(userToUpdate.getId())); + Assert.assertFalse(groupIds.contains(testGroup.getId())); + } + + @Test + public void updateOwnAccountKeepsGroupAssignments() throws Exception { + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + Group testGroup = _groupRepo.findByName("test"); + Assert.assertNotNull(testGroup); + + Set groupIdsBefore = new HashSet<>(_userGroupRepo.findGroupIds(hasUserId(userAdmin.getId()))); + Assert.assertFalse(groupIdsBefore.contains(testGroup.getId())); + + UserDto user = new UserDto(); + user.setId(Integer.toString(userAdmin.getId())); + user.setUsername(userAdmin.getUsername()); + user.setName("a new name"); + user.setProfile(Profile.UserAdmin.name()); + // Asking for a group the caller does not administer + user.setGroupsUserAdmin(Collections.singletonList(Integer.toString(testGroup.getId()))); + user.setEmail(new ArrayList(userAdmin.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + // The update itself is accepted + this.mockMvc.perform(put("/srv/api/users/" + userAdmin.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(204)); + + Assert.assertEquals("a new name", _userRepo.findOneByUsername("testuser-useradmin").getName()); + + // but the group assignments are the ones already stored + Set groupIdsAfter = new HashSet<>(_userGroupRepo.findGroupIds(hasUserId(userAdmin.getId()))); + Assert.assertFalse(groupIdsAfter.contains(testGroup.getId())); + Assert.assertEquals(groupIdsBefore, groupIdsAfter); + } + + @Test + public void updateLastEnabledAdministratorToLowerProfile() throws Exception { + User administrator = null; + int enabledAdministrators = 0; + for (User u : _userRepo.findAllByProfile(Profile.Administrator)) { + if (u.isEnabled()) { + enabledAdministrators++; + administrator = u; + } + } + Assert.assertEquals("the test data is expected to hold a single enabled administrator", + 1, enabledAdministrators); + + UserDto user = new UserDto(); + user.setId(Integer.toString(administrator.getId())); + user.setUsername(administrator.getUsername()); + user.setName(administrator.getName()); + // Demoting the last administrator leaves the catalog without one + user.setProfile(Profile.UserAdmin.name()); + user.setEmail(new ArrayList(administrator.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAsAdmin(); + + this.mockMvc.perform(put("/srv/api/users/" + administrator.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is("Trying to disable all administrator users is not allowed"))); + + Assert.assertEquals(Profile.Administrator, + _userRepo.findById(administrator.getId()).get().getProfile()); + } + + @Test + public void updateAdministratorByUserAdminNotAllowed() throws Exception { + // An administrator that also carries group memberships, as happens when the account + // was promoted from a lower profile + User administrator = createAdministratorInGroup("sample"); + + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + UserDto user = new UserDto(); + user.setId(Integer.toString(administrator.getId())); + user.setUsername(administrator.getUsername()); + user.setName("demoted"); + user.setProfile(Profile.UserAdmin.name()); + user.setEmail(new ArrayList(administrator.getEmailAddresses())); + user.setEnabled(false); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(put("/srv/api/users/" + administrator.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is("You don't have rights to do this"))); + + User after = _userRepo.findById(administrator.getId()).get(); + Assert.assertEquals(Profile.Administrator, after.getProfile()); + Assert.assertTrue(after.isEnabled()); + } + + @Test + public void deleteAdministratorByUserAdminNotAllowed() throws Exception { + User administrator = createAdministratorInGroup("sample"); + + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(delete("/srv/api/users/" + administrator.getId()) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is( + "You don't have rights to delete this user because the user is not part of your group"))); + + Assert.assertTrue(_userRepo.findById(administrator.getId()).isPresent()); + } + + @Test + public void updateUserChecksRightsBeforeValidatingTheRequest() throws Exception { + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + // The built-in administrator, not part of any group administered by the caller + User administrator = _userRepo.findAllByProfile(Profile.Administrator).get(0); + + UserDto user = new UserDto(); + user.setUsername(administrator.getUsername()); + user.setName("x"); + user.setProfile(Profile.UserAdmin.name()); + user.setEmail(new ArrayList(administrator.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + // The answer says nothing about the record, in particular not that it is the last + // enabled administrator + this.mockMvc.perform(put("/srv/api/users/" + administrator.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is("You don't have rights to do this"))); + } + + @Test + public void updateUserWithGroupAdministeredByTheCaller() throws Exception { + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + // Part of the sample group, which testuser-useradmin administers + User userToUpdate = _userRepo.findOneByUsername("testuser-editor"); + Assert.assertNotNull(userToUpdate); + + Group sampleGroup = _groupRepo.findByName("sample"); + Assert.assertNotNull(sampleGroup); + + UserDto user = new UserDto(); + user.setId(Integer.toString(userToUpdate.getId())); + user.setUsername(userToUpdate.getUsername()); + user.setName(userToUpdate.getName()); + user.setProfile(Profile.Reviewer.name()); + user.setGroupsReviewer(Collections.singletonList(Integer.toString(sampleGroup.getId()))); + user.setEmail(new ArrayList(userToUpdate.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(put("/srv/api/users/" + userToUpdate.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(204)); + + Assert.assertEquals(Profile.Reviewer, + _userRepo.findById(userToUpdate.getId()).get().getProfile()); + Assert.assertTrue(_userGroupRepo.findAll(hasUserId(userToUpdate.getId())).stream() + .anyMatch(ug -> ug.getGroup().getId() == sampleGroup.getId() + && Profile.Reviewer.equals(ug.getProfile()))); + } + + @Test + public void updateUserGroupsByAdministrator() throws Exception { + User userToUpdate = _userRepo.findOneByUsername("testuser-editor"); + Assert.assertNotNull(userToUpdate); + + Group testGroup = _groupRepo.findByName("test"); + Assert.assertNotNull(testGroup); + + UserDto user = new UserDto(); + user.setId(Integer.toString(userToUpdate.getId())); + user.setUsername(userToUpdate.getUsername()); + user.setName(userToUpdate.getName()); + user.setProfile(Profile.Editor.name()); + user.setGroupsEditor(Collections.singletonList(Integer.toString(testGroup.getId()))); + user.setEmail(new ArrayList(userToUpdate.getEmailAddresses())); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAsAdmin(); + + this.mockMvc.perform(put("/srv/api/users/" + userToUpdate.getId()) + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(204)); + + // An administrator sees the whole catalog, so the assignments sent are the ones stored + List groupIds = _userGroupRepo.findGroupIds(hasUserId(userToUpdate.getId())); + Assert.assertEquals(Collections.singletonList(testGroup.getId()), + groupIds.stream().distinct().collect(Collectors.toList())); + } + + /** + * Create an enabled administrator which is also a member of the given group, as happens + * when an account is promoted from a lower profile. + */ + private User createAdministratorInGroup(String groupName) { + Group group = _groupRepo.findByName(groupName); + Assert.assertNotNull(group); + + User administrator = new User(); + administrator.setUsername("testuser-admin-in-group"); + administrator.setProfile(Profile.Administrator); + administrator.setEnabled(true); + administrator.getEmailAddresses().add("admin-in-group@mail.com"); + _userRepo.save(administrator); + + _userGroupRepo.save(new UserGroup().setGroup(group) + .setProfile(Profile.Editor).setUser(administrator)); + + return administrator; + } + + @Test + public void createUserWithGroupNotAdministeredByTheCaller() throws Exception { + User userAdmin = _userRepo.findOneByUsername("testuser-useradmin"); + Assert.assertNotNull(userAdmin); + + Group testGroup = _groupRepo.findByName("test"); + Assert.assertNotNull(testGroup); + + UserDto user = new UserDto(); + user.setUsername("newuser-othergroup"); + user.setName("new"); + user.setProfile(Profile.UserAdmin.name()); + user.setGroupsUserAdmin(Collections.singletonList(Integer.toString(testGroup.getId()))); + user.setEmail(Collections.singletonList("mail@test.com")); + user.setPassword("Password7$"); + user.setEnabled(true); + + Gson gson = new Gson(); + String json = gson.toJson(user); + + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); + + this.mockHttpSession = loginAs(userAdmin); + + this.mockMvc.perform(put("/srv/api/users") + .content(json) + .contentType(API_JSON_EXPECTED_ENCODING) + .session(this.mockHttpSession) + .accept(MediaType.parseMediaType("application/json"))) + .andExpect(status().is(400)) + .andExpect(jsonPath("$.message", is( + "You don't have rights to assign a user to the group " + testGroup.getId()))); + + Assert.assertNull(_userRepo.findOneByUsername("newuser-othergroup")); + } + /** * Create sample data for the tests. */ diff --git a/web-ui/src/main/resources/catalog/js/admin/UserGroupController.js b/web-ui/src/main/resources/catalog/js/admin/UserGroupController.js index 58fa0edd977e..e4c6679e5c8a 100644 --- a/web-ui/src/main/resources/catalog/js/admin/UserGroupController.js +++ b/web-ui/src/main/resources/catalog/js/admin/UserGroupController.js @@ -289,6 +289,19 @@ $scope.gnUserEdit.$setPristine(); }; + /** + * A user administrator does not administer administrators, so an + * administrator account is read only for them. The API enforces this, + * the form only avoids offering actions that would be rejected. + */ + $scope.isUserEditable = function () { + return ( + !$scope.userSelected || + $scope.userSelected.profile !== "Administrator" || + $scope.user.isAdministratorOrMore() + ); + }; + /** * Select a user and retrieve its groups and * metadata records. diff --git a/web-ui/src/main/resources/catalog/templates/admin/usergroup/users.html b/web-ui/src/main/resources/catalog/templates/admin/usergroup/users.html index 3ca0d712bf85..1da22d0aa682 100644 --- a/web-ui/src/main/resources/catalog/templates/admin/usergroup/users.html +++ b/web-ui/src/main/resources/catalog/templates/admin/usergroup/users.html @@ -92,7 +92,7 @@