Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,36 @@ WHERE mp.id IN (:ids)""",
"WHERE s.id <> :id " +
"ORDER BY level", nativeQuery = true)
List<MemberProfile> findSupervisorsForId(UUID id);

@Query(
value = """
WITH RECURSIVE subordinate AS (
SELECT id, firstname, middlename, lastname, suffix, title, pdlid, location, workemail, employeeid, startdate, biotext, supervisorid, terminationdate, birthdate, voluntary, excluded, last_seen, 0 as level
FROM member_profile
WHERE id = :id and terminationdate is NULL
UNION ALL
SELECT e.id, e.firstname, e.middlename, e.lastname, e.suffix, e.title, e.pdlid, e.location, e.workemail, e.employeeid, e.startdate, e.biotext, e.supervisorid, e.terminationdate, e.birthdate, e.voluntary, e.excluded, e.last_seen, level + 1
FROM member_profile AS e JOIN subordinate AS s ON s.id = e.supervisorid
WHERE e.terminationdate is NULL
)
SELECT DISTINCT
s.id AS id,
PGP_SYM_DECRYPT(cast(s.firstname as bytea),'${aes.key}') as firstname,
PGP_SYM_DECRYPT(cast(s.middlename as bytea),'${aes.key}') as middlename,
PGP_SYM_DECRYPT(cast(s.lastname as bytea),'${aes.key}') as lastname,
PGP_SYM_DECRYPT(cast(s.suffix as bytea),'${aes.key}') as suffix,
PGP_SYM_DECRYPT(cast(s.title as bytea),'${aes.key}') as title,
s.pdlid,
PGP_SYM_DECRYPT(cast(s.location as bytea), '${aes.key}') as location,
PGP_SYM_DECRYPT(cast(s.workemail as bytea), '${aes.key}') as workemail,
s.employeeid, s.startdate,
PGP_SYM_DECRYPT(cast(s.biotext as bytea), '${aes.key}') as biotext,
s.supervisorid, s.terminationdate, s.birthdate, s.voluntary, s.excluded, s.last_seen,
s.level
FROM subordinate s
WHERE s.id <> :id
ORDER BY level""",
nativeQuery = true
)
List<MemberProfile> findSubordinatesForId(UUID id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,7 @@ Set<MemberProfile> findByValues(String firstName, String lastName, String title,

List<MemberProfile> getSupervisorsForId(UUID id);

List<MemberProfile> getSubordinatesForId(UUID id);

MemberProfile updateProfile(MemberProfile memberProfile);
}
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,18 @@ public List<MemberProfile> getSupervisorsForId(UUID id) {
return supervisorsForId;
}

@Override
@Cacheable
public List<MemberProfile> getSubordinatesForId(UUID id) {
List<MemberProfile> subordinatesForId = memberProfileRepository.findSubordinatesForId(id);
if (!currentUserServices.isAdmin()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (MemberProfile memberProfile : subordinatesForId) {
memberProfile.clearBirthYear();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to clear the birth year of the subordinates?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the pattern for supervisors in the getSupervisorsForId method above 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to expose folks ages to everyone, so we clear them for those who don't need to know.

}
}
return subordinatesForId;
}

@Override
public MemberProfile updateProfile(MemberProfile memberProfile) {
return memberProfileRepository.update(memberProfile);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ public enum Permission {
CAN_CLOSE_REVIEW_PERIOD("Close review period", "Review Period"),
CAN_DELETE_REVIEW_PERIOD("Delete review period", "Review Period"),
CAN_ADMINISTER_SETTINGS("Add or edit settings", "Settings"),
CAN_VIEW_SETTINGS("View settings", "Settings");
CAN_VIEW_SETTINGS("View settings", "Settings"),
CAN_VIEW_ALL_PULSE_RESPONSES("View all pulse responses", "Pulse");

private final String description;
private final String category;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,81 +2,123 @@

import com.objectcomputing.checkins.exceptions.BadArgException;
import com.objectcomputing.checkins.services.memberprofile.MemberProfileRepository;
import com.objectcomputing.checkins.services.memberprofile.MemberProfileServices;
import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices;
import com.objectcomputing.checkins.services.permissions.Permission;
import com.objectcomputing.checkins.services.role.role_permissions.RolePermissionServices;
import jakarta.inject.Singleton;
import jakarta.validation.constraints.NotNull;

import java.time.LocalDate;
import java.util.HashSet;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;

@Singleton
public class PulseResponseServicesImpl implements PulseResponseService {

private final PulseResponseRepository pulseResponseRepo;
private final MemberProfileServices memberProfileServices;
private final MemberProfileRepository memberRepo;
private final CurrentUserServices currentUserServices;
private final RolePermissionServices rolePermissionServices;

public PulseResponseServicesImpl(PulseResponseRepository pulseResponseRepo,
MemberProfileRepository memberRepo) {
public PulseResponseServicesImpl(
PulseResponseRepository pulseResponseRepo,
MemberProfileServices memberProfileServices,
MemberProfileRepository memberRepo,
CurrentUserServices currentUserServices,
RolePermissionServices rolePermissionServices
) {
this.pulseResponseRepo = pulseResponseRepo;
this.memberProfileServices = memberProfileServices;
this.memberRepo = memberRepo;
this.currentUserServices = currentUserServices;
this.rolePermissionServices = rolePermissionServices;
}

@Override
public PulseResponse save(PulseResponse pulseResponse) {
UUID currentUserId = currentUserServices.getCurrentUser().getId();
PulseResponse pulseResponseRet = null;
if(pulseResponse!=null){
if (pulseResponse != null) {
final UUID memberId = pulseResponse.getTeamMemberId();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@timyates timyates May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works, yes. Sorry for the slow reply. I missed yours completely.

LocalDate pulseSubDate = pulseResponse.getSubmissionDate();
if(pulseResponse.getId()!=null){
throw new BadArgException(String.format("Found unexpected id for pulseresponse %s", pulseResponse.getId()));
} else if(!memberRepo.findById(memberId).isPresent()){
throw new BadArgException(String.format("Member %s doesn't exists", memberId));
} else if(pulseSubDate.isBefore(LocalDate.EPOCH) || pulseSubDate.isAfter(LocalDate.MAX)) {
throw new BadArgException(String.format("Invalid date for pulseresponse submission date %s",memberId));
}
pulseResponseRet = pulseResponseRepo.save(pulseResponse);
if (pulseResponse.getId() != null) {
throw new BadArgException(String.format("Found unexpected id for pulseresponse %s", pulseResponse.getId()));
} else if (memberRepo.findById(memberId).isEmpty()) {
throw new BadArgException(String.format("Member %s doesn't exists", memberId));
} else if (pulseSubDate.isBefore(LocalDate.EPOCH) || pulseSubDate.isAfter(LocalDate.MAX)) {
throw new BadArgException(String.format("Invalid date for pulseresponse submission date %s", memberId));
} else if (!currentUserId.equals(memberId) && !isSubordinateTo(memberId, currentUserId)) {
throw new BadArgException(String.format("User %s does not have permission to create pulse response for user %s", currentUserId, memberId));
}
pulseResponseRet = pulseResponseRepo.save(pulseResponse);
}
return pulseResponseRet ;
return pulseResponseRet;
}


@Override
public PulseResponse read(@NotNull UUID id) {
return pulseResponseRepo.findById(id).orElse(null);
UUID currentUserId = currentUserServices.getCurrentUser().getId();
boolean hasPermission = rolePermissionServices.findUserPermissions(currentUserId).contains(Permission.CAN_VIEW_ALL_PULSE_RESPONSES);

return pulseResponseRepo.findById(id)
.filter(pulse -> hasPermission || canViewDueToReportingHierarchy(pulse, currentUserId))
.orElse(null);
}

@Override
public PulseResponse update(PulseResponse pulseResponse) {
UUID currentUserId = currentUserServices.getCurrentUser().getId();
PulseResponse pulseResponseRet = null;
if(pulseResponse!=null){
final UUID id = pulseResponse.getId();
final UUID memberId = pulseResponse.getTeamMemberId();
LocalDate pulseSubDate = pulseResponse.getSubmissionDate();
if(id==null||!pulseResponseRepo.findById(id).isPresent()){
throw new BadArgException(String.format("Unable to find pulseresponse record with id %s", pulseResponse.getId()));
}else if(!memberRepo.findById(memberId).isPresent()){
throw new BadArgException(String.format("Member %s doesn't exist", memberId));
} else if(memberId==null) {
throw new BadArgException(String.format("Invalid pulseresponse %s", pulseResponse));
} else if(pulseSubDate.isBefore(LocalDate.EPOCH) || pulseSubDate.isAfter(LocalDate.MAX)) {
throw new BadArgException(String.format("Invalid date for pulseresponse submission date %s",memberId));
if (pulseResponse != null) {
final UUID id = pulseResponse.getId();
final UUID memberId = pulseResponse.getTeamMemberId();
LocalDate pulseSubDate = pulseResponse.getSubmissionDate();
if (id == null || pulseResponseRepo.findById(id).isEmpty()) {
throw new BadArgException(String.format("Unable to find pulseresponse record with id %s", pulseResponse.getId()));
} else if (memberRepo.findById(memberId).isEmpty()) {
throw new BadArgException(String.format("Member %s doesn't exist", memberId));
} else if (memberId == null) {
throw new BadArgException(String.format("Invalid pulseresponse %s", pulseResponse));
} else if (pulseSubDate.isBefore(LocalDate.EPOCH) || pulseSubDate.isAfter(LocalDate.MAX)) {
throw new BadArgException(String.format("Invalid date for pulseresponse submission date %s", memberId));
} else if (!currentUserId.equals(memberId) && !isSubordinateTo(memberId, currentUserId)) {
throw new BadArgException(String.format("User %s does not have permission to update pulse response for user %s", currentUserId, memberId));
}
pulseResponseRet = pulseResponseRepo.update(pulseResponse);
}

pulseResponseRet = pulseResponseRepo.update(pulseResponse);
}
return pulseResponseRet;
}

@Override
public Set<PulseResponse> findByFields(UUID teamMemberId, LocalDate dateFrom, LocalDate dateTo) {
Set<PulseResponse> pulseResponse = new HashSet<>();
pulseResponseRepo.findAll().forEach(pulseResponse::add);
if(teamMemberId!=null){
UUID currentUserId = currentUserServices.getCurrentUser().getId();
boolean hasPermission = rolePermissionServices.findUserPermissions(currentUserId).contains(Permission.CAN_VIEW_ALL_PULSE_RESPONSES);

Set<PulseResponse> pulseResponse = pulseResponseRepo.findAll()
.stream()
.filter(pulse -> hasPermission || canViewDueToReportingHierarchy(pulse, currentUserId))
.collect(Collectors.toSet());

if (teamMemberId != null) {
pulseResponse.retainAll(pulseResponseRepo.findByTeamMemberId(teamMemberId));
} else if(dateFrom!=null && dateTo!=null) {
} else if (dateFrom != null && dateTo != null) {
pulseResponse.retainAll(pulseResponseRepo.findBySubmissionDateBetween(dateFrom, dateTo));
}
return pulseResponse;
}
}

// The current user can view the pulse response if they are the team member who submitted the pulse response
// or if they are the supervisor of the team member who submitted the pulse response
private boolean canViewDueToReportingHierarchy(PulseResponse pulse, UUID currentUserId) {
return pulse.getTeamMemberId().equals(currentUserId) ||
isSubordinateTo(pulse.getTeamMemberId(), currentUserId);
}

private boolean isSubordinateTo(UUID reportMember, UUID currentUserId) {
return memberProfileServices.getSubordinatesForId(currentUserId)
.stream().anyMatch(member -> member.getId().equals(reportMember));
}
}
5 changes: 5 additions & 0 deletions server/src/main/resources/db/dev/R__Load_testing_data.sql
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,11 @@ insert into role_permissions
values
('e8a4fff8-e984-4e59-be84-a713c9fa8d23', 'CAN_DELETE_REVIEW_PERIOD');

insert into role_permissions
(roleid, permission)
values
('e8a4fff8-e984-4e59-be84-a713c9fa8d23', 'CAN_VIEW_ALL_PULSE_RESPONSES');

-- PDL Permissions
insert into role_permissions
(roleid, permission)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.objectcomputing.checkins.services.memberprofile.MemberProfile;

import java.time.LocalDate;
import java.util.Map;

public interface MemberProfileFixture extends RepositoryFixture {

Expand Down Expand Up @@ -176,4 +177,83 @@ default MemberProfile createATerminatedNewHireProfile() {
null, LocalDate.now().minusMonths(1), null, true, null, LocalDate.now().minusMonths(1)));
}

default MemberProfile memberWithoutBoss(String name) {
return memberWithSupervisor(name, null, null);
}

default MemberProfile memberWithSupervisor(String name, MemberProfile supervisor) {
return memberWithSupervisor(name, supervisor, null);
}

default MemberProfile memberWithSupervisor(String name, MemberProfile supervisor, LocalDate terminationDate) {
return getMemberProfileRepository().save(
new MemberProfile(
name,
null,
name,
null,
null,
null,
null,
name + "@work.com",
null,
null,
null,
supervisor != null ? supervisor.getId() : null,
terminationDate,
null,
null,
null,
LocalDate.now()
)
);
}

// ┌─────────┐
// ┌────────┤ CEO ├────┐
// │ └─────────┘ │
// │ │ ┌──────────┐
// ┌───▼───┐ ┌───▼───┐ │ Wildcard │
// ┌─────┤ Lead1 ├────┐ │ Lead2 │ └──────────┘
// │ └───────┘ │ └───┬───┘
// │ │ │
// ┌────▼──────┐ ┌──────▼────┐ ┌─────▼─────┐
// │ Lead1Sub1 │ │ Lead1Sub2 │ ┌─┤ Lead2Sub1 ├───┐
// └───────────┘ └───────────┘ │ └───────────┘ │
// │ │
// ┌────────▼──────┐ ┌──────▼─────────────┐
// │ Lead2Sub1Sub1 │ │ Lead2Sub1Sub2Fired │
// └───────────────┘ └────────────────────┘
String HIERARCHY_CEO = "ceo";
String HIERARCHY_LEAD1 = "lead1";
String HIERARCHY_LEAD1_SUB1 = "lead1sub1";
String HIERARCHY_LEAD1_SUB2 = "lead1sub2";
String HIERARCHY_LEAD2 = "lead2";
String HIERARCHY_LEAD2_SUB1 = "lead2sub1";
String HIERARCHY_LEAD2_SUB1_SUB1 = "lead2sub1sub1";
String HIERARCHY_LEAD2_SUB1_SUB2_FIRED = "lead2sub1sub2fired";
String HIERARCHY_WILDCARD = "wildcard";

default Map<String, MemberProfile> createHierarchy() {
var ceo = memberWithoutBoss(HIERARCHY_CEO);
var lead1 = memberWithSupervisor(HIERARCHY_LEAD1, ceo);
var lead1sub1 = memberWithSupervisor(HIERARCHY_LEAD1_SUB1, lead1);
var lead1sub2 = memberWithSupervisor(HIERARCHY_LEAD1_SUB2, lead1);
var lead2 = memberWithSupervisor(HIERARCHY_LEAD2, ceo);
var lead2sub1 = memberWithSupervisor(HIERARCHY_LEAD2_SUB1, lead2);
var lead2sub1sub1 = memberWithSupervisor(HIERARCHY_LEAD2_SUB1_SUB1, lead2sub1);
var lead2sub1sub2fired = memberWithSupervisor(HIERARCHY_LEAD2_SUB1_SUB2_FIRED, lead2sub1, LocalDate.now());
var wildcard = memberWithoutBoss(HIERARCHY_WILDCARD);
return Map.of(
HIERARCHY_CEO, ceo,
HIERARCHY_LEAD1, lead1,
HIERARCHY_LEAD1_SUB1, lead1sub1,
HIERARCHY_LEAD1_SUB2, lead1sub2,
HIERARCHY_LEAD2, lead2,
HIERARCHY_LEAD2_SUB1, lead2sub1,
HIERARCHY_LEAD2_SUB1_SUB1, lead2sub1sub1,
HIERARCHY_LEAD2_SUB1_SUB2_FIRED, lead2sub1sub2fired,
HIERARCHY_WILDCARD, wildcard
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ public interface PermissionFixture extends RolePermissionFixture {
Permission.CAN_UPDATE_REVIEW_PERIOD,
Permission.CAN_LAUNCH_REVIEW_PERIOD,
Permission.CAN_CLOSE_REVIEW_PERIOD,
Permission.CAN_DELETE_REVIEW_PERIOD
Permission.CAN_DELETE_REVIEW_PERIOD,
Permission.CAN_VIEW_ALL_PULSE_RESPONSES
);

default void setPermissionsForAdmin(UUID roleID) {
Expand All @@ -95,6 +96,5 @@ default void setPermissionsForPdl(UUID roleID) {

default void setPermissionsForMember(UUID roleID) {
memberPermissions.forEach(permission -> setRolePermission(roleID, permission));

}
}
Loading