chore: Validate tracker events on tracker access manager [DHIS2-20158]#23508
chore: Validate tracker events on tracker access manager [DHIS2-20158]#23508
Conversation
.../dhis-test-integration/src/test/java/org/hisp/dhis/tracker/acl/TrackerAccessManagerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
These changes in the ValidationCode (some codes were removed, others were added) are the result of a bug fixing or they are a breaking change?
There was a problem hiding this comment.
I'm improving the readability, so I guess they would fall into the bug category.
This should not affect frontend nor Android, as they don't rely on the error messages I changed, they rely on the status code instead.
There was a problem hiding this comment.
Anyway, I think we need to make them traceable for other clients.
It would be good if we mention these changes in the release-notes somehow. If any external app is relying on those codes, they should have an easy way to fix their app
| List<ErrorMessage> errors = new ArrayList<>(); | ||
| UserDetails user, | ||
| @Nonnull TrackedEntity trackedEntity, | ||
| @CheckForNull OrganisationUnit payloadTrackedEntityOrgUnit) { |
There was a problem hiding this comment.
For me this signature is confusing, I would either:
- Overload this method with a version without the orgUnit
- Make the orgUnit @nonnull. Then the client needs to always set an orgUnit, if there are no changes, it will use the orgUnit from the
teitself
There was a problem hiding this comment.
I don't want to overload the method, as then it's easy to use the wrong one.
I'll make it not null and use the TE org unit, even in the relationship validator.
dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerAccessManager.java
Outdated
Show resolved
Hide resolved
dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerAccessManager.java
Outdated
Show resolved
Hide resolved
| List<String> canRead(UserDetails user, SingleEvent event); | ||
|
|
||
| List<String> canDelete(UserDetails user, TrackerEvent event); | ||
| List<String> canCreate(UserDetails user, SingleEvent event); | ||
|
|
||
| List<String> canRead(UserDetails user, Relationship relationship); | ||
|
|
||
| List<String> canCreate(UserDetails user, Relationship relationship); | ||
|
|
||
| List<String> canDelete(UserDetails user, @Nonnull Relationship relationship); |
There was a problem hiding this comment.
Are we changing single events and relationships in a later PR?
There was a problem hiding this comment.
yes, I'll do it in different PRs, if not it's a very long PR
dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerAccessManager.java
Outdated
Show resolved
Hide resolved
|
3 participants
Centralizes all tracker event ACL logic into
DefaultTrackerAccessManager, removing inline security checks from theSecurityTrackerEventValidator. That validator now delegates entirely to the access manager, simplifying the validation layer and consolidating access control logic in a single place.Notable changes
TrackedEntity,Enrollment, andTrackerEventnow returnList<ErrorMessage>instead ofList<String>, carrying structuredValidationCodesand args.canReadmethods fromList<String>toList<ErrorMessage>, aligning them with the rest of the interface. That doesn't affect the exporter's response, because theErrorMessageitself is never used there.canUpdatefor bothTrackedEntity,EnrollmentandTrackerEventnow accepts the payload org unit so capture scope can be checked only when the org unit actually changes. Previously the event validator inlined this check separately.checkOrgUnitInCaptureScope,checkDataWriteAccessToProgram,checkOwnershipAccess) so each can* method reads as a flat list of named checks rather than inline.MandatoryFieldsValidatorreturns an error if the org unit is not set. Same goes for the testcheckAccessPermissionForEnrollmentWhenOrgUnitIsNull. I think we test something that can't happen.