Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+189 −160
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5349c0e
fix: Allow temp access only when user in search scope [DHIS2-18784]
muilpp Jan 15, 2025
3ffa1cb
fix: Rephrase message in exception [DHIS2-18784]
muilpp Jan 15, 2025
22ccc8d
Merge remote-tracking branch 'origin/master' into DHIS2-18784
muilpp Jan 15, 2025
7c64434
fix: Remove unused spring dependency [DHIS2-18784]
muilpp Jan 15, 2025
8b43c72
fix: Add dot at the end of the sentence [DHIS2-18784]
muilpp Jan 15, 2025
34b1973
fix: Refactor to improve readability [DHIS2-18784]
muilpp Jan 15, 2025
97cd930
fix: Skip getting TE enrollments [DHIS2-18784]
muilpp Jan 15, 2025
fb78c43
Update dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hi…
muilpp Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 13 additions & 46 deletions ...rvice-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,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 @@ -147,63 +146,26 @@ 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;
}

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");
}
}

@Override
@Transactional
public void grantTemporaryOwnership(
TrackedEntity trackedEntity, Program program, UserDetails user, String reason) {
TrackedEntity trackedEntity, Program program, UserDetails user, String reason)
muilpp marked this conversation as resolved.
Show resolved Hide resolved
throws ForbiddenException {
if (canSkipOwnershipCheck(user, program) || trackedEntity == null) {
muilpp marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (program.isProtected()) {
if (!isOwnerInUserSearchScope(user, trackedEntity, program)) {
throw new ForbiddenException(
"The owner of the entity-program combination is not in the user's search scope");
Copy link
Contributor Author

@muilpp muilpp Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to say TE not found? It could be the TE is just registered and not enrolled, but more often than not, the TE will be enrolled.

Same could apply here

}

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

ProgramTempOwner programTempOwner =
new ProgramTempOwner(
program,
Expand All @@ -214,6 +176,11 @@ public void grantTemporaryOwnership(
programTempOwnerService.addProgramTempOwner(programTempOwner);
tempOwnerCache.invalidate(
getTempOwnershipCacheKey(trackedEntity.getUid(), program.getUid(), user.getUid()));
} else {
muilpp marked this conversation as resolved.
Show resolved Hide resolved
throw new ForbiddenException(
String.format(
"It's only possible to grant temporary ownership to protected programs, and %s access level is %s",
program.getUid(), program.getAccessLevel().name()));
}
}

Expand Down
15 changes: 2 additions & 13 deletions ...dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/TrackerOwnershipManager.java
View file
Open in desktop
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
Open in desktop
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
Loading