fix: Allow granting temporary access only when user in search scope [DHIS2-18784]

dhis-2/dhis-services/dhis-service-tracker/pom.xml

@@ -167,10 +167,6 @@
       <groupId>org.geotools</groupId>
       <artifactId>gt-main</artifactId>
     </dependency>
-    <dependency>
-      <groupId>org.springframework.security</groupId>
-      <artifactId>spring-security-core</artifactId>
-    </dependency>
 
     <!-- Test -->
     <dependency>

dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java

@@ -32,6 +32,7 @@
 import java.util.Optional;
 import java.util.function.LongSupplier;
 import java.util.function.Supplier;
+import javax.annotation.Nonnull;
 import lombok.extern.slf4j.Slf4j;
 import org.hibernate.Hibernate;
 import org.hisp.dhis.cache.Cache;
@@ -53,7 +54,6 @@
 import org.hisp.dhis.user.CurrentUserUtil;
 import org.hisp.dhis.user.UserDetails;
 import org.hisp.dhis.user.UserService;
-import org.springframework.security.access.AccessDeniedException;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
 
@@ -149,72 +149,41 @@ public void transferOwnership(
 
   @Override
   @Transactional
-  public void assignOwnership(
-      TrackedEntity trackedEntity,
-      Program program,
-      OrganisationUnit organisationUnit,
-      boolean skipAccessValidation,
-      boolean overwriteIfExists) {
-    if (trackedEntity == null || program == null || organisationUnit == null) {
-      return;
+  public void grantTemporaryOwnership(
+      @Nonnull TrackedEntity trackedEntity, Program program, UserDetails user, String reason)
+      throws ForbiddenException {
+    if (canSkipOwnershipCheck(user, program)) {
+      throw new ForbiddenException(
+          "Temporary ownership not created. Either current user is a superuser, program supplied does not exist or program supplied is not a tracker program.");
     }
 
-    UserDetails currentUser = CurrentUserUtil.getCurrentUserDetails();
-
-    if (hasAccess(currentUser, trackedEntity, program) || skipAccessValidation) {
-      TrackedEntityProgramOwner teProgramOwner =
-          trackedEntityProgramOwnerService.getTrackedEntityProgramOwner(trackedEntity, program);
-
-      if (teProgramOwner != null) {
-        if (overwriteIfExists && !teProgramOwner.getOrganisationUnit().equals(organisationUnit)) {
-          ProgramOwnershipHistory programOwnershipHistory =
-              new ProgramOwnershipHistory(
-                  program,
-                  trackedEntity,
-                  teProgramOwner.getOrganisationUnit(),
-                  teProgramOwner.getLastUpdated(),
-                  teProgramOwner.getCreatedBy());
-          programOwnershipHistoryService.addProgramOwnershipHistory(programOwnershipHistory);
-          trackedEntityProgramOwnerService.updateTrackedEntityProgramOwner(
-              trackedEntity, program, organisationUnit);
-        }
-      } else {
-        trackedEntityProgramOwnerService.createTrackedEntityProgramOwner(
-            trackedEntity, program, organisationUnit);
-      }
-
-      ownerCache.invalidate(getOwnershipCacheKey(trackedEntity::getId, program));
-    } else {
-      log.error("Unauthorized attempt to assign ownership");
-      throw new AccessDeniedException(
-          "User does not have access to assign ownership for the entity-program combination");
+    if (!program.isProtected()) {
+      throw new ForbiddenException(
+          String.format(
+              "Temporary ownership can only be granted to protected programs. %s access level is %s.",
+              program.getUid(), program.getAccessLevel().name()));
     }
-  }
 
-  @Override
-  @Transactional
-  public void grantTemporaryOwnership(
-      TrackedEntity trackedEntity, Program program, UserDetails user, String reason) {
-    if (canSkipOwnershipCheck(user, program) || trackedEntity == null) {
-      return;
+    if (!isOwnerInUserSearchScope(user, trackedEntity, program)) {
+      throw new ForbiddenException(
+          "The owner of the entity-program combination is not in the user's search scope.");
     }
 
-    if (program.isProtected()) {
-      if (config.isEnabled(CHANGELOG_TRACKER)) {
-        programTempOwnershipAuditService.addProgramTempOwnershipAudit(
-            new ProgramTempOwnershipAudit(program, trackedEntity, reason, user.getUsername()));
-      }
-      ProgramTempOwner programTempOwner =
-          new ProgramTempOwner(
-              program,
-              trackedEntity,
-              reason,
-              userService.getUser(user.getUid()),
-              TEMPORARY_OWNERSHIP_VALIDITY_IN_HOURS);
-      programTempOwnerService.addProgramTempOwner(programTempOwner);
-      tempOwnerCache.invalidate(
-          getTempOwnershipCacheKey(trackedEntity.getUid(), program.getUid(), user.getUid()));
+    if (config.isEnabled(CHANGELOG_TRACKER)) {
+      programTempOwnershipAuditService.addProgramTempOwnershipAudit(
+          new ProgramTempOwnershipAudit(program, trackedEntity, reason, user.getUsername()));
     }
+
+    ProgramTempOwner programTempOwner =
+        new ProgramTempOwner(
+            program,
+            trackedEntity,
+            reason,
+            userService.getUser(user.getUid()),
+            TEMPORARY_OWNERSHIP_VALIDITY_IN_HOURS);
+    programTempOwnerService.addProgramTempOwner(programTempOwner);
+    tempOwnerCache.invalidate(
+        getTempOwnershipCacheKey(trackedEntity.getUid(), program.getUid(), user.getUid()));
   }
 
   @Override

dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/TrackerOwnershipManager.java

@@ -52,18 +52,6 @@ public interface TrackerOwnershipManager {
   void transferOwnership(TrackedEntity trackedEntity, Program program, OrganisationUnit orgUnit)
       throws ForbiddenException;
 
-  /**
-   * @param trackedEntity The tracked entity object
-   * @param program The program object
-   * @param organisationUnit The org unit that has to become the owner
-   */
-  void assignOwnership(
-      TrackedEntity trackedEntity,
-      Program program,
-      OrganisationUnit organisationUnit,
-      boolean skipAccessValidation,
-      boolean overwriteIfExists);
-
   /**
    * Check whether the user has access (as owner or has temporarily broken the glass) to the tracked
    * entity - program combination.
@@ -87,7 +75,8 @@ boolean hasAccess(
    * @param reason The reason for requesting temporary ownership
    */
   void grantTemporaryOwnership(
-      TrackedEntity trackedEntity, Program program, UserDetails user, String reason);
+      TrackedEntity trackedEntity, Program program, UserDetails user, String reason)
+      throws ForbiddenException;
 
   /**
    * Ownership check can be skipped if the user is superuser or if the program type is without

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerAccessManagerTest.java

@@ -62,6 +62,7 @@
 import org.hisp.dhis.program.ProgramType;
 import org.hisp.dhis.security.acl.AccessStringHelper;
 import org.hisp.dhis.test.integration.PostgresIntegrationTestBase;
+import org.hisp.dhis.tracker.acl.TrackedEntityProgramOwnerService;
 import org.hisp.dhis.tracker.acl.TrackerAccessManager;
 import org.hisp.dhis.tracker.acl.TrackerOwnershipManager;
 import org.hisp.dhis.tracker.export.trackedentity.TrackedEntityService;
@@ -82,6 +83,8 @@ class TrackerAccessManagerTest extends PostgresIntegrationTestBase {
 
   @Autowired private TrackerOwnershipManager trackerOwnershipManager;
 
+  @Autowired private TrackedEntityProgramOwnerService trackedEntityProgramOwnerService;
+
   @Autowired private TrackedEntityTypeService trackedEntityTypeService;
 
   @Autowired private TrackedEntityService trackedEntityService;
@@ -271,7 +274,8 @@ void checkAccessPermissionForEnrollmentInClosedProgram() throws ForbiddenExcepti
         "User has no create access to organisation unit:");
     enrollment.setOrganisationUnit(orgUnitA);
     // Transferring ownership to orgUnitB. user is no longer owner
-    trackerOwnershipManager.assignOwnership(trackedEntity, programA, orgUnitA, false, true);
+    trackedEntityProgramOwnerService.createTrackedEntityProgramOwner(
+        trackedEntity, programA, orgUnitA);
     trackerOwnershipManager.transferOwnership(trackedEntity, programA, orgUnitB);
     // Cannot create enrollment if not owner
     assertHasError(
@@ -388,7 +392,8 @@ void checkAccessPermissionsForEventInClosedProgram() throws ForbiddenException {
     assertNoErrors(trackerAccessManager.canUpdate(userDetails, eventB, false));
     // Can delete events if user is owner irrespective of eventOU
     assertNoErrors(trackerAccessManager.canDelete(userDetails, eventB, false));
-    trackerOwnershipManager.assignOwnership(trackedEntityA, programA, orgUnitA, false, true);
+    trackedEntityProgramOwnerService.createTrackedEntityProgramOwner(
+        trackedEntityA, programA, orgUnitA);
     trackerOwnershipManager.transferOwnership(trackedEntityA, programA, orgUnitB);
     // Cannot create events anywhere if user is not owner
     assertHasErrors(2, trackerAccessManager.canCreate(userDetails, eventB, false));