Skip to content

fix: [DHIS2-18551] (2.39) Fix access check when registering TEI #19372

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
ameenhere merged 4 commits into from
Dec 4, 2024
Merged

fix: [DHIS2-18551] (2.39) Fix access check when registering TEI #19372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee1bfaf
fix: [DHIS2-18551] (2.39) Fix access check when registering TEI
ameenhere Dec 3, 2024
54b5ad5
Apply spotless
ameenhere Dec 3, 2024
c625100
fix: Test setup of orgUnit Capture
ameenhere Dec 3, 2024
7c31895
fix: Review comments
ameenhere Dec 3, 2024
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
2 changes: 2 additions & 0 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/trackedentity/TrackerAccessManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public interface TrackerAccessManager {

List<String> canWrite(User user, TrackedEntityInstance trackedEntityInstance);

List<String> canCreate(User user, TrackedEntityInstance trackedEntity);

List<String> canRead(
User user,
TrackedEntityInstance trackedEntityInstance,
Expand Down
29 changes: 29 additions & 0 deletions ...s-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackerAccessManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,35 @@ public List<String> canWrite(User user, TrackedEntityInstance trackedEntity) {
return canWrite(user, trackedEntity, programService.getAllPrograms());
}

/**
* Check if the user can create the TE. For a regular user, they must have data write permissions
* to tracked entity type metadata as well as Capture Access to the Org Unit.
*
* @return No errors if a user has write access to the TrackedEntityType and to the OrgUnit
*/
@Override
@Transactional(readOnly = true)
public List<String> canCreate(User user, TrackedEntityInstance trackedEntity) {
if (user == null || user.isSuper() || trackedEntity == null) {
Copy link
Contributor

@muilpp muilpp Dec 3, 2024

Choose a reason for hiding this comment

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

If I remember correctly, user was null when running internal jobs, but after the last refactor by the platform team, that's no longer the case

Copy link
Contributor

@muilpp muilpp Dec 3, 2024

Choose a reason for hiding this comment

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

ah wait, this is for 2.39, so nevermind, probably good to have the null check

Copy link
Contributor Author

@ameenhere ameenhere Dec 3, 2024

Choose a reason for hiding this comment

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

This PR is for 2.39, and I also see other methods in the class doing the same user == null check.
Unsure if the refactor you talk about exists in 2.39.

Copy link
Contributor Author

@ameenhere ameenhere Dec 3, 2024

Choose a reason for hiding this comment

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

Ah for some reason, your second comment just got in. Yes Agreed :D

Copy link
Contributor

@teleivo teleivo Dec 3, 2024

Choose a reason for hiding this comment

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

That a null user means canCreate == true does not feel right. If we assume that the user must be authenticated when this is called we should not check for null. Better an NPE than unauthorized access

Copy link
Contributor

@muilpp muilpp Dec 4, 2024

Choose a reason for hiding this comment

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

It doesn't feel right, but there's a job in previous versions run by a null user, which is why we need to skip this validation. However, it might be worth checking if that job needs to create a TE.

Copy link
Contributor Author

@ameenhere ameenhere Dec 4, 2024

Choose a reason for hiding this comment

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

I agree it does not feel right. Which is why in later versions, there was some work done to fix the user to never being null.
But in 2.39, I feel this may be safer. To follow the same approach as is done for canUpdate , canDelete, and canRead.

return List.of();
}
List<String> errors = new ArrayList<>();
if (!aclService.canDataWrite(user, trackedEntity.getTrackedEntityType())) {
errors.add(
"User has no data write access to tracked entity type: "
+ trackedEntity.getTrackedEntityType().getUid());
}

if (trackedEntity.getOrganisationUnit() != null
&& !organisationUnitService.isInUserHierarchy(user, trackedEntity.getOrganisationUnit())) {
errors.add(
"User has no write access to organisation unit: "
+ trackedEntity.getOrganisationUnit().getUid());
}

return errors;
}

private List<String> canWrite(
User user, TrackedEntityInstance trackedEntity, List<Program> programs) {

Expand Down
3 changes: 2 additions & 1 deletion ...in/java/org/hisp/dhis/dxf2/events/trackedentity/AbstractTrackedEntityInstanceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,8 @@ private ImportSummary addTrackedEntityInstance(
return importSummary;
}

List<String> errors = trackerAccessManager.canWrite(importOptions.getUser(), daoEntityInstance);
List<String> errors =
trackerAccessManager.canCreate(importOptions.getUser(), daoEntityInstance);

if (!errors.isEmpty()) {
return new ImportSummary(ImportStatus.ERROR, errors.toString()).incrementIgnored();
Expand Down
50 changes: 49 additions & 1 deletion ...integration/src/test/java/org/hisp/dhis/dxf2/events/TrackedEntityInstanceServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
import org.hisp.dhis.trackedentityattributevalue.TrackedEntityAttributeValueService;
import org.hisp.dhis.user.User;
import org.hisp.dhis.user.UserService;
import org.hisp.dhis.user.sharing.Sharing;
import org.joda.time.DateTime;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -593,16 +594,63 @@ void testUpdateTeiByDeletingExistingEventAndAddNewEventForSameProgramStage() {
}

@Test
void testSavePerson() {
void shouldPassWhenRegisteringPerson() {
trackedEntityType.setSharing(Sharing.builder().publicAccess("rwrw----").build());
regularUser.setOrganisationUnits(Set.of(organisationUnitA));
injectSecurityContext(regularUser);
TrackedEntityInstance trackedEntityInstance = new TrackedEntityInstance();
trackedEntityInstance.setTrackedEntityInstance(CodeGenerator.generateUid());
trackedEntityInstance.setOrgUnit(organisationUnitA.getUid());
trackedEntityInstance.setTrackedEntityType(trackedEntityType.getUid());

ImportSummary importSummary =
trackedEntityInstanceService.addTrackedEntityInstance(trackedEntityInstance, null);

assertEquals(ImportStatus.SUCCESS, importSummary.getStatus());
}

@Test
void shouldFailWhenRegisteringPersonOutsideCaptureScope() {
trackedEntityType.setSharing(Sharing.builder().publicAccess("rwrw----").build());
regularUser.setOrganisationUnits(Set.of(organisationUnitB));
injectSecurityContext(regularUser);
TrackedEntityInstance trackedEntityInstance = new TrackedEntityInstance();
trackedEntityInstance.setTrackedEntityInstance(CodeGenerator.generateUid());
trackedEntityInstance.setOrgUnit(organisationUnitA.getUid());
trackedEntityInstance.setTrackedEntityType(trackedEntityType.getUid());

ImportSummary importSummary =
trackedEntityInstanceService.addTrackedEntityInstance(trackedEntityInstance, null);

assertEquals(ImportStatus.ERROR, importSummary.getStatus());
assertEquals(1, importSummary.getImportCount().getIgnored());
assertEquals(
String.format(
"[User has no write access to organisation unit: %s]", organisationUnitA.getUid()),
importSummary.getDescription());
}

@Test
void shouldFailWhenRegisteringPersonWithoutTETypeAccess() {
regularUser.setOrganisationUnits(Set.of(organisationUnitB));
injectSecurityContext(regularUser);
TrackedEntityInstance trackedEntityInstance = new TrackedEntityInstance();
trackedEntityInstance.setTrackedEntityInstance(CodeGenerator.generateUid());
trackedEntityInstance.setOrgUnit(organisationUnitB.getUid());
trackedEntityInstance.setTrackedEntityType(trackedEntityType.getUid());

ImportSummary importSummary =
trackedEntityInstanceService.addTrackedEntityInstance(trackedEntityInstance, null);

assertEquals(ImportStatus.ERROR, importSummary.getStatus());
assertEquals(1, importSummary.getImportCount().getIgnored());
assertEquals(
String.format(
"[User has no data write access to tracked entity type: %s]",
trackedEntityType.getUid()),
importSummary.getDescription());
}

@Test
void testDeletePerson() {
trackedEntityInstanceService.deleteTrackedEntityInstance(maleA.getUid());
Expand Down