Skip to content

chore: Refactor ACL validation of single events [DHIS2-20158] #23752

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

Merged
muilpp merged 5 commits into from
May 11, 2026
Merged

chore: Refactor ACL validation of single events [DHIS2-20158] #23752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1df985e
chore: Refactor ACL validation of single events [DHIS2-20158]
muilpp Apr 27, 2026
ba9c648
chore: Reuse user scope validation methods [DHIS2-20158]
muilpp Apr 28, 2026
a947c40
chore: Validate single event AOC on update and delete [DHIS2-20158]
muilpp May 6, 2026
d1685a0
chore: Validate both AOC on single event update [DHIS2-20158]
muilpp May 6, 2026
710a052
chore: Validate both AOC on single event update [DHIS2-20158]
muilpp May 6, 2026
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
108 changes: 108 additions & 0 deletions ...is-test-integration/src/test/java/org/hisp/dhis/tracker/acl/TrackerAccessManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
package org.hisp.dhis.tracker.acl;

import static org.hisp.dhis.security.acl.AccessStringHelper.CATEGORY_NO_DATA_SHARING_DEFAULT;
import static org.hisp.dhis.security.acl.AccessStringHelper.CATEGORY_OPTION_DEFAULT;
import static org.hisp.dhis.test.utils.Assertions.assertIsEmpty;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1000;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1099;
Expand All @@ -51,6 +52,8 @@
import java.util.Set;
import java.util.function.BiFunction;
import org.apache.commons.lang3.time.DateUtils;
import org.hisp.dhis.category.Category;
import org.hisp.dhis.category.CategoryCombo;
import org.hisp.dhis.category.CategoryOption;
import org.hisp.dhis.category.CategoryOptionCombo;
import org.hisp.dhis.category.CategoryService;
Expand All @@ -75,6 +78,7 @@
import org.hisp.dhis.tracker.export.trackedentity.TrackedEntityService;
import org.hisp.dhis.tracker.imports.validation.ValidationCode;
import org.hisp.dhis.tracker.model.Enrollment;
import org.hisp.dhis.tracker.model.SingleEvent;
import org.hisp.dhis.tracker.model.TrackedEntity;
import org.hisp.dhis.tracker.model.TrackerEvent;
import org.hisp.dhis.user.User;
Expand Down Expand Up @@ -122,6 +126,16 @@ class TrackerAccessManagerTest extends PostgresIntegrationTestBase {

private TrackerEvent eventB;

private SingleEvent singleEvent;

private CategoryOption oldCatOption;

private CategoryOption newCatOption;

private CategoryOptionCombo oldAoc;

private CategoryOptionCombo newAoc;

@BeforeEach
void setUp() {
CategoryOptionCombo coA = categoryService.getDefaultCategoryOptionCombo();
Expand Down Expand Up @@ -203,6 +217,51 @@ void setUp() {
eventB.setAttributeOptionCombo(coA);
manager.save(eventB, false);

oldCatOption = createCategoryOption('M');
categoryService.addCategoryOption(oldCatOption);
oldCatOption.getSharing().setPublicAccess(CATEGORY_OPTION_DEFAULT);
categoryService.updateCategoryOption(oldCatOption);

newCatOption = createCategoryOption('N');
categoryService.addCategoryOption(newCatOption);
newCatOption.getSharing().setPublicAccess(CATEGORY_OPTION_DEFAULT);
categoryService.updateCategoryOption(newCatOption);

Category singleEventCategory = createCategory('M', oldCatOption, newCatOption);
categoryService.addCategory(singleEventCategory);

CategoryCombo singleEventCategoryCombo = createCategoryCombo('M', singleEventCategory);
categoryService.addCategoryCombo(singleEventCategoryCombo);

oldAoc = createCategoryOptionCombo(singleEventCategoryCombo, oldCatOption);
categoryService.addCategoryOptionCombo(oldAoc);
oldCatOption.getCategoryOptionCombos().add(oldAoc);
categoryService.updateCategoryOption(oldCatOption);

newAoc = createCategoryOptionCombo(singleEventCategoryCombo, newCatOption);
categoryService.addCategoryOptionCombo(newAoc);
newCatOption.getCategoryOptionCombos().add(newAoc);
categoryService.updateCategoryOption(newCatOption);

ProgramStage singleEventProgramStage = createProgramStage('C', 0);
manager.save(singleEventProgramStage);

Program singleEventProgram = createProgram('C', new HashSet<>(), orgUnitA);
singleEventProgram.setProgramType(ProgramType.WITHOUT_REGISTRATION);
singleEventProgramStage.setProgram(singleEventProgram);
singleEventProgram.getProgramStages().add(singleEventProgramStage);
manager.save(singleEventProgram);
singleEventProgram.setPublicAccess(AccessStringHelper.DATA_READ_WRITE);
manager.update(singleEventProgram);
manager.update(singleEventProgramStage);

singleEvent = new SingleEvent();
singleEvent.setProgramStage(singleEventProgramStage);
singleEvent.setOrganisationUnit(orgUnitA);
singleEvent.setAttributeOptionCombo(oldAoc);
singleEvent.setOccurredDate(new Date());
manager.save(singleEvent);

User adminUser = getAdminUser();
adminUser.setTeiSearchOrganisationUnits(Set.of(orgUnitA, orgUnitB));
adminUser.setOrganisationUnits(Set.of(orgUnitA));
Expand Down Expand Up @@ -511,6 +570,55 @@ void shouldFailToDeleteEnrollmentWhenUserLacksAccess() {
(userDetails, enrollment) -> trackerAccessManager.canDelete(userDetails, enrollment));
}

@Test
void shouldPassWhenUpdatingSingleEventWithAccessToBothOldAndNewAoc() {
User user = createUserWithAuth("user1").setOrganisationUnits(Sets.newHashSet(orgUnitA));

assertNoErrors(trackerAccessManager.canUpdate(fromUser(user), singleEvent, orgUnitA, newAoc));
}

@Test
void shouldFailWhenUpdatingSingleEventWithAccessOnlyToNewAoc() {
oldCatOption.getSharing().setPublicAccess(CATEGORY_NO_DATA_SHARING_DEFAULT);
manager.update(oldCatOption);

User user = createUserWithAuth("user1").setOrganisationUnits(Sets.newHashSet(orgUnitA));

assertHasErrorMessage(
trackerAccessManager.canUpdate(fromUser(user), singleEvent, orgUnitA, newAoc), E1099);
}

@Test
void shouldFailWhenUpdatingSingleEventWithAccessOnlyToOldAoc() {
newCatOption.getSharing().setPublicAccess(CATEGORY_NO_DATA_SHARING_DEFAULT);
manager.update(newCatOption);

User user = createUserWithAuth("user1").setOrganisationUnits(Sets.newHashSet(orgUnitA));

assertHasErrorMessage(
trackerAccessManager.canUpdate(fromUser(user), singleEvent, orgUnitA, newAoc), E1099);
}

@Test
void shouldPassWhenUpdatingSingleEventWithNoAocChangeAndUserHasAccess() {
User user = createUserWithAuth("user1").setOrganisationUnits(Sets.newHashSet(orgUnitA));

assertNoErrors(trackerAccessManager.canUpdate(fromUser(user), singleEvent, orgUnitA, oldAoc));
}

@Test
void shouldFailWhenUpdatingSingleEventWithNoAccessToEitherAoc() {
oldCatOption.getSharing().setPublicAccess(CATEGORY_NO_DATA_SHARING_DEFAULT);
manager.update(oldCatOption);
newCatOption.getSharing().setPublicAccess(CATEGORY_NO_DATA_SHARING_DEFAULT);
manager.update(newCatOption);

User user = createUserWithAuth("user1").setOrganisationUnits(Sets.newHashSet(orgUnitA));

assertHasErrors(
2, trackerAccessManager.canUpdate(fromUser(user), singleEvent, orgUnitA, newAoc));
}

private void assertEnrollmentCategoryOptionAccessFails(
BiFunction<UserDetails, Enrollment, List<ErrorMessage>> accessCheck) {
TrackedEntity trackedEntity = manager.get(TrackedEntity.class, trackedEntityA.getUid());
Expand Down
155 changes: 65 additions & 90 deletions dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerAccessManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
*/
package org.hisp.dhis.tracker.acl;

import static org.hisp.dhis.tracker.acl.TrackerOwnershipManager.NO_READ_ACCESS_TO_ORG_UNIT;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1000;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1096;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1097;
Expand All @@ -46,7 +45,6 @@
import lombok.RequiredArgsConstructor;
import org.hisp.dhis.category.CategoryOption;
import org.hisp.dhis.category.CategoryOptionCombo;
import org.hisp.dhis.dataelement.DataElement;
import org.hisp.dhis.organisationunit.OrganisationUnit;
import org.hisp.dhis.program.Program;
import org.hisp.dhis.program.ProgramStage;
Expand Down Expand Up @@ -319,57 +317,73 @@ private List<ErrorMessage> validateTrackerEventAccess(
}

@Override
public List<String> canRead(@Nonnull UserDetails user, SingleEvent event) {
if (user.isSuper() || event == null) {
public List<ErrorMessage> canRead(@Nonnull UserDetails user, @Nonnull SingleEvent event) {
if (user.isSuper()) {
return List.of();
}
ProgramStage programStage = event.getProgramStage();

ProgramStage programStage = event.getProgramStage();
Program program = programStage.getProgram();
List<String> errors = new ArrayList<>();
if (!aclService.canDataRead(user, program)) {
errors.add("User has no data read access to program: " + program.getUid());
}
OrganisationUnit ou = event.getOrganisationUnit();
if (!canAccess(user, program, ou)) {
errors.add(NO_READ_ACCESS_TO_ORG_UNIT + ": " + ou.getUid());
}
errors.addAll(canRead(user, event.getAttributeOptionCombo()));

List<ErrorMessage> errors = new ArrayList<>();
checkOrgUnitInScope(
errors, user, event.getProgramStage().getProgram(), event.getOrganisationUnit());
checkDataReadAccessToProgram(errors, user, program);
checkDataReadAccessToCategoryOptionCombo(errors, user, event.getAttributeOptionCombo());

return errors;
}

@Override
public List<String> canRead(
@Nonnull UserDetails user, SingleEvent event, DataElement dataElement) {
public List<ErrorMessage> canCreate(@Nonnull UserDetails user, @Nonnull SingleEvent event) {
if (user.isSuper()) {
return List.of();
}

List<String> errors = new ArrayList<>(canRead(user, event));
return new ArrayList<>(validateSingleEventAccess(user, event));
}

@Override
public List<ErrorMessage> canUpdate(
@Nonnull UserDetails user,
@Nonnull SingleEvent event,
@Nonnull OrganisationUnit orgUnit,
@Nonnull CategoryOptionCombo categoryOptionCombo) {
if (user.isSuper()) {
return List.of();
}

List<ErrorMessage> errors = new ArrayList<>(validateSingleEventAccess(user, event));
if (!orgUnit.getUid().equals(event.getOrganisationUnit().getUid())) {
checkOrgUnitInCaptureScope(errors, user, orgUnit);
}

if (!aclService.canRead(user, dataElement)) {
errors.add("User has no read access to data element: " + dataElement.getUid());
if (!categoryOptionCombo.getUid().equals(event.getAttributeOptionCombo().getUid())) {
checkDataWriteAccessToCategoryOptionCombo(errors, user, categoryOptionCombo);
}

return errors;
}

@Override
public List<String> canCreate(@Nonnull UserDetails user, SingleEvent event) {
if (user.isSuper() || event == null) {
public List<ErrorMessage> canDelete(@Nonnull UserDetails user, @Nonnull SingleEvent event) {
if (user.isSuper()) {
return List.of();
}

ProgramStage programStage = event.getProgramStage();
return canCreate(user, event);
}

Program program = programStage.getProgram();
List<String> errors = new ArrayList<>();
if (!aclService.canDataWrite(user, program)) {
errors.add("User has no data write access to program: " + program.getUid());
private List<ErrorMessage> validateSingleEventAccess(
@Nonnull UserDetails user, @Nonnull SingleEvent event) {
if (user.isSuper()) {
return List.of();
}

errors.addAll(canWrite(user, event.getAttributeOptionCombo()));
List<ErrorMessage> errors = new ArrayList<>();
checkOrgUnitInCaptureScope(errors, user, event.getOrganisationUnit());
checkDataWriteAccessToProgram(errors, user, event.getProgramStage().getProgram());
checkDataWriteAccessToCategoryOptionCombo(errors, user, event.getAttributeOptionCombo());

return errors;
}
Expand Down Expand Up @@ -426,32 +440,6 @@ private List<String> canWriteRelationship(UserDetails user, Relationship relatio
return errors;
}

/**
* Checks if the user has access to organisation unit under defined tracker program protection
* level
*
* @param user the user to check access for
* @param program program to check against protection level
* @param orgUnit the org unit to be checked under user's scope and program protection
* @return true if the user has access to the org unit under the mentioned program context,
* otherwise return false
*/
boolean canAccess(@Nonnull UserDetails user, Program program, OrganisationUnit orgUnit) {
if (orgUnit == null) {
return false;
}

if (user.isSuper()) {
return true;
}

if (program != null && (program.isClosed() || program.isProtected())) {
return user.isInUserHierarchy(orgUnit.getStoredPath());
}

return user.isInUserEffectiveSearchOrgUnitHierarchy(orgUnit.getStoredPath());
}

private List<String> canRead(@Nonnull UserDetails user, RelationshipItem item) {
if (item.getTrackedEntity() != null)
return canRead(user, item.getTrackedEntity()).stream()
Expand All @@ -465,7 +453,10 @@ private List<String> canRead(@Nonnull UserDetails user, RelationshipItem item) {
return canRead(user, item.getTrackerEvent()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
if (item.getSingleEvent() != null) return canRead(user, item.getSingleEvent());
if (item.getSingleEvent() != null)
return canRead(user, item.getSingleEvent()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
return List.of();
}

Expand All @@ -488,7 +479,10 @@ private List<String> canWrite(@Nonnull UserDetails user, RelationshipItem item)
.map(em -> em.validationCode().getMessage())
.toList();
}
if (item.getSingleEvent() != null) return canCreate(user, item.getSingleEvent());
if (item.getSingleEvent() != null)
return canCreate(user, item.getSingleEvent()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
return List.of();
}

Expand Down Expand Up @@ -573,23 +567,6 @@ private void checkDataReadAccessToCategoryOptionCombo(
}
}

// TODO(tracker) Remove this method and use #checkDataReadAccessToCategoryOptionCombo when
// refactoring single events
private List<String> canRead(@Nonnull UserDetails user, CategoryOptionCombo categoryOptionCombo) {
if (user.isSuper() || categoryOptionCombo == null) {
return List.of();
}

List<String> errors = new ArrayList<>();
for (CategoryOption categoryOption : categoryOptionCombo.getCategoryOptions()) {
if (!aclService.canDataRead(user, categoryOption)) {
errors.add("User has no read access to category option: " + categoryOption.getUid());
}
}

return errors;
}

private void checkDataWriteAccessToCategoryOptionCombo(
List<ErrorMessage> errors, UserDetails user, CategoryOptionCombo categoryOptionCombo) {
if (categoryOptionCombo == null) {
Expand All @@ -605,24 +582,6 @@ private void checkDataWriteAccessToCategoryOptionCombo(
}
}

// TODO(tracker) Remove this method and use #checkDataWriteAccessToCategoryOptionCombo when
// refactoring single events
private List<String> canWrite(
@Nonnull UserDetails user, CategoryOptionCombo categoryOptionCombo) {
if (user.isSuper() || categoryOptionCombo == null) {
return List.of();
}

List<String> errors = new ArrayList<>();
for (CategoryOption categoryOption : categoryOptionCombo.getCategoryOptions()) {
if (!aclService.canDataWrite(user, categoryOption)) {
errors.add("User has no write access to category option: " + categoryOption.getUid());
}
}

return errors;
}

private void checkOwnershipAccess(
List<ErrorMessage> errors, UserDetails user, TrackedEntity trackedEntity, Program program) {
if (!ownershipAccessManager.hasAccess(user, trackedEntity, program)) {
Expand All @@ -648,6 +607,22 @@ private void checkOrgUnitInCaptureScope(
}
}

private void checkOrgUnitInScope(
List<ErrorMessage> errors,
UserDetails user,
@Nonnull Program program,
@Nonnull OrganisationUnit orgUnit) {
if (user.isSuper()) {
return;
}

if (program.isClosed() || program.isProtected()) {
checkOrgUnitInCaptureScope(errors, user, orgUnit);
} else {
checkOrgUnitInSearchScope(errors, user, orgUnit);
}
}

private void checkTrackedEntityProgramAccess(
List<ErrorMessage> errors,
UserDetails user,
Expand Down
Loading
Loading