fix: Allow granting temporary access only when user in search scope [DHIS2-18784] #19670
Conversation
muilpp
changed the title
| if (canSkipOwnershipCheck(user, program) || trackedEntity == null) { | ||
| 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"); |
There was a problem hiding this comment.
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 (canSkipOwnershipCheck(user, program) || trackedEntity == null) { | ||
| 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."); |
There was a problem hiding this comment.
Better saying 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 would apply here I think
...-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java
Outdated
Show resolved
Hide resolved
...-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java
Outdated
Show resolved
Hide resolved
...-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java
Outdated
Show resolved
Hide resolved
.../main/java/org/hisp/dhis/webapi/controller/tracker/ownership/TrackerOwnershipController.java
Outdated
Show resolved
Hide resolved
| programService.getProgram(program), | ||
| organisationUnitService.getOrganisationUnit(ou)); | ||
| return ok("Ownership transferred"); | ||
| } | ||
|
|
||
| @PostMapping(value = "/override", produces = APPLICATION_JSON_VALUE) | ||
| @ResponseBody | ||
| public WebMessage overrideOwnershipAccess( | ||
| public WebMessage grantTemporaryAccess( |
There was a problem hiding this comment.
Does this mean the path /override is also not accurate?
There was a problem hiding this comment.
To me, grantTemporaryAccess better represents the logic behind it.
I'd change the endpoint value too, but maybe @ameenhere has something to say here.
There was a problem hiding this comment.
From a strict RESTful naming convention, I guess having a verb is odd for the API path. APIs could have been api/tracker/ownership/permanent and api/tracker/ownership/temporary
But apart from that, override is not inaccurate. The functionality for this API is called "Breaking the glass" which is pretty much overriding/bypassing existing access check mechanisms. Atleast for now, I guess we really don't have to change API paths for this. Unless there is very strong reasonings, which I don't see at this point.
...-service-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java
Outdated
Show resolved
Hide resolved
…sp/dhis/tracker/acl/DefaultTrackerOwnershipManager.java Co-authored-by: Enrico Colasante <enrico@dhis2.org>
|
| @Nonnull TrackedEntity trackedEntity, Program program, UserDetails user, String reason) | ||
| throws ForbiddenException { | ||
| if (canSkipOwnershipCheck(user, program)) { | ||
| throw new ForbiddenException( |
There was a problem hiding this comment.
Doesn't this break the API?
I feel it may be intrusive to throw an exception for the scenario were the check itself could be skipped? Thoughts?
There was a problem hiding this comment.
The issue was that the check was used to skip the actual action that is granting temporary access.
In my opinion the API was just wrong because when the user was asking access for a not existent program, the API would return with a positive answer even though nothing happened.
There are two conditions required to be granted temporary access to an enrollment:
1. The program must be protected.
2. The TE/program owner must be within the user's search scope.
Currently, it is possible to gain temporary access to an enrollment even if the user is not within the search scope. This PR addresses and fixes that issue. After the fix, if the org unit is not within the search scope, an exception will be thrown.
Similarly, if the program is not protected, it will also throw an exception. Previously, access was denied in such cases, but no clear feedback was provided, which was confusing.
Additionally, I am removing the
assignOwnershipmethod, as it was only used in tests and is no longer necessary.Finally, when transferring ownerships, I am including the program parameter when searching for the TE. Since the program is available in the request, it makes more sense to check the access to that TE/program directly when transferring an enrollment to another org unit.
Ticket: https://dhis2.atlassian.net/browse/DHIS2-18784