[Bug 24841] New: REST API should check if patron is restricted/debarred
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Bug ID: 24841 Summary: REST API should check if patron is restricted/debarred Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: magnus@libriotech.no Bug 17247 "ILS-DI HoldTitle and HoldItem should check if patron is restricted" adds checks to see if the patron is restricted before a hold is placed. These checks are added to C4::ILSDI::Services::HoldItem() and C4::ILSDI::Services::HoldTitle(). As far as I can tell, the same checks are missing from the REST API, specifically for "POST /api/v1/holds". Koha::REST::V1::Holds::add() does call C4::Reserves::CanItemBeReserved() and C4::Reserves::CanBookBeReserved(), but as far as I can tell, those subs do not check if the patron in question is debarred. Am I missing something or is this a bug? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Depends on| |17247 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Looks like the check must be moved to CanItemBeReserved Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17247 [Bug 17247] ILS-DI HoldTitle and HoldItem should check if patron is restricted -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #1)
Looks like the check must be moved to CanItemBeReserved
Or not, see 17247 comment 13 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 --- Comment #3 from Magnus Enger <magnus@libriotech.no> --- (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? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=26463 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |arthur.suzuki@biblibre.com --- Comment #6 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- 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). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25408 --- Comment #7 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- 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). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mathsabypro@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 --- Comment #9 from mathieu saby <mathsabypro@gmail.com> --- 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 ) ? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 --- Comment #10 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #12 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- In bug 39936 I proposed a more comprehensive approach to making restrictions configurable by restriction type, defining which actions should be blocked/allowed through which interface. That would handle this issue, in addition to others. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24841 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |DUPLICATE --- Comment #13 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- This has been addressed on bug 39657. *** This bug has been marked as a duplicate of bug 39657 *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org