Bug 24841 - REST API should check if patron is restricted/debarred
Summary: REST API should check if patron is restricted/debarred
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 8 votes (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on: 17247
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-10 14:20 UTC by Magnus Enger
Modified: 2022-11-14 12:07 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2020-03-10 14:20:02 UTC
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?
Comment 1 Jonathan Druart 2020-03-11 09:18:54 UTC
Looks like the check must be moved to CanItemBeReserved
Comment 2 Jonathan Druart 2020-03-11 09:20:53 UTC
(In reply to Jonathan Druart from comment #1)
> Looks like the check must be moved to CanItemBeReserved

Or not, see 17247 comment 13
Comment 3 Magnus Enger 2020-03-11 09:38:51 UTC
(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?
Comment 4 Jonathan Druart 2020-03-11 10:03:40 UTC
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.
Comment 5 Nick Clemens 2021-01-14 13:59:11 UTC
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.
Comment 6 Arthur Suzuki 2021-01-15 08:27:29 UTC
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).
Comment 7 Arthur Suzuki 2021-01-15 08:27:54 UTC
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).
Comment 8 Tomás Cohen Arazi 2022-01-10 11:23:19 UTC
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.
Comment 9 Mathieu Saby 2022-11-13 14:00:26 UTC
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 ) ?
Comment 10 Arthur Suzuki 2022-11-14 12:02:32 UTC
(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
Comment 11 Tomás Cohen Arazi 2022-11-14 12:07:23 UTC
(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.