fix: Grant temp access only if user in search scope [DHIS2-18784] [2.39]

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

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

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,7 @@
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import org.hisp.dhis.common.AccessLevel;
 import org.hisp.dhis.common.IllegalQueryException;
 import org.hisp.dhis.dxf2.events.EnrollmentEventsParams;
@@ -56,12 +61,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 +157,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 +174,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
@@ -209,8 +212,12 @@ void shouldHaveAccessWhenProgramProtectedAndUserInCaptureScope() {
 
   @Test
   void shouldHaveAccessWhenProgramProtectedAndHasTemporaryAccess() {
+    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 +240,67 @@ void shouldHaveAccessWhenProgramClosedAndUserInCaptureScope() {
             userB, entityInstanceB1.getUid(), entityInstanceB1.getOrganisationUnit(), programB));
   }
 
+  private static Stream<Program> 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 +483,11 @@ private TrackedEntityInstanceParams createInstanceParams() {
         false,
         false);
   }
+
+  private static Program createProgram(AccessLevel accessLevel) {
+    Program program = new Program();
+    program.setAccessLevel(accessLevel);
+
+    return program;
+  }
 }

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,