Skip to content

Fix permissions for updating earned certifications #2495

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

Merged
timyates merged 2 commits into from
Jun 14, 2024
Merged

Fix permissions for updating earned certifications #2495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f67afd
Fix permissions for updating earned certifications
timyates Jun 12, 2024
e0f9ca3
Merge branch 'develop' into bugfix-certification-permission-fix
Jun 12, 2024
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
18 changes: 16 additions & 2 deletions ...in/java/com/objectcomputing/checkins/services/certification/CertificationServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<UUID> 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) {
Expand Down
12 changes: 8 additions & 4 deletions ...va/com/objectcomputing/checkins/services/certification/EarnedCertificationRepository.java
View file
Open in desktop
Copy link
Contributor Author

@timyates timyates Jun 12, 2024

Choose a reason for hiding this comment

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

I also changed the join from LEFT JOIN to JOIN, this is a no-op I believe as there cannot be one without the other, however JOIN is more correct

Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -17,15 +19,15 @@ public interface EarnedCertificationRepository extends CrudRepository<EarnedCert
@Query("""
SELECT earned.*
FROM earned_certification AS earned
LEFT JOIN certification AS cert USING(certification_id)
JOIN certification AS cert USING(certification_id)
WHERE cert.is_active = TRUE OR :includeDeactivated = TRUE
ORDER BY earned.earned_date DESC, earned.description""")
List<EarnedCertification> findAllOrderByEarnedDateDesc(boolean includeDeactivated);

@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""")
Expand All @@ -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""")
Expand All @@ -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<EarnedCertification> findByMemberIdAndCertificationIdOrderByEarnedDateDesc(@NotNull UUID memberId, @NotNull UUID certificationId, boolean includeDeactivated);

Optional<EarnedCertification> findById(@Nullable UUID id);
}
48 changes: 48 additions & 0 deletions ...om/objectcomputing/checkins/services/certification/EarnedCertificationControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down