From 87e9b9ca60a0728515daa51ea8d6c0d13336bbdc Mon Sep 17 00:00:00 2001 From: Marc Date: Sun, 19 Jan 2025 14:46:14 +0100 Subject: [PATCH 1/2] fix: Grant temp access only if user in search scope [DHIS2-18784] [2.39] --- .../TrackerOwnershipManager.java | 4 +- .../DefaultTrackerOwnershipManager.java | 60 +++++++-- .../TrackerOwnershipManagerTest.java | 118 ++++++++++++++---- .../TrackerOwnershipControllerTest.java | 52 ++++++-- 4 files changed, 186 insertions(+), 48 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/trackedentity/TrackerOwnershipManager.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/trackedentity/TrackerOwnershipManager.java index 00f6119f6b8c..c65565447cfd 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/trackedentity/TrackerOwnershipManager.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/trackedentity/TrackerOwnershipManager.java @@ -27,6 +27,7 @@ */ package org.hisp.dhis.trackedentity; +import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.dxf2.events.event.EventContext; import org.hisp.dhis.organisationunit.OrganisationUnit; import org.hisp.dhis.program.Program; @@ -92,7 +93,8 @@ boolean hasAccessUsingContext( * @param reason The reason for requesting temporary ownership */ void grantTemporaryOwnership( - TrackedEntityInstance entityInstance, Program program, User user, String reason); + TrackedEntityInstance entityInstance, Program program, User user, String reason) + throws IllegalQueryException; /** * Ownership check can be skipped if the user is super user or if the program type is without diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackerOwnershipManager.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackerOwnershipManager.java index f65d4c5d54d4..3d47bc271368 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackerOwnershipManager.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackerOwnershipManager.java @@ -233,22 +233,56 @@ public void assignOwnership( @Transactional public void grantTemporaryOwnership( TrackedEntityInstance entityInstance, Program program, User user, String reason) { - if (canSkipOwnershipCheck(user, program) || entityInstance == null) { - return; + + validateGrantTemporaryOwnershipInputs(entityInstance, program, user); + + if (config.isEnabled(CHANGELOG_TRACKER)) { + programTempOwnershipAuditService.addProgramTempOwnershipAudit( + new ProgramTempOwnershipAudit(program, entityInstance, reason, user.getUsername())); } - if (program.isProtected()) { - if (config.isEnabled(CHANGELOG_TRACKER)) { - programTempOwnershipAuditService.addProgramTempOwnershipAudit( - new ProgramTempOwnershipAudit(program, entityInstance, reason, user.getUsername())); - } - ProgramTempOwner programTempOwner = - new ProgramTempOwner( - program, entityInstance, reason, user, TEMPORARY_OWNERSHIP_VALIDITY_IN_HOURS); - programTempOwnerService.addProgramTempOwner(programTempOwner); - tempOwnerCache.invalidate( - getTempOwnershipCacheKey(entityInstance.getUid(), program.getUid(), user.getUid())); + ProgramTempOwner programTempOwner = + new ProgramTempOwner( + program, entityInstance, reason, user, TEMPORARY_OWNERSHIP_VALIDITY_IN_HOURS); + programTempOwnerService.addProgramTempOwner(programTempOwner); + tempOwnerCache.invalidate( + getTempOwnershipCacheKey(entityInstance.getUid(), program.getUid(), user.getUid())); + } + + private void validateGrantTemporaryOwnershipInputs( + TrackedEntityInstance entityInstance, Program program, User user) { + if (program == null) { + throw new IllegalQueryException( + "Temporary ownership not created. Program supplied does not exist."); + } + + if (user.isSuper()) { + throw new IllegalQueryException( + "Temporary ownership not created. Current user is a superuser."); + } + + if (ProgramType.WITHOUT_REGISTRATION == program.getProgramType()) { + throw new IllegalQueryException( + "Temporary ownership not created. Program supplied is not a tracker program."); + } + + if (!program.isProtected()) { + throw new IllegalQueryException( + String.format( + "Temporary ownership can only be granted to protected programs. %s access level is %s.", + program.getUid(), program.getAccessLevel().name())); } + + if (!isOwnerInUserSearchScope(user, entityInstance, program)) { + throw new IllegalQueryException( + "The owner of the entity-program combination is not in the user's search scope."); + } + } + + private boolean isOwnerInUserSearchScope( + User user, TrackedEntityInstance trackedEntity, Program program) { + return organisationUnitService.isInUserSearchHierarchyCached( + user, getOwner(trackedEntity.getId(), program, trackedEntity::getOrganisationUnit)); } @Override diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java index 877462245248..241c42ac0cfa 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java @@ -27,10 +27,14 @@ */ package org.hisp.dhis.trackedentity; +import static org.hisp.dhis.common.AccessLevel.AUDITED; +import static org.hisp.dhis.common.AccessLevel.CLOSED; +import static org.hisp.dhis.common.AccessLevel.OPEN; import static org.hisp.dhis.common.OrganisationUnitSelectionMode.ACCESSIBLE; import static org.hisp.dhis.security.acl.AccessStringHelper.DEFAULT; import static org.hisp.dhis.security.acl.AccessStringHelper.FULL; import static org.hisp.dhis.user.UserRole.AUTHORITY_ALL; +import static org.hisp.dhis.utils.Assertions.assertContains; import static org.hisp.dhis.utils.Assertions.assertContainsOnly; import static org.hisp.dhis.utils.Assertions.assertIsEmpty; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -43,6 +47,8 @@ import java.util.List; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.ws.rs.ForbiddenException; import org.hisp.dhis.common.AccessLevel; import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.dxf2.events.EnrollmentEventsParams; @@ -56,12 +62,15 @@ import org.hisp.dhis.program.ProgramInstance; import org.hisp.dhis.program.ProgramInstanceService; import org.hisp.dhis.program.ProgramService; +import org.hisp.dhis.program.ProgramType; import org.hisp.dhis.test.integration.IntegrationTestBase; import org.hisp.dhis.user.User; import org.hisp.dhis.user.UserService; import org.hisp.dhis.user.sharing.Sharing; import org.hisp.dhis.user.sharing.UserAccess; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.beans.factory.annotation.Autowired; /** @@ -149,7 +158,7 @@ protected void setUpTest() throws Exception { programA.setSharing(new Sharing(FULL, userAccess)); programService.updateProgram(programA); programB = createProgram('B'); - programB.setAccessLevel(AccessLevel.CLOSED); + programB.setAccessLevel(CLOSED); programB.setTrackedEntityType(trackedEntityType); programService.addProgram(programB); programB.setSharing(new Sharing(DEFAULT, userAccess)); @@ -166,25 +175,20 @@ protected void setUpTest() throws Exception { } @Test - void testAssignOwnership() { + void shouldFailWhenGrantingTemporaryOwnershipAndUserNotInSearchScope() { assertTrue(trackerOwnershipAccessManager.hasAccess(userA, entityInstanceA1, programA)); assertFalse(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceA1, programA)); - assertTrue(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceB1, programA)); - trackerOwnershipAccessManager.assignOwnership( - entityInstanceA1, programA, organisationUnitB, false, true); - assertFalse(trackerOwnershipAccessManager.hasAccess(userA, entityInstanceA1, programA)); - assertTrue(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceA1, programA)); - } - @Test - void testGrantTemporaryOwnershipWithAudit() { - assertTrue(trackerOwnershipAccessManager.hasAccess(userA, entityInstanceA1, programA)); - assertFalse(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceA1, programA)); - trackerOwnershipAccessManager.grantTemporaryOwnership( - entityInstanceA1, programA, userB, "testing reason"); - assertTrue(trackerOwnershipAccessManager.hasAccess(userA, entityInstanceA1, programA)); - assertTrue(trackerOwnershipAccessManager.hasAccess(userA, entityInstanceA1, programA)); - assertTrue(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceA1, programA)); + Exception exception = + assertThrows( + IllegalQueryException.class, + () -> + trackerOwnershipAccessManager.grantTemporaryOwnership( + entityInstanceA1, programA, userB, "testing reason")); + + assertEquals( + "The owner of the entity-program combination is not in the user's search scope.", + exception.getMessage()); } @Test @@ -208,9 +212,13 @@ void shouldHaveAccessWhenProgramProtectedAndUserInCaptureScope() { } @Test - void shouldHaveAccessWhenProgramProtectedAndHasTemporaryAccess() { + void shouldHaveAccessWhenProgramProtectedAndHasTemporaryAccess() throws ForbiddenException { + userB.setTeiSearchOrganisationUnits(Set.of(organisationUnitA)); + userService.updateUser(userB); + trackerOwnershipAccessManager.grantTemporaryOwnership( entityInstanceA1, programA, userB, "test protected program"); + assertTrue(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceA1, programA)); assertTrue( trackerOwnershipAccessManager.hasAccess( @@ -233,14 +241,67 @@ void shouldHaveAccessWhenProgramClosedAndUserInCaptureScope() { userB, entityInstanceB1.getUid(), entityInstanceB1.getOrganisationUnit(), programB)); } + private static Stream providePrograms() { + return Stream.of(createProgram(OPEN), createProgram(AUDITED), createProgram(CLOSED)); + } + + @ParameterizedTest + @MethodSource("providePrograms") + void shouldFailWhenGrantingTemporaryOwnershipToProgramWithAccessLevelOtherThanProtected( + Program program) { + Exception exception = + assertThrows( + IllegalQueryException.class, + () -> + trackerOwnershipAccessManager.grantTemporaryOwnership( + entityInstanceA1, program, userB, "test temporary ownership")); + + assertContains( + "Temporary ownership can only be granted to protected programs.", exception.getMessage()); + } + @Test - void shouldNotHaveAccessWhenProgramClosedAndUserHasTemporaryAccess() { - trackerOwnershipAccessManager.grantTemporaryOwnership( - entityInstanceA1, programB, userB, "test closed program"); - assertFalse(trackerOwnershipAccessManager.hasAccess(userB, entityInstanceA1, programB)); - assertFalse( - trackerOwnershipAccessManager.hasAccess( - userB, entityInstanceA1.getUid(), entityInstanceA1.getOrganisationUnit(), programB)); + void shouldFailWhenGrantingTemporaryAccessIfUserIsSuperuser() { + Exception exception = + assertThrows( + IllegalQueryException.class, + () -> + trackerOwnershipAccessManager.grantTemporaryOwnership( + entityInstanceA1, programA, superUser, "test temporary ownership")); + + assertEquals( + "Temporary ownership not created. Current user is a superuser.", exception.getMessage()); + } + + @Test + void shouldFailWhenGrantingTemporaryAccessIfProgramIsNull() { + Exception exception = + assertThrows( + IllegalQueryException.class, + () -> + trackerOwnershipAccessManager.grantTemporaryOwnership( + entityInstanceA1, null, userB, "test temporary ownership")); + + assertEquals( + "Temporary ownership not created. Program supplied does not exist.", + exception.getMessage()); + } + + @Test + void shouldFailWhenGrantingTemporaryAccessIfProgramIsNotTrackerProgram() { + Program eventProgram = createProgram(AccessLevel.PROTECTED); + eventProgram.setProgramType(ProgramType.WITHOUT_REGISTRATION); + + Exception exception = + assertThrows( + IllegalQueryException.class, + () -> + trackerOwnershipAccessManager.grantTemporaryOwnership( + entityInstanceA1, eventProgram, userB, "test temporary ownership")); + + assertEquals( + "Temporary ownership not created. Program supplied is not a tracker program.", + exception.getMessage()); } @Test @@ -423,4 +484,11 @@ private TrackedEntityInstanceParams createInstanceParams() { false, false); } + + private static Program createProgram(AccessLevel accessLevel) { + Program program = new Program(); + program.setAccessLevel(accessLevel); + + return program; + } } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/TrackerOwnershipControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/TrackerOwnershipControllerTest.java index 3dc15f3d02e5..15f56e95424d 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/TrackerOwnershipControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/TrackerOwnershipControllerTest.java @@ -29,6 +29,9 @@ import static org.hisp.dhis.web.WebClientUtils.assertStatus; +import java.util.Set; +import org.hisp.dhis.organisationunit.OrganisationUnit; +import org.hisp.dhis.user.User; import org.hisp.dhis.web.HttpStatus; import org.hisp.dhis.webapi.DhisControllerConvenienceTest; import org.junit.jupiter.api.BeforeEach; @@ -42,36 +45,66 @@ */ class TrackerOwnershipControllerTest extends DhisControllerConvenienceTest { - private String ouId; + private String orgUnitAUid; + + private String pId; private String teiId; - private String pId; + private User regularUser; @BeforeEach void setUp() { - ouId = + orgUnitAUid = + assertStatus( + HttpStatus.CREATED, + POST( + "/organisationUnits/", + "{'name':'My Unit', 'shortName':'OU1', 'openingDate': '2020-01-01'}")); + String orgUnitBUid = assertStatus( HttpStatus.CREATED, POST( "/organisationUnits/", "{'name':'My Unit', 'shortName':'OU1', 'openingDate': '2020-01-01'}")); - String tetId = assertStatus(HttpStatus.CREATED, POST("/trackedEntityTypes/", "{'name': 'A'}")); + + OrganisationUnit orgUnitA = manager.get(OrganisationUnit.class, orgUnitAUid); + OrganisationUnit orgUnitB = manager.get(OrganisationUnit.class, orgUnitBUid); + + regularUser = + createAndAddUser( + false, "regular-user", Set.of(orgUnitA, orgUnitB), Set.of(orgUnitA, orgUnitB)); + User superuser = + createAndAddUser(true, "superuser", Set.of(orgUnitA, orgUnitB), Set.of(orgUnitA, orgUnitB)); + injectSecurityContext(superuser); + + String tetId = + assertStatus( + HttpStatus.CREATED, + POST( + "/trackedEntityTypes/", + "{'name': 'A', 'sharing':{'external':false,'public':'rwrw----'}}")); teiId = assertStatus( HttpStatus.OK, POST( "/trackedEntityInstances", - "{'name':'A', 'trackedEntityType':'" + tetId + "', 'orgUnit':'" + ouId + "'}")); + "{'name':'A', 'trackedEntityType':'" + + tetId + + "', 'orgUnit':'" + + orgUnitAUid + + "'}")); pId = assertStatus( HttpStatus.CREATED, POST( "/programs/", - "{'name':'P1', 'shortName':'P1', 'programType':'WITHOUT_REGISTRATION', 'organisationUnits': [{'id':'" - + ouId - + "'}]}")); + "{'name':'P1', 'shortName':'P1', 'programType':'WITH_REGISTRATION', 'accessLevel':'PROTECTED', 'trackedEntityType': {'id': '" + + tetId + + "'}, 'organisationUnits': [{'id':'" + + orgUnitAUid + + "'}], 'sharing':{'external':false,'public':'rwrw----'}}")); } @Test @@ -85,12 +118,13 @@ void testUpdateTrackerProgramOwner() { "/tracker/ownership/transfer?trackedEntityInstance={tei}&program={prog}&ou={ou}", teiId, pId, - ouId) + orgUnitAUid) .content(HttpStatus.OK)); } @Test void testOverrideOwnershipAccess() { + injectSecurityContext(regularUser); assertWebMessage( "OK", 200, From 7fe417eb9e1eee80f91ed86dabd4135761d7b249 Mon Sep 17 00:00:00 2001 From: Marc Date: Sun, 19 Jan 2025 14:55:29 +0100 Subject: [PATCH 2/2] fix: Remove unnecessary import [DHIS2-18784] [2.39] --- .../hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java index 241c42ac0cfa..07c325fd6a18 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerOwnershipManagerTest.java @@ -48,7 +48,6 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -import javax.ws.rs.ForbiddenException; import org.hisp.dhis.common.AccessLevel; import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.dxf2.events.EnrollmentEventsParams; @@ -212,7 +211,7 @@ void shouldHaveAccessWhenProgramProtectedAndUserInCaptureScope() { } @Test - void shouldHaveAccessWhenProgramProtectedAndHasTemporaryAccess() throws ForbiddenException { + void shouldHaveAccessWhenProgramProtectedAndHasTemporaryAccess() { userB.setTeiSearchOrganisationUnits(Set.of(organisationUnitA)); userService.updateUser(userB);