Skip to content

Commit

Permalink
fix: Allow granting temporary access only when user in search scope […
Browse files Browse the repository at this point in the history 
…DHIS2-18784] (#19670)

* fix: Allow temp access only when user in search scope [DHIS2-18784]

* fix: Rephrase message in exception [DHIS2-18784]

* fix: Remove unused spring dependency [DHIS2-18784]

* fix: Add dot at the end of the sentence [DHIS2-18784]

* fix: Refactor to improve readability [DHIS2-18784]

* fix: Skip getting TE enrollments [DHIS2-18784]

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

Co-authored-by: Enrico Colasante <[email protected]>

* fix: Use regular user on controller test [DHIS2-18784]

* fix: Fix error message in tests [DHIS2-18784]

* fix: Split param validation [DHIS2-18784]

---------

Co-authored-by: Enrico Colasante <[email protected]>
  • Loading branch information
@muilpp @enricocolasante
muilpp and enricocolasante authored Jan 18, 2025
1 parent a95f669 commit 6e8d1f7
Show file tree
Hide file tree
Showing 10 changed files with 222 additions and 165 deletions.
4 changes: 0 additions & 4 deletions dhis-2/dhis-services/dhis-service-tracker/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
97 changes: 40 additions & 57 deletions ...rvice-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -149,71 +149,54 @@ 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 {
validateGrantTemporaryOwnershipInputs(trackedEntity, program, user);

if (config.isEnabled(CHANGELOG_TRACKER)) {
programTempOwnershipAuditService.addProgramTempOwnershipAudit(
new ProgramTempOwnershipAudit(program, trackedEntity, reason, user.getUsername()));
}

UserDetails currentUser = CurrentUserUtil.getCurrentUserDetails();
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 (hasAccess(currentUser, trackedEntity, program) || skipAccessValidation) {
TrackedEntityProgramOwner teProgramOwner =
trackedEntityProgramOwnerService.getTrackedEntityProgramOwner(trackedEntity, program);
private void validateGrantTemporaryOwnershipInputs(
TrackedEntity trackedEntity, Program program, UserDetails user) throws ForbiddenException {
if (program == null) {
throw new ForbiddenException(
"Temporary ownership not created. Program supplied does not exist.");
}

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);
}
if (user.isSuper()) {
throw new ForbiddenException("Temporary ownership not created. Current user is a superuser.");
}

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 (ProgramType.WITHOUT_REGISTRATION == program.getProgramType()) {
throw new ForbiddenException(
"Temporary ownership not created. Program supplied is not a tracker program.");
}
}

@Override
@Transactional
public void grantTemporaryOwnership(
TrackedEntity trackedEntity, Program program, UserDetails user, String reason) {
if (canSkipOwnershipCheck(user, program) || trackedEntity == null) {
return;
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()));
}

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 (!isOwnerInUserSearchScope(user, trackedEntity, program)) {
throw new ForbiddenException(
"The owner of the entity-program combination is not in the user's search scope.");
}
}

Expand Down
15 changes: 2 additions & 13 deletions ...dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/TrackerOwnershipManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
9 changes: 7 additions & 2 deletions ...-test-integration/src/test/java/org/hisp/dhis/trackedentity/TrackerAccessManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -82,6 +83,8 @@ class TrackerAccessManagerTest extends PostgresIntegrationTestBase {

@Autowired private TrackerOwnershipManager trackerOwnershipManager;

@Autowired private TrackedEntityProgramOwnerService trackedEntityProgramOwnerService;

@Autowired private TrackedEntityTypeService trackedEntityTypeService;

@Autowired private TrackedEntityService trackedEntityService;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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));
Expand Down
Loading

0 comments on commit 6e8d1f7

Please sign in to comment.