diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/DefaultEnrollmentService.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/DefaultEnrollmentService.java index 9a252cf141ca..91a610e5441d 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/DefaultEnrollmentService.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/DefaultEnrollmentService.java @@ -84,23 +84,21 @@ public Enrollment getEnrollment(String uid, EnrollmentParams params, boolean inc throw new NotFoundException(Enrollment.class, uid); } - return getEnrollment(enrollment, params, includeDeleted, null); - } - - @Override - public Enrollment getEnrollment( - @Nonnull Enrollment enrollment, - EnrollmentParams params, - boolean includeDeleted, - OrganisationUnitSelectionMode orgUnitMode) - throws ForbiddenException { User user = currentUserService.getCurrentUser(); - List errors = trackerAccessManager.canRead(user, enrollment, orgUnitMode == ALL); + List errors = trackerAccessManager.canRead(user, enrollment, false); if (!errors.isEmpty()) { throw new ForbiddenException(errors.toString()); } + return getEnrollment(enrollment, params, includeDeleted, user); + } + + @Override + public Enrollment getEnrollment( + @Nonnull Enrollment enrollment, EnrollmentParams params, boolean includeDeleted, User user) + throws ForbiddenException { + Enrollment result = new Enrollment(); result.setId(enrollment.getId()); result.setUid(enrollment.getUid()); @@ -228,8 +226,9 @@ private List getEnrollments( if (enrollment != null && (orgUnitMode == ALL || trackerOwnershipAccessManager.hasAccess( - user, enrollment.getTrackedEntity(), enrollment.getProgram()))) { - enrollmentList.add(getEnrollment(enrollment, params, includeDeleted, orgUnitMode)); + user, enrollment.getTrackedEntity(), enrollment.getProgram())) + && trackerAccessManager.canRead(user, enrollment, orgUnitMode == ALL).isEmpty()) { + enrollmentList.add(getEnrollment(enrollment, params, includeDeleted, user)); } } diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapper.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapper.java index 28245a5e3464..4a9efda62134 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapper.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapper.java @@ -35,7 +35,6 @@ import java.util.HashSet; import java.util.Set; import lombok.RequiredArgsConstructor; -import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.feedback.BadRequestException; import org.hisp.dhis.feedback.ForbiddenException; import org.hisp.dhis.organisationunit.OrganisationUnit; @@ -79,7 +78,8 @@ public EnrollmentQueryParams map(EnrollmentOperationParams operationParams) Program program = validateProgram(operationParams.getProgramUid(), user); TrackedEntityType trackedEntityType = validateTrackedEntityType(operationParams.getTrackedEntityTypeUid(), user); - TrackedEntity trackedEntity = validateTrackedEntity(operationParams.getTrackedEntityUid()); + TrackedEntity trackedEntity = + validateTrackedEntity(operationParams.getTrackedEntityUid(), user); Set orgUnits = validateOrgUnits(operationParams.getOrgUnitUids(), user); validateOrgUnitMode(operationParams.getOrgUnitMode(), user, program); @@ -122,7 +122,8 @@ private void mergeOrgUnitModes( } } - private Program validateProgram(String uid, User user) throws BadRequestException { + private Program validateProgram(String uid, User user) + throws BadRequestException, ForbiddenException { if (uid == null) { return null; } @@ -133,14 +134,14 @@ private Program validateProgram(String uid, User user) throws BadRequestExceptio } if (!aclService.canDataRead(user, program)) { - throw new IllegalQueryException( + throw new ForbiddenException( "Current user is not authorized to read data from selected program: " + program.getUid()); } if (program.getTrackedEntityType() != null && !aclService.canDataRead(user, program.getTrackedEntityType())) { - throw new IllegalQueryException( + throw new ForbiddenException( "Current user is not authorized to read data from selected program's tracked entity type: " + program.getTrackedEntityType().getUid()); } @@ -149,7 +150,7 @@ private Program validateProgram(String uid, User user) throws BadRequestExceptio } private TrackedEntityType validateTrackedEntityType(String uid, User user) - throws BadRequestException { + throws BadRequestException, ForbiddenException { if (uid == null) { return null; } @@ -160,7 +161,7 @@ private TrackedEntityType validateTrackedEntityType(String uid, User user) } if (!aclService.canDataRead(user, trackedEntityType)) { - throw new IllegalQueryException( + throw new ForbiddenException( "Current user is not authorized to read data from selected tracked entity type: " + trackedEntityType.getUid()); } @@ -168,7 +169,8 @@ private TrackedEntityType validateTrackedEntityType(String uid, User user) return trackedEntityType; } - private TrackedEntity validateTrackedEntity(String uid) throws BadRequestException { + private TrackedEntity validateTrackedEntity(String uid, User user) + throws BadRequestException, ForbiddenException { if (uid == null) { return null; } @@ -178,6 +180,13 @@ private TrackedEntity validateTrackedEntity(String uid) throws BadRequestExcepti throw new BadRequestException("Tracked entity is specified but does not exist: " + uid); } + if (trackedEntity.getTrackedEntityType() != null + && !aclService.canDataRead(user, trackedEntity.getTrackedEntityType())) { + throw new ForbiddenException( + "Current user is not authorized to read data from type of selected tracked entity: " + + trackedEntity.getTrackedEntityType().getUid()); + } + return trackedEntity; } diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentService.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentService.java index 975401395a0c..a4137d833174 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentService.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentService.java @@ -29,23 +29,20 @@ import java.util.List; import java.util.Set; -import org.hisp.dhis.common.OrganisationUnitSelectionMode; import org.hisp.dhis.feedback.BadRequestException; import org.hisp.dhis.feedback.ForbiddenException; import org.hisp.dhis.feedback.NotFoundException; import org.hisp.dhis.program.Enrollment; import org.hisp.dhis.tracker.export.Page; import org.hisp.dhis.tracker.export.PageParams; +import org.hisp.dhis.user.User; public interface EnrollmentService { Enrollment getEnrollment(String uid, EnrollmentParams params, boolean includeDeleted) throws NotFoundException, ForbiddenException; Enrollment getEnrollment( - Enrollment enrollment, - EnrollmentParams params, - boolean includeDeleted, - OrganisationUnitSelectionMode orgUnitMode) + Enrollment enrollment, EnrollmentParams params, boolean includeDeleted, User user) throws ForbiddenException; /** Get all enrollments matching given params. */ diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapperTest.java b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapperTest.java index 9e7b4002ece1..89db04102052 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapperTest.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentOperationParamsMapperTest.java @@ -44,7 +44,6 @@ import java.util.List; import java.util.Set; import org.hisp.dhis.common.BaseIdentifiableObject; -import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.feedback.BadRequestException; import org.hisp.dhis.feedback.ForbiddenException; import org.hisp.dhis.organisationunit.OrganisationUnit; @@ -142,6 +141,7 @@ void setUp() { trackedEntity = new TrackedEntity(); trackedEntity.setUid(TRACKED_ENTITY_UID); + trackedEntity.setTrackedEntityType(trackedEntityType); when(trackedEntityService.getTrackedEntity(TRACKED_ENTITY_UID)).thenReturn(trackedEntity); } @@ -185,7 +185,7 @@ void shouldMapOrgUnitsWhenOrgUnitUidsAreSpecified() } @Test - void shouldThrowExceptionWhenOrgUnitNotFound() { + void shouldThrowBadRequestExceptionWhenOrgUnitNotFound() { EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder() .orgUnitUids(Set.of("JW6BrFd0HLu")) @@ -203,7 +203,7 @@ void shouldThrowExceptionWhenOrgUnitNotFound() { } @Test - void shouldThrowExceptionWhenOrgUnitNotInScope() { + void shouldThrowForbiddenExceptionWhenOrgUnitNotInScope() { EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder().orgUnitUids(Set.of(ORG_UNIT_1_UID)).build(); when(organisationUnitService.isInUserHierarchy( @@ -239,7 +239,7 @@ void shouldMapParamsWhenOrgUnitNotInScopeButUserIsSuperuser() } @Test - void shouldFailWhenOrgUnitNotInScopeAndUserHasSearchInAllAuthority() { + void shouldThrowForbiddenExceptionWhenOrgUnitNotInScopeAndUserHasSearchInAllAuthority() { User user = createUser(F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS.name()); user.setTeiSearchOrganisationUnits(Set.of(orgUnit2)); @@ -283,7 +283,7 @@ void shouldMapProgramWhenProgramUidIsSpecified() throws BadRequestException, For } @Test - void shouldThrowExceptionWhenProgramNotFound() { + void shouldThrowBadRequestExceptionWhenProgramNotFound() { EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder().programUid("JW6BrFd0HLu").build(); @@ -293,15 +293,14 @@ void shouldThrowExceptionWhenProgramNotFound() { } @Test - void shouldFailWhenUserCantReadProgramData() { + void shouldThrowForbiddenExceptionWhenUserCantReadProgramData() { when(programService.getProgram(PROGRAM_UID)).thenReturn(program); when(aclService.canDataRead(user, program)).thenReturn(false); EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder().programUid(PROGRAM_UID).build(); - Exception exception = - assertThrows(IllegalQueryException.class, () -> mapper.map(operationParams)); + Exception exception = assertThrows(ForbiddenException.class, () -> mapper.map(operationParams)); assertEquals( String.format( "Current user is not authorized to read data from selected program: %s", PROGRAM_UID), @@ -309,7 +308,7 @@ void shouldFailWhenUserCantReadProgramData() { } @Test - void shouldFailWhenUserCantReadProgramTrackedEntityTypeData() { + void shouldThrowForbiddenExceptionWhenUserCantReadProgramTrackedEntityTypeData() { when(programService.getProgram(PROGRAM_UID)).thenReturn(program); when(aclService.canDataRead(user, program)).thenReturn(true); when(aclService.canDataRead(user, program.getTrackedEntityType())).thenReturn(false); @@ -317,8 +316,7 @@ void shouldFailWhenUserCantReadProgramTrackedEntityTypeData() { EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder().programUid(PROGRAM_UID).build(); - Exception exception = - assertThrows(IllegalQueryException.class, () -> mapper.map(operationParams)); + Exception exception = assertThrows(ForbiddenException.class, () -> mapper.map(operationParams)); assertEquals( String.format( "Current user is not authorized to read data from selected program's tracked entity type: %s", @@ -342,7 +340,7 @@ void shouldMapTrackedEntityTypeWhenTrackedEntityTypeUidIsSpecified() } @Test - void shouldThrowExceptionWhenTrackedEntityTypeNotFound() { + void shouldThrowBadRequestExceptionWhenTrackedEntityTypeNotFound() { EnrollmentOperationParams requestParams = EnrollmentOperationParams.builder().trackedEntityTypeUid("JW6BrFd0HLu").build(); @@ -352,7 +350,7 @@ void shouldThrowExceptionWhenTrackedEntityTypeNotFound() { } @Test - void shouldFailWhenUserCantReadTrackedEntityTypeData() { + void shouldThrowBadRequestExceptionWhenUserCantReadTrackedEntityTypeData() { when(trackedEntityTypeService.getTrackedEntityType(TRACKED_ENTITY_TYPE_UID)) .thenReturn(trackedEntityType); when(aclService.canDataRead(user, trackedEntityType)).thenReturn(false); @@ -376,13 +374,15 @@ void shouldMapTrackedEntityWhenTrackedEntityUidIsSpecified() .orgUnitMode(ACCESSIBLE) .build(); + when(aclService.canDataRead(user, trackedEntity.getTrackedEntityType())).thenReturn(true); + EnrollmentQueryParams params = mapper.map(operationParams); assertEquals(trackedEntity, params.getTrackedEntity()); } @Test - void shouldThrowExceptionTrackedEntityNotFound() { + void shouldThrowBadRequestExceptionTrackedEntityNotFound() { EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder().trackedEntityUid("JW6BrFd0HLu").build(); @@ -392,6 +392,21 @@ void shouldThrowExceptionTrackedEntityNotFound() { "Tracked entity is specified but does not exist: JW6BrFd0HLu", exception.getMessage()); } + @Test + void shouldThrowForbiddenExceptionWhenTypeOfTrackedEntityNotAccessible() { + EnrollmentOperationParams operationParams = + EnrollmentOperationParams.builder().trackedEntityUid(TRACKED_ENTITY_UID).build(); + + when(aclService.canDataRead(user, trackedEntity.getTrackedEntityType())).thenReturn(false); + + Exception exception = assertThrows(ForbiddenException.class, () -> mapper.map(operationParams)); + assertEquals( + String.format( + "Current user is not authorized to read data from type of selected tracked entity: %s", + trackedEntity.getTrackedEntityType().getUid()), + exception.getMessage()); + } + @Test void shouldMapOrderInGivenOrder() throws BadRequestException, ForbiddenException { EnrollmentOperationParams operationParams = diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentServiceTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentServiceTest.java index a26dc76b5fd9..b6e4836c8f33 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentServiceTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/export/enrollment/EnrollmentServiceTest.java @@ -88,7 +88,7 @@ class EnrollmentServiceTest extends TransactionalIntegrationTest { @Autowired private IdentifiableObjectManager manager; - private User admin; + private User superuser; private User userWithoutOrgUnit; @@ -125,7 +125,7 @@ class EnrollmentServiceTest extends TransactionalIntegrationTest { @Override protected void setUpTest() throws Exception { userService = _userService; - admin = preCreateInjectAdminUser(); + superuser = preCreateInjectAdminUser(); orgUnitA = createOrganisationUnit('A'); manager.save(orgUnitA, false); @@ -177,12 +177,18 @@ protected void setUpTest() throws Exception { programA = createProgram('A', new HashSet<>(), orgUnitA); programA.setProgramType(ProgramType.WITH_REGISTRATION); programA.setTrackedEntityType(trackedEntityTypeA); - programA.getSharing().setOwner(admin); + programA.getSharing().setOwner(superuser); programA.getSharing().setPublicAccess(AccessStringHelper.DATA_READ); manager.save(programA, false); + Program programB = createProgram('B', new HashSet<>(), orgUnitB); + programB.setProgramType(ProgramType.WITH_REGISTRATION); + programB.setTrackedEntityType(trackedEntityTypeA); + programB.getSharing().setPublicAccess(AccessStringHelper.DEFAULT); + manager.save(programB, false); + trackedEntityAttributeA = createTrackedEntityAttribute('A'); - trackedEntityAttributeA.getSharing().setOwner(admin); + trackedEntityAttributeA.getSharing().setOwner(superuser); manager.save(trackedEntityAttributeA, false); TrackedEntityAttributeValue trackedEntityAttributeValueA = new TrackedEntityAttributeValue(); trackedEntityAttributeValueA.setAttribute(trackedEntityAttributeA); @@ -190,18 +196,25 @@ protected void setUpTest() throws Exception { trackedEntityAttributeValueA.setValue("12"); trackedEntityA.setTrackedEntityAttributeValues(Set.of(trackedEntityAttributeValueA)); manager.update(trackedEntityA); + programA.setProgramAttributes( List.of(createProgramTrackedEntityAttribute(programA, trackedEntityAttributeA))); manager.update(programA); + programB.setProgramAttributes( + List.of(createProgramTrackedEntityAttribute(programB, trackedEntityAttributeA))); + manager.update(programB); + programStageA = createProgramStage('A', programA); manager.save(programStageA, false); ProgramStage inaccessibleProgramStage = createProgramStage('B', programA); - inaccessibleProgramStage.getSharing().setOwner(admin); + inaccessibleProgramStage.getSharing().setOwner(superuser); inaccessibleProgramStage.setPublicAccess(AccessStringHelper.DEFAULT); manager.save(inaccessibleProgramStage, false); programA.setProgramStages(Set.of(programStageA, inaccessibleProgramStage)); manager.save(programA, false); + programB.setProgramStages(Set.of(programStageA, inaccessibleProgramStage)); + manager.save(programB, false); relationshipTypeA = createRelationshipType('A'); relationshipTypeA @@ -242,7 +255,7 @@ protected void setUpTest() throws Exception { enrollmentB = programInstanceService.enrollTrackedEntity( - trackedEntityB, programA, new Date(), new Date(), orgUnitB); + trackedEntityB, programB, new Date(), new Date(), orgUnitB); enrollmentChildA = programInstanceService.enrollTrackedEntity( @@ -298,7 +311,7 @@ void shouldGetEnrollmentWithEventsWhenUserHasAccessToEvent() @Test void shouldGetEnrollmentWithoutEventsWhenUserHasNoAccessToProgramStage() throws ForbiddenException, NotFoundException { - programStageA.getSharing().setOwner(admin); + programStageA.getSharing().setOwner(superuser); programStageA.getSharing().setPublicAccess(AccessStringHelper.DEFAULT); manager.updateNoAcl(programStageA); @@ -326,7 +339,7 @@ void shouldGetEnrollmentWithRelationshipsWhenUserHasAccessToThem() @Test void shouldGetEnrollmentWithoutRelationshipsWhenUserHasAccessToThem() throws ForbiddenException, NotFoundException { - relationshipTypeA.getSharing().setOwner(admin); + relationshipTypeA.getSharing().setOwner(superuser); relationshipTypeA.getSharing().setPublicAccess(AccessStringHelper.DEFAULT); EnrollmentParams params = EnrollmentParams.FALSE; @@ -352,7 +365,7 @@ void shouldGetEnrollmentWithAttributesWhenUserHasAccessToThem() @Test void shouldFailGettingEnrollmentWhenUserHasNoAccessToProgramsTrackedEntityType() { - trackedEntityTypeA.getSharing().setOwner(admin); + trackedEntityTypeA.getSharing().setOwner(superuser); trackedEntityTypeA.getSharing().setPublicAccess(AccessStringHelper.DEFAULT); manager.updateNoAcl(trackedEntityTypeA); @@ -428,11 +441,7 @@ void shouldGetEnrollmentsWhenUserHasReadAccessToProgramAndSearchScopeAccessToOrg assertNotNull(enrollments); assertContainsOnly( - List.of( - enrollmentA.getUid(), - enrollmentB.getUid(), - enrollmentChildA.getUid(), - enrollmentGrandchildA.getUid()), + List.of(enrollmentA.getUid(), enrollmentChildA.getUid(), enrollmentGrandchildA.getUid()), uids(enrollments)); } @@ -453,11 +462,7 @@ void shouldGetEnrollmentsWhenUserHasReadAccessToProgramAndNoOrgUnitNorOrgUnitMod assertNotNull(enrollments); assertContainsOnly( - List.of( - enrollmentA.getUid(), - enrollmentB.getUid(), - enrollmentChildA.getUid(), - enrollmentGrandchildA.getUid()), + List.of(enrollmentA.getUid(), enrollmentChildA.getUid(), enrollmentGrandchildA.getUid()), uids(enrollments)); } @@ -518,27 +523,6 @@ void shouldGetEnrollmentsByTrackedEntityWhenUserHasAccessToTrackedEntityType() assertContainsOnly(List.of(enrollmentA.getUid()), uids(enrollments)); } - @Test - void shouldFailGettingEnrollmentsByTrackedEntityWhenUserHasNoAccessToTrackedEntityType() { - programA.getSharing().setPublicAccess(AccessStringHelper.DATA_READ); - manager.updateNoAcl(programA); - - trackedEntityTypeA.getSharing().setOwner(admin); - trackedEntityTypeA.getSharing().setPublicAccess(AccessStringHelper.DEFAULT); - manager.updateNoAcl(trackedEntityTypeA); - - EnrollmentOperationParams params = - EnrollmentOperationParams.builder() - .orgUnitUids(Set.of(trackedEntityA.getOrganisationUnit().getUid())) - .orgUnitMode(SELECTED) - .trackedEntityUid(trackedEntityA.getUid()) - .build(); - - ForbiddenException exception = - assertThrows(ForbiddenException.class, () -> enrollmentService.getEnrollments(params)); - assertContains("access to tracked entity type", exception.getMessage()); - } - @Test void shouldReturnEnrollmentIfEnrollmentWasUpdatedBeforePassedDateAndTime() throws ForbiddenException, BadRequestException { @@ -650,9 +634,8 @@ void shouldReturnEmptyIfEnrollmentEndedBeforePassedDateAndTime() } @Test - void shouldReturnAllEnrollmentsInTheSystemWhenModeAllAndUserAuthorized() + void shouldReturnAllAccessibleEnrollmentsInTheSystemWhenModeAllAndUserAuthorized() throws ForbiddenException, BadRequestException { - injectSecurityContext(authorizedUser); EnrollmentOperationParams operationParams = @@ -660,11 +643,7 @@ void shouldReturnAllEnrollmentsInTheSystemWhenModeAllAndUserAuthorized() List enrollments = enrollmentService.getEnrollments(operationParams); assertContainsOnly( - List.of( - enrollmentA.getUid(), - enrollmentB.getUid(), - enrollmentChildA.getUid(), - enrollmentGrandchildA.getUid()), + List.of(enrollmentA.getUid(), enrollmentChildA.getUid(), enrollmentGrandchildA.getUid()), uids(enrollments)); } @@ -702,7 +681,7 @@ void shouldFailWhenUserCanSearchEverywhereModeDescendantsAndOrgUnitNotInSearchSc @Test void shouldReturnAllEnrollmentsWhenOrgUnitModeAllAndUserAuthorized() throws ForbiddenException, BadRequestException { - injectSecurityContext(admin); + injectSecurityContext(superuser); EnrollmentOperationParams operationParams = EnrollmentOperationParams.builder().orgUnitMode(ALL).build();