fix: [DHIS2-18551] (2.39) Fix access check when registering TEI #19372
Conversation
| @Override | ||
| @Transactional(readOnly = true) | ||
| public List<String> canCreate(User user, TrackedEntityInstance trackedEntity) { | ||
| if (user == null || user.isSuper() || trackedEntity == null) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ah wait, this is for 2.39, so nevermind, probably good to have the null check
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah for some reason, your second comment just got in. Yes Agreed :D
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| @Test | ||
| void shouldFailWhenRegisteringPersonOutsideCaptureScope() { | ||
|
|
There was a problem hiding this comment.
Usually, we don't include a line break between the method name and its code, and we format the test to follow the Arrange-Act-Assert pattern.
|
This is great, @ameenhere. Just for clarification: |
| @Override | ||
| @Transactional(readOnly = true) | ||
| public List<String> canCreate(User user, TrackedEntityInstance trackedEntity) { | ||
| if (user == null || user.isSuper() || trackedEntity == null) { |
There was a problem hiding this comment.
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
@zubaira My assumption is, if the user was able to register one TEI, he will be able to register any number of TEIs (Since the cached entry of ownerOU falls into his scope). If the user was not able to register a TEI because of an incorrect ownerOU (polluted cache), he will not be able to register any TEIs until the polluted cache is cleared. |
* fix: [DHIS2-18551] (2.39) Fix access check when registering TEI * Apply spotless * fix: Test setup of orgUnit Capture * fix: Review comments
…) (#19379) * fix: [DHIS2-18551] (2.39) Fix access check when registering TEI * Apply spotless * fix: Test setup of orgUnit Capture * fix: Review comments
When registering a new TEI, there is no context of a program. But the old Tracker API goes ahead and checks program ownership for “all the programs in the system”. This itself was only part of the problem.
There is a cache used to store programOwnerOu for a TEI-Program combination. For TE creation, when the TE is yet to be created, it will not have an id (the primary key column which is obtained after a TE is persisted). So during the time of TE creation, the entity object will always have an id of 0. This 0 is used as part of the key to cache programOwners. This means any subsequent TEI registration will also have 0 as the id and will pick up the "wrong owner OU" from the cache(the first one that got into the cache).
So irrelevant access checks + incorrect cache hits was the root cause of the original issue.
This PR fixes this by making sure programOwnership is not checked when registering a new TEI and rather simply check if the user has TE.orgUnit in their Capture Scope and if the user has Data Write access to the TE.TEType
This fix is consistent with the new Tracker API, in master. In v42(master), when creating a new TE, it only checks if the user has dataWrite access to the TeType and nothing of ownership is checked. It has separate access check flows when the TE is being created vs when the TE is being updated.