From 0f67afd370c0aeb7e97a23350c4ea04516ad5c85 Mon Sep 17 00:00:00 2001 From: Tim Yates Date: Wed, 12 Jun 2024 13:46:43 +0100 Subject: [PATCH] Fix permissions for updating earned certifications Previously we checked the owner of the passed in object, but not the owner of the db object in the case of an update. This meant that people could effectively change ownership of earned certifications from someone else to themselves. This commit makes the change so we check both the object in flight, and the object at rest for updates. --- .../CertificationServiceImpl.java | 18 ++++++- .../EarnedCertificationRepository.java | 12 +++-- .../EarnedCertificationControllerTest.java | 48 +++++++++++++++++++ 3 files changed, 72 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/com/objectcomputing/checkins/services/certification/CertificationServiceImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/certification/CertificationServiceImpl.java index c9bd324810..a1af17ac41 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/certification/CertificationServiceImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/certification/CertificationServiceImpl.java @@ -136,10 +136,24 @@ private void validateEarnedCertificate(EarnedCertification earnedCertification, } private void validatePermission(EarnedCertification earnedCertification, String action) { - // Fail if the user doesn't have permission to modify the earned certification UUID currentUserId = currentUserServices.getCurrentUser().getId(); + + // Check if they have the admin permission boolean hasPermission = rolePermissionServices.findUserPermissions(currentUserId).contains(Permission.CAN_MANAGE_EARNED_CERTIFICATIONS); - validate(!hasPermission && !earnedCertification.getMemberId().equals(currentUserId), "User %s does not have permission to %s Earned Certificate for user %s", currentUserId, action, earnedCertification.getMemberId()); + if (hasPermission) { + return; + } + + // Verify the userId in the request matches the current user + if (!earnedCertification.getMemberId().equals(currentUserId)) { + throw new BadArgException("User %s does not have permission to %s Earned Certificate for user %s".formatted(currentUserId, action, earnedCertification.getMemberId())); + } + + // Verify the current user owns the earned certification they are trying to modify (if it exists in the db) + Optional dbUuid = earnedCertificationRepository.findById(earnedCertification.getId()).map(EarnedCertification::getMemberId); + if (dbUuid.map(id -> !id.equals(currentUserId)).orElse(false)) { + throw new BadArgException("User %s does not have permission to %s Earned Certificate for user %s".formatted(currentUserId, action, dbUuid.orElse(null))); + } } private void validate(boolean isError, String message, Object... args) { diff --git a/server/src/main/java/com/objectcomputing/checkins/services/certification/EarnedCertificationRepository.java b/server/src/main/java/com/objectcomputing/checkins/services/certification/EarnedCertificationRepository.java index cefa5ccb97..bc658b023e 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/certification/EarnedCertificationRepository.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/certification/EarnedCertificationRepository.java @@ -4,9 +4,11 @@ import io.micronaut.data.jdbc.annotation.JdbcRepository; import io.micronaut.data.model.query.builder.sql.Dialect; import io.micronaut.data.repository.CrudRepository; +import jakarta.annotation.Nullable; import jakarta.validation.constraints.NotNull; import java.util.List; +import java.util.Optional; import java.util.UUID; @JdbcRepository(dialect = Dialect.POSTGRES) @@ -17,7 +19,7 @@ public interface EarnedCertificationRepository extends CrudRepository findAllOrderByEarnedDateDesc(boolean includeDeactivated); @@ -25,7 +27,7 @@ LEFT JOIN certification AS cert USING(certification_id) @Query(""" SELECT earned.* FROM earned_certification AS earned - LEFT JOIN certification AS cert USING(certification_id) + JOIN certification AS cert USING(certification_id) WHERE earned.certification_id = :certificationId AND (cert.is_active = TRUE OR :includeDeactivated = TRUE) ORDER BY earned.earned_date DESC, earned.description""") @@ -34,7 +36,7 @@ LEFT JOIN certification AS cert USING(certification_id) @Query(""" SELECT earned.* FROM earned_certification AS earned - LEFT JOIN certification AS cert USING(certification_id) + JOIN certification AS cert USING(certification_id) WHERE earned.member_id = :memberId AND (cert.is_active = TRUE OR :includeDeactivated = TRUE) ORDER BY earned.earned_date DESC, earned.description""") @@ -43,10 +45,12 @@ LEFT JOIN certification AS cert USING(certification_id) @Query(""" SELECT earned.* FROM earned_certification AS earned - LEFT JOIN certification AS cert USING(certification_id) + JOIN certification AS cert USING(certification_id) WHERE earned.certification_id = :certificationId AND earned.member_id = :memberId AND (cert.is_active = TRUE OR :includeDeactivated = TRUE) ORDER BY earned.earned_date DESC, earned.description""") List findByMemberIdAndCertificationIdOrderByEarnedDateDesc(@NotNull UUID memberId, @NotNull UUID certificationId, boolean includeDeactivated); + + Optional findById(@Nullable UUID id); } diff --git a/server/src/test/java/com/objectcomputing/checkins/services/certification/EarnedCertificationControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/certification/EarnedCertificationControllerTest.java index e75d580290..9acf9840e7 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/certification/EarnedCertificationControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/certification/EarnedCertificationControllerTest.java @@ -171,6 +171,54 @@ void cannotUpdateEarnedCertificationForOthers() { assertEquals("User %s does not have permission to update Earned Certificate for user %s".formatted(tim.getId(), member.getId()), exception.getMessage()); } + @Test + void cannotUpdateEarnedCertificationToSomeoneElse() { + MemberProfile tim = memberWithoutBoss("Tim"); + MemberProfile sarah = memberWithoutBoss("Sarah"); + + Certification certification = createDefaultCertification(); + EarnedCertification sarahsCertification = createEarnedCertification(sarah, certification, "Description", LocalDate.now()); + + // Try to change the owner to me + EarnedCertificationDTO update = new EarnedCertificationDTO( + tim.getId(), + sarahsCertification.getCertificationId(), + sarahsCertification.getDescription(), + sarahsCertification.getEarnedDate() + ); + HttpClientResponseException exception = assertThrows(HttpClientResponseException.class, () -> + earnedCertificationClient.toBlocking() + .retrieve(HttpRequest.PUT("/%s".formatted(sarahsCertification.getId()), update) + .basicAuth(sarah.getWorkEmail(), MEMBER_ROLE)) + ); + assertEquals(HttpStatus.BAD_REQUEST, exception.getStatus()); + assertEquals("User %s does not have permission to update Earned Certificate for user %s".formatted(sarah.getId(), tim.getId()), exception.getMessage()); + } + + @Test + void cannotUpdateSomeoneElsesEarnedCertificateToMe() { + MemberProfile tim = memberWithoutBoss("Tim"); + MemberProfile sarah = memberWithoutBoss("Sarah"); + + Certification certification = createDefaultCertification(); + EarnedCertification sarahsCertification = createEarnedCertification(sarah, certification, "Description", LocalDate.now()); + + // Try to change the owner to me + EarnedCertificationDTO update = new EarnedCertificationDTO( + tim.getId(), + sarahsCertification.getCertificationId(), + sarahsCertification.getDescription(), + sarahsCertification.getEarnedDate() + ); + HttpClientResponseException exception = assertThrows(HttpClientResponseException.class, () -> + earnedCertificationClient.toBlocking() + .retrieve(HttpRequest.PUT("/%s".formatted(sarahsCertification.getId()), update) + .basicAuth(tim.getWorkEmail(), MEMBER_ROLE)) + ); + assertEquals(HttpStatus.BAD_REQUEST, exception.getStatus()); + assertEquals("User %s does not have permission to update Earned Certificate for user %s".formatted(tim.getId(), sarah.getId()), exception.getMessage()); + } + @Test void canDeleteEarnedCertificationWithRole() { MemberProfile member = createADefaultMemberProfile();