Implement Pulse Survey report permissions #2429
Conversation
timyates
changed the title
|
I removed the service test, the coverage is almost exactly the same, and the mocking was getting excessive. The controller test does a better job of exercising the paths with real services and actual data (IMO) |
| ORDER BY level""", | ||
| nativeQuery = true | ||
| ) | ||
| List<MemberProfile> findSubordinatesForId(UUID id); |
| List<MemberProfile> subordinatesForId = memberProfileRepository.findSubordinatesForId(id); | ||
| if (!currentUserServices.isAdmin()) { | ||
| for (MemberProfile memberProfile : subordinatesForId) { | ||
| memberProfile.clearBirthYear(); |
There was a problem hiding this comment.
Why do we want to clear the birth year of the subordinates?
There was a problem hiding this comment.
I was following the pattern for supervisors in the getSupervisorsForId method above 🤔
There was a problem hiding this comment.
We don't want to expose folks ages to everyone, so we clear them for those who don't need to know.
| @Cacheable | ||
| public List<MemberProfile> getSubordinatesForId(UUID id) { | ||
| List<MemberProfile> subordinatesForId = memberProfileRepository.findSubordinatesForId(id); | ||
| if (!currentUserServices.isAdmin()) { |
There was a problem hiding this comment.
This bit makes me sad, but I think it's the right choice for this PR. I'll create a spike to identify any usages of the roles for security (we should be using permissions) so that we can break out stories to address those.
| CAN_VIEW_SETTINGS("View settings", "Settings"), | ||
| CAN_VIEW_ALL_PULSE_RESPONSES("View all pulse responses", "Pulse"), | ||
| CAN_CREATE_ALL_PULSE_RESPONSES("Create pulse responses for anyone", "Pulse"), | ||
| CAN_UPDATE_ALL_PULSE_RESPONSES("Update pulse responses for anyone", "Pulse"); |
There was a problem hiding this comment.
I don't think there's a use case for these bottom two. This is probably a failure of the AC, so, my bad.
We want to control whether a role can create/update pulse responses, but in either case, they should only be able to enter a response for themselves, not edit or enter them for others.
There was a problem hiding this comment.
Fingers crossed this fixes it 🤞 7d08d3b
I removed the CREATE and UPDATE permissions, but still check the memberId so only the member themselves (or a "logical company superior") can create, edit or update a report for a given memberId
| List<MemberProfile> subordinatesForId = memberProfileRepository.findSubordinatesForId(id); | ||
| if (!currentUserServices.isAdmin()) { | ||
| for (MemberProfile memberProfile : subordinatesForId) { | ||
| memberProfile.clearBirthYear(); |
There was a problem hiding this comment.
We don't want to expose folks ages to everyone, so we clear them for those who don't need to know.
| PulseResponse pulseResponseRet = null; | ||
| if(pulseResponse!=null){ | ||
| if (pulseResponse != null) { | ||
| final UUID memberId = pulseResponse.getTeamMemberId(); |
There was a problem hiding this comment.
Given my comment about only being able to create/update your own responses. It might be worthwhile to turn the teammemberid field into one that is annotated with and populated via @CreatedBy. Thoughts?
There was a problem hiding this comment.
I had a look, but I am unsure as to how to get the tests working with @CreatedBy 🤔
Should I change
} else if (!currentUserId.equals(memberId) && !isSubordinateTo(memberId, currentUserId)) {
in save and update to
} else if (!currentUserId.equals(memberId)) {
To more closely match your comment? We can then raise a card to investigate @CreatedBy and I can pick it up when I return on the 3rd?
There was a problem hiding this comment.
That works, yes. Sorry for the slow reply. I missed yours completely.
See #2350
Added 1 new permission:
create and update check
teamMemberIdin the supervisory tree of the current user?And view checks the permission exists, and if not then performs the 2 above checks