Summary: | REST API should check if patron is restricted/debarred | ||
---|---|---|---|
Product: | Koha | Reporter: | Magnus Enger <magnus> |
Component: | REST API | Assignee: | Bugs List <koha-bugs> |
Status: | NEW --- | QA Contact: | |
Severity: | normal | ||
Priority: | P5 - low | CC: | arthur.suzuki, jonathan.druart, mathsabypro, nick, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26463 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 17247 | ||
Bug Blocks: |
Description
Magnus Enger
2020-03-10 14:20:02 UTC
Looks like the check must be moved to CanItemBeReserved (In reply to Jonathan Druart from comment #1) > Looks like the check must be moved to CanItemBeReserved Or not, see 17247 comment 13 (In reply to Jonathan Druart from comment #2) > Or not, see 17247 comment 13 Sorry, not sure I understand those comments. Are you saying the check should not be moved to CanItemBeReserved, because that would break the way things work in the staff client? If so, I assume you still agree there should be a check on debarred for the REST API, similar to what was added for ILS-DI? The first implementation of bug 17247 added the check to CanItemBeReserved. On bug 17247 comment 13 I noticed that it had a behavior that was not advertised (the change was not only modifying ILSDI, but also placing a hold from the interface, and also the REST API finally). Then Arthur moved the check out of CanItemBeReserved. This bug report shows that the initial check was correctly placed, the check must be put at lower (C4::Reserves) module. But in that case the different behaviors must be advertised, functionaly tested, validated, and covered by testes. The holds route is privileged, not public - it behaves as staff, not as OPAC - so it doesn't check if patron restricted/debarred We have had the issue come up with Aspen discovery layer as well, but I think we want is either a route for the patron placing their own holds, or a way to tell the API to act as if it is the opac. Hi there, What I did for bz25408 is I've used an argument in CanItemBeReserved so we can give different answers depending on the calling context. API could query CanItemBeReserved either like staff or opac depending on a parameter (but then the permissions will also have to be checked). Hi there, What I did for bz25408 is I've used an argument in CanItemBeReserved so we can give different answers depending on the calling context. API could query CanItemBeReserved either like staff or opac depending on a parameter (but then the permissions will also have to be checked). What is the status for this? My two cents would be: We have a header that is used here: x-koha-override: 'any', that will tell the controller to override blocker conditions. We could add a check like: if ( $patron->debarred ) { return $c->render( status => 400, ... ) unless $override_restricted_patron; } This is a workaround to the fact we don't have the 'holdability' routes on the API. Because if we wanted to implement the staff hold request flow with the API, we would certainly need to: - Ask if a hold can be placed - Prompt the user for confirmation, telling there are things to override - Actually place the hold, overridding checks So, as Nick says, we could also say this route shouldn't be checking things unless they are hard blockers for Koha's core business logic, and what's missing is the patron's (OPAC) perspective, or a route to check the situation for feedback. Hi My library is planning to use the holds API to mimic the behavior of the OPAC, and the current behavior is problematic. What about a new parameter to define the "context" of the action (?context=staff vs ?context=opac ) ? (In reply to mathieu saby from comment #9) > Hi > My library is planning to use the holds API to mimic the behavior of the > OPAC, and the current behavior is problematic. > > What about a new parameter to define the "context" of the action > (?context=staff vs ?context=opac ) ? Hi Mathieu, I totally agree with you. Moreover, this is what I've implemented in another bug I've been working on : https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 This one was for ILS-DI but some circulation rules are dependent of the context by design (opacitemholds especially). So, we need a way to set C4::Context->interface from API calls. I think this new parameter should be optional so as not to break any existing software that use this API. What do you think the default behavior should be if parameter is not set? Opac or Staff? I would recommend Opac since it is more restrictive. Arthur (In reply to mathieu saby from comment #9) > Hi > My library is planning to use the holds API to mimic the behavior of the > OPAC, and the current behavior is problematic. > > What about a new parameter to define the "context" of the action > (?context=staff vs ?context=opac ) ? By design, the /api/v1/holds endpoint is to be used as an admin user. As such, it has some -x-koha-override options (that can be enlarged, and it should prevent placing a hold under those problematic scenarios. If it doesn't it is because the underlying methods are not implementing the checks, as Arthur mentions. On the other hand, if you really want to do things as the patron (OPAC) we should be implementing a /api/v1/public/ route instead. |