[Backport 4.2.x] Users API / Align the checks on what a user administrator may change (#9449) - #9450
Open
juanluisrp wants to merge 10 commits into
Open
[Backport 4.2.x] Users API / Align the checks on what a user administrator may change (#9449)#9450juanluisrp wants to merge 10 commits into
juanluisrp wants to merge 10 commits into
Conversation
updateUser mutated the record named by the path variable but read userDto.getId() from the request body in places. Reject a body whose identifier differs from the path, and use the path variable for the group lookup. (cherry picked from commit 08ef65c)
createUser and updateUser applied the group lists from the request body without checking that the caller administers those groups. Check each requested group in both endpoints, and split the update path into administrator, self update and useradmin cases. (cherry picked from commit 72dae43)
The check read the identifier and the enabled flag from the request body, and ran only when the submitted profile was Administrator, so it missed demotions. Base it on the stored record instead. Drop the call in createUser, where it could never fire. (cherry picked from commit aa04623)
getNonExistingUser and deleteNonExistingUser hardcode 222 as an identifier that no user holds. The generator is not reset between tests in a class, so the assumption breaks as soon as enough users are created. Use Integer.MAX_VALUE instead. (cherry picked from commit a907797)
…quest updateUser validated the submitted values, and ran the last administrator check, before working out whether the caller may act on the record at all. A useradmin therefore got different answers for a record they administer, a record they do not, and an identifier nobody holds. Move the check on the groups in common ahead of that validation, and refuse records whose stored profile is Administrator: a useradmin administers the members of their groups, not the administrators that happen to share one. getUsers already leaves administrators out of the list it returns them. Keep the group lookups from that check for the group merge further down rather than repeating them. (cherry picked from commit 7388863)
deleteUser let a useradmin remove any user sharing one of their groups, including an administrator. Refuse those records here too, reusing the message of the group check so the answer does not depend on the profile of the record. (cherry picked from commit 82dd511)
The user form let a useradmin save or delete a record whose profile is Administrator. Such a record is not reachable from the list they get, but nothing in the form itself said so. Add isUserEditable and use it to disable the save button and hide the delete button in that case. (cherry picked from commit 9553d41)
The four processGroups calls appeared three times and the check on the groups the caller administers twice. Move both into helpers. Drop the parameters checkIfAtLeastOneAdminIsEnabled can read from the record or the field, and compare the group assignments of updateOwnAccountKeepsGroupAssignments as sets, findGroupIds having no defined order. (cherry picked from commit 0d179c3)
deleteUser compared every group the caller belongs to with the groups of the record, so a useradmin could remove a user sharing a group they are only an editor of. updateUser looks at the groups where the caller holds the UserAdmin profile; use the same set here. (cherry picked from commit fc891c2)
The profile description repeated "their own groups" without defining it, which reads as any group the user belongs to. Say that it means the groups where they have the User Administrator role, and list what they can do there. (cherry picked from commit 6f7ea57)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #9449 to
4.2.x.