Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature 2825/adjust admin menu #2828

Merged
merged 13 commits into from
Jan 23, 2025
Merged

Feature 2825/adjust admin menu #2828

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
523015b
Added permissions to upload hours, and edit/delete skills. Partially…
ocielliottc Jan 17, 2025
b786c08
Fixed skill name
ocielliottc Jan 17, 2025
a30fd25
Added the ability to check permissions on the current user and use th…
ocielliottc Jan 17, 2025
51f334a
Added an implementation of the hasPermission method.
ocielliottc Jan 17, 2025
fbe0b18
Added a permission for editing member roles, disallow removal of edit…
ocielliottc Jan 20, 2025
ae8bed0
Replace check for admin with permission check.
ocielliottc Jan 20, 2025
2d8c62f
Updated tests.
ocielliottc Jan 20, 2025
091554a
Updated exception message.
ocielliottc Jan 20, 2025
1bc6da6
Replace admin check with a new permission and renamed a method to ref…
ocielliottc Jan 20, 2025
40b92b4
Merge branch 'develop' into feature-2825/adjust-admin-menu
ocielliottc Jan 20, 2025
6360bfb
Renamed and modified the unsecure update method to only update the la…
ocielliottc Jan 21, 2025
29488da
Merge branch 'develop' into feature-2825/adjust-admin-menu
ocielliottc Jan 21, 2025
8d4b2ef
Merge branch 'develop' into feature-2825/adjust-admin-menu
mkimberlin Jan 22, 2025
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
16 changes: 3 additions & 13 deletions ...in/java/com/objectcomputing/checkins/services/employee_hours/EmployeeHoursController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.objectcomputing.checkins.services.employee_hours;

import com.objectcomputing.checkins.services.permissions.Permission;
import com.objectcomputing.checkins.services.permissions.RequiredPermission;
import com.objectcomputing.checkins.exceptions.NotFoundException;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.http.MediaType;
Expand Down Expand Up @@ -39,25 +41,13 @@ public Set<EmployeeHours> findEmployeeHours(@Nullable String employeeId) {
}


/**
* @param id
* @return
*/
@Get("/{id}")
public EmployeeHours readEmployeeHours(@NotNull UUID id) {
EmployeeHours result = employeeHoursServices.read(id);
if (result == null) {
throw new NotFoundException("No employee hours for employee id");
}
return result;
}

/**
* Parse the CSV file and store it to employee hours table
* @param file
* @{@link HttpResponse<EmployeeHoursResponseDTO>}
*/
@Post(uri="/upload" , consumes = MediaType.MULTIPART_FORM_DATA)
@RequiredPermission(Permission.CAN_UPLOAD_HOURS)
public EmployeeHoursResponseDTO upload(CompletedFileUpload file){
return employeeHoursServices.save(file);
}
Expand Down
2 changes: 0 additions & 2 deletions ...main/java/com/objectcomputing/checkins/services/employee_hours/EmployeeHoursServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ public interface EmployeeHoursServices {

EmployeeHoursResponseDTO save(CompletedFileUpload file);

EmployeeHours read(UUID id);

Set<EmployeeHours> findByFields(String employeeId);

}
6 changes: 0 additions & 6 deletions .../java/com/objectcomputing/checkins/services/employee_hours/EmployeeHoursServicesImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ public EmployeeHoursResponseDTO save(CompletedFileUpload file) {
List<EmployeeHours> employeeHoursList = new ArrayList<>();
Set<EmployeeHours> employeeHours = new HashSet<>();
EmployeeHoursResponseDTO responseDTO = new EmployeeHoursResponseDTO();
validate(!isAdmin, NOT_AUTHORIZED_MSG);
responseDTO.setRecordCountDeleted(employeehourRepo.count());
employeehourRepo.deleteAll();
try {
Expand All @@ -58,11 +57,6 @@ public EmployeeHoursResponseDTO save(CompletedFileUpload file) {
}


@Override
public EmployeeHours read(UUID id) {
return employeehourRepo.findById(id).orElse(null);
}

@Override
public Set<EmployeeHours> findByFields(String employeeId) {
MemberProfile currentUser = currentUserServices.getCurrentUser();
Expand Down
29 changes: 13 additions & 16 deletions server/src/main/java/com/objectcomputing/checkins/services/kudos/KudosServicesImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.objectcomputing.checkins.services.kudos;

import com.objectcomputing.checkins.services.permissions.Permission;
import com.objectcomputing.checkins.configuration.CheckInsConfiguration;
import com.objectcomputing.checkins.notifications.email.EmailSender;
import com.objectcomputing.checkins.notifications.email.MailJetFactory;
Expand Down Expand Up @@ -121,10 +122,6 @@ public Kudos save(KudosCreateDTO kudosDTO) {

@Override
public Kudos approve(Kudos kudos) {
if (!currentUserServices.isAdmin()) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}

UUID kudosId = kudos.getId();
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need to replace this with a permission check? I don't see it happening elsewhere in here, but I could be missing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The controller handles this:

    @Put
    @RequiredPermission(Permission.CAN_ADMINISTER_KUDOS)
    public Kudos approve(@Body @Valid Kudos kudos) {
        return kudosServices.approve(kudos);
    }

Kudos existingKudos = kudosRepository.findById(kudosId).orElseThrow(() ->
new BadArgException(KUDOS_DOES_NOT_EXIST_MSG.formatted(kudosId)));
Expand All @@ -151,7 +148,7 @@ public KudosResponseDTO getById(UUID id) {

if (kudos.getDateApproved() == null) {
// If not yet approved, only admins and the sender can access the kudos
if (!currentUserServices.isAdmin() && !isSender) {
if (!hasAdministerKudosPermission() && !isSender) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}
} else {
Expand All @@ -162,7 +159,7 @@ public KudosResponseDTO getById(UUID id) {
.stream()
.anyMatch(recipient -> recipient.getMemberId().equals(currentUserId));

if (!currentUserServices.isAdmin() && !isSender && !isRecipient) {
if (!hasAdministerKudosPermission() && !isSender && !isRecipient) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}
}
Expand All @@ -173,10 +170,6 @@ public KudosResponseDTO getById(UUID id) {

@Override
public void delete(UUID id) {
if (!currentUserServices.isAdmin()) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}

Kudos kudos = kudosRepository.findById(id).orElseThrow(() ->
Copy link
Member

Choose a reason for hiding this comment

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

Same above here. Is there a permission check happening?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same with the delete:

    @Delete("/{id}")
    @Status(HttpStatus.NO_CONTENT)
    @RequiredPermission(Permission.CAN_ADMINISTER_KUDOS)
    public void delete(@NotNull UUID id) {
        kudosServices.delete(id);
    }

new NotFoundException(KUDOS_DOES_NOT_EXIST_MSG.formatted(id)));

Expand All @@ -196,7 +189,7 @@ public List<KudosResponseDTO> findByValues(@Nullable UUID recipientId, @Nullable
} else if (senderId != null) {
return findAllFromMember(senderId);
} else {
if (!currentUserServices.isAdmin()) {
if (!hasAdministerKudosPermission()) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}

Expand All @@ -215,7 +208,7 @@ public List<KudosResponseDTO> getRecent() {
}

private List<KudosResponseDTO> findByPending(boolean isPending) {
if (!currentUserServices.isAdmin()) {
if (!hasAdministerKudosPermission()) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}

Expand All @@ -235,10 +228,10 @@ private List<KudosResponseDTO> findByPending(boolean isPending) {


private List<KudosResponseDTO> findAllToMember(UUID memberId) {
boolean isAdmin = currentUserServices.isAdmin();
UUID currentUserId = currentUserServices.getCurrentUser().getId();

if (!currentUserId.equals(memberId) && !isAdmin) {
if (!currentUserId.equals(memberId) &&
!hasAdministerKudosPermission()) {
throw new PermissionException("You are not authorized to retrieve the kudos another user has received");
}

Expand All @@ -261,10 +254,10 @@ private List<KudosResponseDTO> findAllToMember(UUID memberId) {

private List<KudosResponseDTO> findAllFromMember(UUID senderId) {

boolean isAdmin = currentUserServices.isAdmin();
UUID currentUserId = currentUserServices.getCurrentUser().getId();

if (!currentUserId.equals(senderId) && !isAdmin) {
if (!currentUserId.equals(senderId) &&
!hasAdministerKudosPermission()) {
throw new PermissionException("You are not authorized to retrieve the kudos another user has sent");
}

Expand Down Expand Up @@ -386,4 +379,8 @@ private void slackApprovedKudos(Kudos kudos) {
LOG.error("Unable to POST to Slack: " + httpResponse.reason());
}
}

private boolean hasAdministerKudosPermission() {
return currentUserServices.hasPermission(Permission.CAN_ADMINISTER_KUDOS);
}
}
2 changes: 1 addition & 1 deletion ...ain/java/com/objectcomputing/checkins/services/memberprofile/MemberProfileController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public HttpResponse<MemberProfileResponseDTO> save(@Body @Valid MemberProfileCre
*/
@Put
public HttpResponse<MemberProfileResponseDTO> update(@Body @Valid MemberProfileUpdateDTO memberProfile) {
MemberProfile savedProfile = memberProfileServices.saveProfile(fromDTO(memberProfile));
MemberProfile savedProfile = memberProfileServices.updateProfile(fromDTO(memberProfile));
return HttpResponse.ok(fromEntity(savedProfile))
.headers(headers -> headers.location(location(savedProfile.getId())));
}
Expand Down
2 changes: 2 additions & 0 deletions .../main/java/com/objectcomputing/checkins/services/memberprofile/MemberProfileServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,6 @@ Set<MemberProfile> findByValues(String firstName, String lastName, String title,
List<MemberProfile> getSubordinatesForId(UUID id);

MemberProfile updateProfile(MemberProfile memberProfile);

MemberProfile updateCurrentUserProfile(MemberProfile memberProfile);
}
67 changes: 44 additions & 23 deletions ...n/java/com/objectcomputing/checkins/services/memberprofile/MemberProfileServicesImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.*;

import static com.objectcomputing.checkins.util.Util.nullSafeUUIDToString;
import static com.objectcomputing.checkins.services.validate.PermissionsValidation.NOT_AUTHORIZED_MSG;

@Singleton
@CacheConfig("member-cache")
Expand Down Expand Up @@ -110,27 +111,9 @@ public MemberProfile saveProfile(MemberProfile memberProfile) {
emailAssignment(createdMemberProfile, true); // PDL
emailAssignment(createdMemberProfile, false); // Supervisor
return createdMemberProfile;
}

Optional<MemberProfile> existingProfileOpt = memberProfileRepository.findById(memberProfile.getId());
MemberProfile updatedMemberProfile = memberProfileRepository.update(memberProfile);
if (existingProfileOpt.isEmpty()) {
LOG.error("MemberProfile with id {} not found", memberProfile.getId());
} else {
MemberProfile existingProfile = existingProfileOpt.get();

boolean pdlChanged = !Objects.equals(existingProfile.getPdlId(), memberProfile.getPdlId());
boolean supervisorChanged = !Objects.equals(existingProfile.getSupervisorid(), memberProfile.getSupervisorid());

if (pdlChanged) {
emailAssignment(updatedMemberProfile, true); // PDL
}
if (supervisorChanged) {
emailAssignment(updatedMemberProfile, false); // Supervisor
}
throw new BadArgException("New member created with an id");
}

return updatedMemberProfile;
}

public void emailAssignment(MemberProfile member, boolean isPDL) {
Expand Down Expand Up @@ -165,9 +148,6 @@ public void emailAssignment(MemberProfile member, boolean isPDL) {
@Override
@CacheInvalidate(cacheNames = {"member-cache"})
public boolean deleteProfile(@NotNull UUID id) {
if (!currentUserServices.isAdmin()) {
throw new PermissionException("Requires admin privileges");
}
MemberProfile memberProfile = memberProfileRepository.findById(id).orElse(null);
Set<Role> userRoles = (memberProfile != null) ? roleServices.findUserRoles(memberProfile.getId()) : Collections.emptySet();

Expand All @@ -180,7 +160,7 @@ public boolean deleteProfile(@NotNull UUID id) {
} else if (!teamMemberServices.findByFields(null, id, null).isEmpty()) {
throw new BadArgException(String.format("User %s cannot be deleted since TeamMember record(s) exist", MemberProfileUtils.getFullName(memberProfile)));
} else if (!userRoles.isEmpty()) {
throw new BadArgException(String.format("User %s cannot be deleted since user has PDL role", MemberProfileUtils.getFullName(memberProfile)));
throw new BadArgException(String.format("User %s cannot be deleted since user has one or more roles", MemberProfileUtils.getFullName(memberProfile)));
}

// Update PDL ID for all associated members before termination
Expand Down Expand Up @@ -237,6 +217,47 @@ public List<MemberProfile> getSubordinatesForId(UUID id) {
@Override
@CacheInvalidate(cacheNames = {"member-cache"})
public MemberProfile updateProfile(MemberProfile memberProfile) {
if (memberProfile.getId() == null) {
throw new BadArgException("Null profile id in update");
}

MemberProfile currentUser = currentUserServices.getCurrentUser();
boolean isAdmin = currentUserServices.isAdmin();
if (!isAdmin && (currentUser == null || !currentUser.getId().equals(memberProfile.getId()))) {
Copy link
Member

Choose a reason for hiding this comment

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

Was this an intentional or accidental can kicking? 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, intentional in that there are no required permissions to update a member profile. Since every member can update their own profile, I didn't see a way to limit which profile could be updated except by using the existing mechanism of checking for admin. But, as I'm writing this, I'm thinking that we could add a permission to be able to "update all profiles" and specifically check for it here instead of admin. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think that would work!

Copy link
Member

Choose a reason for hiding this comment

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

👍 I'm onboard with that.

throw new PermissionException(NOT_AUTHORIZED_MSG);
}

MemberProfile emailProfile = memberProfileRepository.findByWorkEmail(memberProfile.getWorkEmail()).orElse(null);

if (emailProfile != null && emailProfile.getId() != null && !Objects.equals(memberProfile.getId(), emailProfile.getId())) {
throw new AlreadyExistsException(String.format("Email %s already exists in database",
memberProfile.getWorkEmail()));
}

Optional<MemberProfile> existingProfileOpt = memberProfileRepository.findById(memberProfile.getId());
MemberProfile updatedMemberProfile = memberProfileRepository.update(memberProfile);
if (existingProfileOpt.isEmpty()) {
LOG.error("MemberProfile with id {} not found", memberProfile.getId());
} else {
MemberProfile existingProfile = existingProfileOpt.get();

boolean pdlChanged = !Objects.equals(existingProfile.getPdlId(), memberProfile.getPdlId());
boolean supervisorChanged = !Objects.equals(existingProfile.getSupervisorid(), memberProfile.getSupervisorid());

if (pdlChanged) {
emailAssignment(updatedMemberProfile, true); // PDL
}
if (supervisorChanged) {
emailAssignment(updatedMemberProfile, false); // Supervisor
}
}

return updatedMemberProfile;
}

@Override
@CacheInvalidate(cacheNames = {"member-cache"})
public MemberProfile updateCurrentUserProfile(MemberProfile memberProfile) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be misleading. This isn't related to the "current user" at all, is it? Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it is only called from the current user controller when updating the "last seen" for the current user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it should be renamed to something like "directProfileUpdate" or something indicating that there is no security related to it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we isolate that code to update the "last seen" here and call it something like 'updateLastSeen' and just pass in the user id? that feels less risky than an unsecured profile update API.

return memberProfileRepository.update(memberProfile);
}
}
2 changes: 1 addition & 1 deletion ...om/objectcomputing/checkins/services/memberprofile/currentuser/CurrentUserController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public HttpResponse<CurrentUserDTO> currentUser(@Nullable Authentication authent
MemberProfile user = currentUserServices.findOrSaveUser(firstName, lastName, workEmail);

user.setLastSeen(LocalDate.now());
memberProfileServices.updateProfile(user);
memberProfileServices.updateCurrentUserProfile(user);
List<Permission> permissions = rolePermissionServices.findUserPermissions(user.getId());

Set<Role> roles = roleServices.findUserRoles(user.getId());
Expand Down
3 changes: 3 additions & 0 deletions .../com/objectcomputing/checkins/services/memberprofile/currentuser/CurrentUserServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.objectcomputing.checkins.services.memberprofile.currentuser;

import com.objectcomputing.checkins.services.memberprofile.MemberProfile;
import com.objectcomputing.checkins.services.permissions.Permission;
import com.objectcomputing.checkins.services.role.RoleType;

public interface CurrentUserServices {
Expand All @@ -9,6 +10,8 @@ public interface CurrentUserServices {

boolean hasRole(RoleType role);

boolean hasPermission(Permission permission);

boolean isAdmin();

MemberProfile getCurrentUser();
Expand Down
17 changes: 16 additions & 1 deletion .../objectcomputing/checkins/services/memberprofile/currentuser/CurrentUserServicesImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,22 @@

import com.objectcomputing.checkins.exceptions.AlreadyExistsException;
import com.objectcomputing.checkins.exceptions.NotFoundException;
import com.objectcomputing.checkins.services.permissions.Permission;
import com.objectcomputing.checkins.services.memberprofile.MemberProfile;
import com.objectcomputing.checkins.services.memberprofile.MemberProfileRepository;
import com.objectcomputing.checkins.services.role.Role;
import com.objectcomputing.checkins.services.role.RoleServices;
import com.objectcomputing.checkins.services.role.RoleType;
import com.objectcomputing.checkins.services.role.member_roles.MemberRoleServices;
import com.objectcomputing.checkins.services.role.role_permissions.RolePermissionServices;
import io.micronaut.security.authentication.Authentication;
import io.micronaut.security.utils.SecurityService;
import jakarta.inject.Singleton;
import jakarta.validation.constraints.NotNull;

import java.time.LocalDate;
import java.util.Optional;
import java.util.List;

@Singleton
public class CurrentUserServicesImpl implements CurrentUserServices {
Expand All @@ -23,14 +26,18 @@ public class CurrentUserServicesImpl implements CurrentUserServices {
private final SecurityService securityService;
private final RoleServices roleServices;
private final MemberRoleServices memberRoleServices;
private final RolePermissionServices rolePermissionServices;

public CurrentUserServicesImpl(MemberProfileRepository memberProfileRepository,
RoleServices roleServices,
SecurityService securityService, MemberRoleServices memberRoleServices) {
SecurityService securityService,
MemberRoleServices memberRoleServices,
RolePermissionServices rolePermissionServices) {
this.memberProfileRepo = memberProfileRepository;
this.roleServices = roleServices;
this.securityService = securityService;
this.memberRoleServices = memberRoleServices;
this.rolePermissionServices = rolePermissionServices;
}

@Override
Expand All @@ -48,6 +55,14 @@ public boolean hasRole(RoleType role) {
return securityService.hasRole(role.toString());
}

@Override
public boolean hasPermission(Permission permission) {
List<Permission> userPermissions =
rolePermissionServices.findUserPermissions(getCurrentUser().getId());
return userPermissions.stream().map(Permission::name)
.anyMatch(str -> str.equals(permission.name()));
}

@Override
public boolean isAdmin() {
return hasRole(RoleType.ADMIN);
Expand Down
3 changes: 3 additions & 0 deletions server/src/main/java/com/objectcomputing/checkins/services/permissions/Permission.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public enum Permission {
CAN_IMPERSONATE_MEMBERS("Impersonate organization members", "Security"),
CAN_VIEW_ROLE_PERMISSIONS("View role permissions", "Security"),
CAN_ASSIGN_ROLE_PERMISSIONS("Assign role permissions", "Security"),
CAN_EDIT_MEMBER_ROLES("Edit member roles", "Security"),
CAN_VIEW_PERMISSIONS("View all permissions", "Security"),
CAN_VIEW_SKILLS_REPORT("View skills report", "Reporting"),
CAN_VIEW_RETENTION_REPORT("View retention report", "Reporting"),
Expand All @@ -27,6 +28,7 @@ public enum Permission {
CAN_VIEW_PROFILE_REPORT("View profile report", "Reporting"),
CAN_VIEW_CHECKINS_REPORT("View checkins report", "Reporting"),
CAN_CREATE_MERIT_REPORT("Create Merit Reports", "Reporting"),
CAN_UPLOAD_HOURS("Upload Hours", "Reporting"),
CAN_CREATE_CHECKINS("Create check-ins", "Check-ins"),
CAN_VIEW_CHECKINS("View check-ins", "Check-ins"),
CAN_UPDATE_CHECKINS("Update check-ins", "Check-ins"),
Expand All @@ -41,6 +43,7 @@ public enum Permission {
CAN_VIEW_ALL_CHECKINS("View all check-ins", "Check-ins"),
CAN_UPDATE_ALL_CHECKINS("Update all check-ins, including completed check-ins", "Check-ins"),
CAN_EDIT_SKILL_CATEGORIES("Edit skill categories", "Skill Categories"),
CAN_EDIT_SKILLS("Edit skills", "Skills"),
CAN_CREATE_REVIEW_ASSIGNMENTS("Create review assignments", "Reviews"),
CAN_VIEW_REVIEW_ASSIGNMENTS("View review assignments", "Reviews"),
CAN_UPDATE_REVIEW_ASSIGNMENTS("Update review assignments", "Reviews"),
Expand Down
Loading
Loading