From 01b6d63e30dbff2a11e51f5af31580a3b3de067f Mon Sep 17 00:00:00 2001 From: Matt Perry Date: Tue, 28 May 2024 17:05:41 -0500 Subject: [PATCH 1/2] [2237] Cleaning up authorization --- .../agenda_item/AgendaItemController.java | 6 +-- .../agenda_item/AgendaItemServicesImpl.java | 21 +++-------- .../checkin_notes/CheckinNoteController.java | 7 ++-- .../CheckinNoteServicesImpl.java | 18 +++------ .../services/checkins/CheckInServices.java | 6 ++- .../checkins/CheckInServicesImpl.java | 37 +++++++++---------- .../PrivateNoteServicesImpl.java | 17 ++------- .../crud/ActionItemCRUDValidator.java | 17 ++------- .../agenda_item/AgendaItemControllerTest.java | 16 ++------ 9 files changed, 48 insertions(+), 97 deletions(-) diff --git a/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemController.java b/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemController.java index b773968832..9fd91b054a 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemController.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemController.java @@ -4,10 +4,8 @@ import com.objectcomputing.checkins.services.permissions.Permission; import com.objectcomputing.checkins.services.permissions.RequiredPermission; import io.micronaut.core.annotation.Nullable; -import io.micronaut.http.HttpRequest; import io.micronaut.http.HttpResponse; import io.micronaut.http.HttpStatus; -import io.micronaut.http.MediaType; import io.micronaut.http.annotation.*; import io.micronaut.http.uri.UriBuilder; import io.micronaut.scheduling.TaskExecutors; @@ -16,7 +14,6 @@ import io.micronaut.security.rules.SecurityRule; import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.validation.Valid; -import reactor.core.publisher.Mono; import java.net.URI; import java.util.List; @@ -70,7 +67,8 @@ HttpResponse updateAgendaItem(@Body @Valid AgendaItem agendaItem) { } /** - * Find agenda items that match all filled in parameters, return all results when given no params + * Find agenda items that match all filled in parameters, or return all results when provided no parameters, and + * the user has permission to view all checkin items * * @param checkinid {@link UUID} of checkin * @param createdbyid {@link UUID} of member diff --git a/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemServicesImpl.java index dc815c5eb1..c5386a6df4 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemServicesImpl.java @@ -1,6 +1,7 @@ package com.objectcomputing.checkins.services.agenda_item; import com.objectcomputing.checkins.exceptions.BadArgException; +import com.objectcomputing.checkins.exceptions.PermissionException; import com.objectcomputing.checkins.services.checkins.CheckIn; import com.objectcomputing.checkins.services.checkins.CheckInRepository; import com.objectcomputing.checkins.services.checkins.CheckInServices; @@ -135,23 +136,13 @@ public AgendaItem update(AgendaItem agendaItem) { } @Override - public Set findByFields(@Nullable UUID checkinid, @Nullable UUID createbyid) { - - + public Set findByFields(@Nullable UUID checkinId, @Nullable UUID createdById) { MemberProfile currentUser = currentUserServices.getCurrentUser(); - final UUID currentUserId = currentUser.getId(); - boolean canViewAllCheckins = checkInServices.canViewAllCheckins(currentUserId); - - if (checkinid != null) { - validate(!checkInServices.accessGranted(checkinid, currentUserId), "User is unauthorized to do this operation"); - } else if (createbyid != null) { - MemberProfile memberRecord = memberRepo.findById(createbyid).orElseThrow(); - validate(!currentUserId.equals(memberRecord.getId()) && !canViewAllCheckins, "User is unauthorized to do this operation"); - } else { - validate(!canViewAllCheckins, "User is unauthorized to do this operation"); + if(!checkInServices.doesUserHaveViewAccess(currentUser.getId(), checkinId, createdById)){ + throw new PermissionException("User is unauthorized to do this operation"); } - LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinid, createbyid); - return agendaItemRepository.search(nullSafeUUIDToString(checkinid), nullSafeUUIDToString(createbyid)); + LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinId, createdById); + return agendaItemRepository.search(nullSafeUUIDToString(checkinId), nullSafeUUIDToString(createdById)); } @Override diff --git a/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteController.java b/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteController.java index 48717accfa..d3361b774b 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteController.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteController.java @@ -5,7 +5,6 @@ import io.micronaut.core.annotation.Nullable; import io.micronaut.http.HttpRequest; import io.micronaut.http.HttpResponse; -import io.micronaut.http.MediaType; import io.micronaut.http.annotation.*; import io.micronaut.scheduling.TaskExecutors; import io.micronaut.scheduling.annotation.ExecuteOn; @@ -65,7 +64,8 @@ public HttpResponse updateCheckinNote(@Body @Valid CheckinNote chec } /** - * Get notes by checkind or createbyid + * Get notes by checkind or createbyid. If no parameters are provided, and user has permission to view all checkins, + * then return all checkin notes. * * @param checkinid * @param createdbyid @@ -73,8 +73,7 @@ public HttpResponse updateCheckinNote(@Body @Valid CheckinNote chec */ @Get("/{?checkinid,createdbyid}") @RequiredPermission(Permission.CAN_VIEW_CHECKINS) - public Set findCheckinNote(@Nullable UUID checkinid, - @Nullable UUID createdbyid) { + public Set findCheckinNote(@Nullable UUID checkinid, @Nullable UUID createdbyid) { return checkinNoteServices.findByFields(checkinid, createdbyid); } diff --git a/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteServicesImpl.java index c8b03c28ba..05bf409627 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/checkin_notes/CheckinNoteServicesImpl.java @@ -6,7 +6,6 @@ import com.objectcomputing.checkins.services.checkins.CheckIn; import com.objectcomputing.checkins.services.checkins.CheckInRepository; import com.objectcomputing.checkins.services.checkins.CheckInServices; -import com.objectcomputing.checkins.services.memberprofile.MemberProfile; import com.objectcomputing.checkins.services.memberprofile.MemberProfileRepository; import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices; import io.micronaut.core.annotation.Nullable; @@ -124,20 +123,13 @@ public CheckinNote update(@NotNull CheckinNote checkinNote) { } @Override - public Set findByFields(@Nullable UUID checkinid, @Nullable UUID createbyid) { + public Set findByFields(@Nullable UUID checkinId, @Nullable UUID createById) { final UUID currentUserId = currentUserServices.getCurrentUser().getId(); - boolean canViewAllCheckins = checkinServices.canViewAllCheckins(currentUserId); - - if (checkinid != null) { - validate(!checkinServices.accessGranted(checkinid, currentUserId), "User is unauthorized to do this operation"); - } else if (createbyid != null) { - MemberProfile memberRecord = memberRepo.findById(createbyid).orElseThrow(); - validate(!currentUserId.equals(memberRecord.getId()) && !canViewAllCheckins, "User is unauthorized to do this operation"); - } else { - validate(!canViewAllCheckins, "User is unauthorized to do this operation"); + if(!checkinServices.doesUserHaveViewAccess(currentUserId, checkinId, createById)){ + throw new PermissionException("User is unauthorized to do this operation"); } - LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinid, createbyid); - return checkinNoteRepository.search(nullSafeUUIDToString(checkinid), nullSafeUUIDToString(createbyid)); + LOG.info("Finding AgendaItem by checkinId: {}, and createById: {}", checkinId, createById); + return checkinNoteRepository.search(nullSafeUUIDToString(checkinId), nullSafeUUIDToString(createById)); } private void validate(boolean isError, String message, Object... args) { diff --git a/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServices.java b/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServices.java index 07523e3ffc..67e2e52f97 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServices.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServices.java @@ -1,10 +1,10 @@ package com.objectcomputing.checkins.services.checkins; +import com.objectcomputing.checkins.services.permissions.Permission; + import java.util.Set; import java.util.UUID; -import com.objectcomputing.checkins.services.permissions.Permission; - public interface CheckInServices { @@ -20,6 +20,8 @@ public interface CheckInServices { Boolean accessGranted(UUID checkinId, UUID memberId); + Boolean doesUserHaveViewAccess(UUID currentUserId, UUID checkinId, UUID createdById); + Boolean canViewAllCheckins(UUID memberId); Boolean canUpdateAllCheckins(UUID memberId); diff --git a/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServicesImpl.java index 02058ccf56..d717667626 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/checkins/CheckInServicesImpl.java @@ -7,7 +7,6 @@ import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices; import com.objectcomputing.checkins.services.permissions.Permission; import com.objectcomputing.checkins.services.role.RoleServices; -import com.objectcomputing.checkins.services.role.RoleType; import com.objectcomputing.checkins.services.role.role_permissions.RolePermissionServices; import com.objectcomputing.checkins.util.Util; import jakarta.inject.Singleton; @@ -56,21 +55,10 @@ public Boolean hasPermission(@NotNull UUID memberId, @NotNull Permission permiss public Boolean accessGranted(@NotNull UUID checkinId, @NotNull UUID memberId) { memberRepo.findById(memberId).orElseThrow(() -> new NotFoundException(String.format("Member %s not found", memberId))); - boolean canViewAllCheckins = canViewAllCheckins(memberId); - - // TODO: the check for Admin as a role should be removed when permissions are fully implemented - boolean isAdmin = false; - if (roleServices.findByRole(RoleType.ADMIN.name()).isPresent()){ - isAdmin = roleServices.findUserRoles(memberId) - .contains(roleServices.findByRole(RoleType.ADMIN.name()).get()); - LOG.debug("Member is Admin: {}", isAdmin); - } + if(!canViewAllCheckins(memberId)) { + CheckIn checkinRecord = checkinRepo.findById(checkinId).orElseThrow(() -> + new NotFoundException(String.format("Checkin not found by Id: %s.", checkinId))); - Boolean grantAccess = false; - if(canViewAllCheckins || isAdmin){ - grantAccess = true; - } else { - CheckIn checkinRecord = checkinRepo.findById(checkinId).orElseThrow(() -> new NotFoundException(String.format("Checkin %s not found", checkinId))); MemberProfile teamMemberOnCheckin = memberRepo.findById(checkinRecord.getTeamMemberId()).orElseThrow(() -> new NotFoundException(String.format("Team member not found %s not found", checkinRecord.getTeamMemberId()))); UUID currentPdlId = teamMemberOnCheckin.getPdlId(); @@ -80,13 +68,22 @@ public Boolean accessGranted(@NotNull UUID checkinId, @NotNull UUID memberId) { LOG.debug("PDL on Checkin: {}", checkinRecord.getPdlId()); LOG.debug("Current PDL: {}", currentPdlId); - if (memberId.equals(checkinRecord.getTeamMemberId()) + return memberId.equals(checkinRecord.getTeamMemberId()) || memberId.equals(checkinRecord.getPdlId()) - || memberId.equals(currentPdlId)) { - grantAccess = true; - } + || memberId.equals(currentPdlId); + } + return true; + } + + @Override + public Boolean doesUserHaveViewAccess(UUID currentUserId, UUID checkinId, UUID createdById) { + if (canViewAllCheckins(currentUserId)) { + return true; + } else if (checkinId != null) { + return accessGranted(checkinId, currentUserId); + } else { + return createdById != null && createdById.equals(currentUserId); } - return grantAccess; } @Override diff --git a/server/src/main/java/com/objectcomputing/checkins/services/private_notes/PrivateNoteServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/private_notes/PrivateNoteServicesImpl.java index ffb57dab7d..1d62399d35 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/private_notes/PrivateNoteServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/private_notes/PrivateNoteServicesImpl.java @@ -6,7 +6,6 @@ import com.objectcomputing.checkins.services.checkins.CheckIn; import com.objectcomputing.checkins.services.checkins.CheckInRepository; import com.objectcomputing.checkins.services.checkins.CheckInServices; -import com.objectcomputing.checkins.services.memberprofile.MemberProfile; import com.objectcomputing.checkins.services.memberprofile.MemberProfileRepository; import com.objectcomputing.checkins.services.memberprofile.MemberProfileServices; import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices; @@ -133,22 +132,12 @@ public PrivateNote update(@NotNull PrivateNote privateNote) { } @Override - public Set findByFields(@Nullable UUID checkinid, @Nullable UUID createbyid) { + public Set findByFields(@Nullable UUID checkinId, @Nullable UUID createById) { final UUID currentUserId = currentUserServices.getCurrentUser().getId(); - boolean canViewAllCheckins = checkinServices.canViewAllCheckins(currentUserId); - - if (checkinid != null) { - if (!checkinServices.accessGranted(checkinid, currentUserId)) - throw new PermissionException("User is unauthorized to do this operation"); - } else if (createbyid != null) { - MemberProfile memberRecord = memberRepo.findById(createbyid).orElseThrow(); - if (!currentUserId.equals(memberRecord.getId()) && !canViewAllCheckins) - throw new PermissionException("User is unauthorized to do this operation"); - } else if (!canViewAllCheckins) { + if(!checkinServices.doesUserHaveViewAccess(currentUserId, checkinId, createById)){ throw new PermissionException("User is unauthorized to do this operation"); } - - return privateNoteRepository.search(nullSafeUUIDToString(checkinid), nullSafeUUIDToString(createbyid)); + return privateNoteRepository.search(nullSafeUUIDToString(checkinId), nullSafeUUIDToString(createById)); } private void validate(boolean isError, String message, Object... args) { diff --git a/server/src/main/java/com/objectcomputing/checkins/services/validate/crud/ActionItemCRUDValidator.java b/server/src/main/java/com/objectcomputing/checkins/services/validate/crud/ActionItemCRUDValidator.java index 5aee6712a0..ee6bf00fef 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/validate/crud/ActionItemCRUDValidator.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/validate/crud/ActionItemCRUDValidator.java @@ -1,5 +1,6 @@ package com.objectcomputing.checkins.services.validate.crud; +import com.objectcomputing.checkins.exceptions.PermissionException; import com.objectcomputing.checkins.services.action_item.ActionItem; import com.objectcomputing.checkins.services.action_item.ActionItemRepository; import com.objectcomputing.checkins.services.checkins.CheckIn; @@ -154,20 +155,10 @@ public void validatePermissionsUpdate(@Valid @NotNull ActionItem actionItem) { } @Override - public void validatePermissionsFindByFields(UUID checkinid, UUID createdbyid) { + public void validatePermissionsFindByFields(UUID checkinId, UUID createdById) { final UUID currentUserId = currentUserServices.getCurrentUser().getId(); - boolean canViewAllCheckins = checkInServices.canViewAllCheckins(currentUserId); - - if (checkinid != null) { // find by checkinId validation - boolean allowedToView = checkInServices.accessGranted(checkinid, currentUserId); - permissionsValidation.validatePermissions(!allowedToView, "User is unauthorized to do this operation"); - } else if (createdbyid != null) { // find by createById validation - final UUID memberRecordId = memberServices.getById(createdbyid).getId(); - boolean currentUserIsCheckinSubject = currentUserId.equals(memberRecordId); - permissionsValidation.validatePermissions(!currentUserIsCheckinSubject && !canViewAllCheckins, - "User is unauthorized to do this operation"); - } else { // find all validation - permissionsValidation.validatePermissions(!canViewAllCheckins, "User is unauthorized to do this operation"); + if(!checkInServices.doesUserHaveViewAccess(currentUserId, checkinId, createdById)){ + throw new PermissionException("User is unauthorized to do this operation"); } } diff --git a/server/src/test/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemControllerTest.java index 7f37c39a49..f8ac370508 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/agenda_item/AgendaItemControllerTest.java @@ -19,19 +19,11 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Set; -import java.util.UUID; +import java.util.*; import java.util.stream.Collectors; -import static com.objectcomputing.checkins.services.role.RoleType.Constants.ADMIN_ROLE; -import static com.objectcomputing.checkins.services.role.RoleType.Constants.MEMBER_ROLE; -import static com.objectcomputing.checkins.services.role.RoleType.Constants.PDL_ROLE; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static com.objectcomputing.checkins.services.role.RoleType.Constants.*; +import static org.junit.jupiter.api.Assertions.*; class AgendaItemControllerTest extends TestContainersSuite implements MemberProfileFixture, CheckInFixture, AgendaItemFixture, RoleFixture { @@ -346,7 +338,7 @@ void testFindAllAgendaItemByNonAdmin() { String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText(); assertEquals(request.getPath(), href); - assertEquals("User is unauthorized to do this operation", error); + assertTrue(error.contains("User is unauthorized to do this operation")); } From 94bb39621893891823cdef75cbc6514ab3f31310 Mon Sep 17 00:00:00 2001 From: Matt Perry Date: Tue, 28 May 2024 17:34:03 -0500 Subject: [PATCH 2/2] [2237] Fixing tests --- .../action_item/ActionItemControllerTest.java | 18 +++++------------- .../checkins/CheckInControllerTest.java | 10 +++------- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/server/src/test/java/com/objectcomputing/checkins/services/action_item/ActionItemControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/action_item/ActionItemControllerTest.java index 875aae811a..9f7b5d3974 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/action_item/ActionItemControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/action_item/ActionItemControllerTest.java @@ -20,19 +20,11 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Set; -import java.util.UUID; +import java.util.*; import java.util.stream.Collectors; -import static com.objectcomputing.checkins.services.role.RoleType.Constants.ADMIN_ROLE; -import static com.objectcomputing.checkins.services.role.RoleType.Constants.MEMBER_ROLE; -import static com.objectcomputing.checkins.services.role.RoleType.Constants.PDL_ROLE; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static com.objectcomputing.checkins.services.role.RoleType.Constants.*; +import static org.junit.jupiter.api.Assertions.*; class ActionItemControllerTest extends TestContainersSuite implements MemberProfileFixture, RoleFixture, CheckInFixture, ActionItemFixture { @@ -174,7 +166,7 @@ void testCreateAnActionItemForExistingCheckInId() { String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText(); assertEquals(request.getPath(), href); - assertEquals(String.format("Checkin %s not found", actionItemCreateDTO.getCheckinid()), error); + assertEquals(String.format("Checkin not found by Id: %s.", actionItemCreateDTO.getCheckinid()), error); assertEquals(HttpStatus.NOT_FOUND, responseException.getStatus()); } @@ -730,7 +722,7 @@ void testUpdateNonExistingActionItemForCheckInId() { String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText(); assertEquals(request.getPath(), href); - assertEquals(String.format("Checkin %s not found", actionItem.getCheckinid()), error); + assertEquals(String.format("Checkin not found by Id: %s.", actionItem.getCheckinid()), error); } diff --git a/server/src/test/java/com/objectcomputing/checkins/services/checkins/CheckInControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/checkins/CheckInControllerTest.java index 58456dab7b..5689317fc5 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/checkins/CheckInControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/checkins/CheckInControllerTest.java @@ -14,20 +14,16 @@ import io.micronaut.http.client.HttpClient; import io.micronaut.http.client.annotation.Client; import io.micronaut.http.client.exceptions.HttpClientResponseException; - +import jakarta.inject.Inject; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import jakarta.inject.Inject; import java.time.LocalDateTime; import java.util.*; import java.util.stream.Collectors; import static com.objectcomputing.checkins.services.role.RoleType.Constants.*; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.*; class CheckInControllerTest extends TestContainersSuite implements MemberProfileFixture, CheckInFixture, RoleFixture { @@ -375,7 +371,7 @@ void testGetReadCheckInDoesNotExist() { String href = Objects.requireNonNull(body).get("_links").get("self").get("href").asText(); assertEquals(request.getPath(), href); - assertEquals(String.format("Checkin %s not found", randomCheckinID), error); + assertEquals(String.format("Checkin not found by Id: %s.", randomCheckinID), error); } @Test